Re: [PATCH net-next 09/11] ptp: ocp: Add debugfs entry for timecard

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 30 Aug 2021 16:52:34 -0700 Jonathan Lemon wrote:
> Provide a view into the timecard internals for debugging.
> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>

> +#ifdef CONFIG_DEBUG_FS
> +#define gpio_map(gpio, bit, pri, sec, def) ({			\
> +	char *_ans;						\
> +	if (gpio & (1 << bit))					\
> +		_ans = pri;					\
> +	else if (gpio & (1 << (bit + 16)))			\
> +		_ans = sec;					\
> +	else							\
> +		_ans = def;					\
> +	_ans;							\
> +})
> +
> +#define gpio_multi_map(buf, gpio, bit, pri, sec, def) ({	\
> +		char *_ans;					\
> +		_ans = buf;					\
> +		strcpy(buf, def);				\
> +		if (gpio & (1 << (bit + 16)))			\
> +			_ans += sprintf(_ans, "%s ", pri);	\
> +		if (gpio & (1 << bit))				\
> +			_ans += sprintf(_ans, "%s ", sec);	\
> +})

These can't be static inlines?

> +static int
> +ptp_ocp_summary_show(struct seq_file *s, void *data)
> +{
> +	struct device *dev = s->private;
> +	struct ts_reg __iomem *ts_reg;
> +	u32 sma_in, sma_out, val;
> +	struct timespec64 ts;
> +	struct ptp_ocp *bp;
> +	char *buf, *src;
> +	bool on;
> +
> +	buf = (char *)__get_free_page(GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	sma1_out_show(dev, NULL, buf);
> +	seq_printf(s, "   sma1: out from %s", buf);
> +
> +	sma2_out_show(dev, NULL, buf);
> +	seq_printf(s, "   sma2: out from %s", buf);
> +
> +	sma3_in_show(dev, NULL, buf);
> +	seq_printf(s, "   sma3: input to %s", buf);
> +
> +	sma4_in_show(dev, NULL, buf);
> +	seq_printf(s, "   sma4: input to %s", buf);

Why duplicate the data already available via sysfs?

> +static int
> +ptp_ocp_debugfs_add_device(struct ptp_ocp *bp)
> +{
> +	struct dentry *d;
> +
> +	d = debugfs_create_dir(dev_name(&bp->dev), ptp_ocp_debugfs_root);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);

Driver's are not supposed to depend on debugfs, you should be able to
carry on and all debugfs functions you pass an error pointer as a
parent will just return the same error right back.

> +	bp->debug_root = d;
> +
> +	d = debugfs_create_file("summary", 0444, bp->debug_root,
> +				&bp->dev, &ptp_ocp_summary_fops);
> +	if (IS_ERR(d))
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	debugfs_remove_recursive(bp->debug_root);
> +	bp->debug_root = NULL;
> +
> +	return PTR_ERR(d);
> +}
> +
> +static void
> +ptp_ocp_debugfs_remove_device(struct ptp_ocp *bp)
> +{
> +	debugfs_remove_recursive(bp->debug_root);
> +}
> +
> +static int
> +ptp_ocp_debugfs_init(void)
> +{
> +	struct dentry *d;
> +
> +	d = debugfs_create_dir("timecard", NULL);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);
> +
> +	ptp_ocp_debugfs_root = d;
> +
> +	return 0;
> +}
> +
> +static void
> +ptp_ocp_debugfs_fini(void)
> +{
> +	debugfs_remove_recursive(ptp_ocp_debugfs_root);
> +}
> +#else
> +#define ptp_ocp_debugfs_init()			0
> +#define ptp_ocp_debugfs_fini()
> +#define ptp_ocp_debugfs_add_device(bp)		0
> +#define ptp_ocp_debugfs_remove_device(bp)
> +#endif

This should not be necessary. Compiler should remove all those
functions as dead code when debugfs is not compiled in.

>  static void
>  ptp_ocp_dev_release(struct device *dev)
>  {