Re: [PATCH v2 2/2] staging: pi433: add debugfs interface

[Dan Carpenter <dan.carpenter@oracle.com>]

Since you're going to have to redo these anyway can you make some
additional changes?

On Mon, Jan 24, 2022 at 05:27:21PM +1300, Paulo Miguel Almeida wrote:
> +static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
> +{
> +	struct pi433_device *dev;
> +	u8 reg_data[114];
> +	size_t i;

int i; unless the sizes are really going to exceed 2 billion.

> +	char *fmt = "0x%02x, 0x%02x\n";
> +
> +	dev = m->private;
> +
> +	// acquire locks to avoid race conditions

This comment does not add any information.  Delete it.

> +	mutex_lock(&dev->tx_fifo_lock);
> +	mutex_lock(&dev->rx_lock);
> +
> +	// wait for on-going operations to finish
> +	if (dev->tx_active)

This condition is unnecessary, it's already checked in wait_event_interruptible().

> +		wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);

It makes me nervous that you're not checking the returns from these...

> +
> +	if (dev->rx_active)
> +		wait_event_interruptible(dev->tx_wait_queue, !dev->rx_active);
> +
> +	// read contiguous regs
> +	// skip FIFO register (0x0) otherwise this can affect some of uC ops
> +	for (i = 1; i < 0x50; i++)
> +		reg_data[i] = rf69_read_reg(dev->spi, i);
> +
> +	// read non-contiguous regs
> +	reg_data[REG_TESTLNA] = rf69_read_reg(dev->spi, REG_TESTLNA);
> +	reg_data[REG_TESTPA1] = rf69_read_reg(dev->spi, REG_TESTPA1);
> +	reg_data[REG_TESTPA2] = rf69_read_reg(dev->spi, REG_TESTPA2);
> +	reg_data[REG_TESTDAGC] = rf69_read_reg(dev->spi, REG_TESTDAGC);
> +	reg_data[REG_TESTAFC] = rf69_read_reg(dev->spi, REG_TESTAFC);
> +
> +	seq_puts(m, "# reg, val\n");
> +
> +	// print contiguous regs

These comments duplicate the comments a few lines earlier so they don't
add anything new.

> +	for (i = 1; i < 0x50; i++)
> +		seq_printf(m, fmt, i, reg_data[i]);
> +
> +	// print non-contiguous regs

Delete.

> +	seq_printf(m, fmt, REG_TESTLNA, reg_data[REG_TESTLNA]);
> +	seq_printf(m, fmt, REG_TESTPA1, reg_data[REG_TESTPA1]);
> +	seq_printf(m, fmt, REG_TESTPA2, reg_data[REG_TESTPA2]);
> +	seq_printf(m, fmt, REG_TESTDAGC, reg_data[REG_TESTDAGC]);
> +	seq_printf(m, fmt, REG_TESTAFC, reg_data[REG_TESTAFC]);
> +
> +	// release locks

Delete this comment

> +	mutex_unlock(&dev->tx_fifo_lock);
> +	mutex_unlock(&dev->rx_lock);

Could you flip these locks around so they mirror the start of the
function?  It doesn't affect runtime, but really it's nicer if the
ordering are always consistent.  ABBA.

> +
> +	return 0;
> +}
> +
> +static ssize_t pi433_debugfs_regs_open(struct inode *inode, struct file *filp)
> +{
> +	return single_open(filp, pi433_debugfs_regs_show, inode->i_private);
> +}
> +
> +static const struct file_operations debugfs_fops = {
> +	.llseek =	seq_lseek,
> +	.open =		pi433_debugfs_regs_open,
> +	.owner =	THIS_MODULE,
> +	.read =		seq_read,
> +	.release =	single_release
> +};
> +
>  /*-------------------------------------------------------------------------*/
>  
>  static int pi433_probe(struct spi_device *spi)
>  {
>  	struct pi433_device	*device;
> +	struct dentry		*entry; /* debugfs */

Delete the comment.  The variable name is not good.  "dir" would be
better.

>  	int			retval;
>  
>  	/* setup spi parameters */
> @@ -1256,6 +1324,11 @@ static int pi433_probe(struct spi_device *spi)
>  	/* spi setup */
>  	spi_set_drvdata(spi, device);
>  
> +	/* debugfs setup */

Delete comment (it does not add information).

> +	entry = debugfs_create_dir(dev_name(device->dev),
> +				   debugfs_lookup(KBUILD_MODNAME, NULL));
> +	debugfs_create_file("regs", 0400, entry, device, &debugfs_fops);
> +
>  	return 0;
>  
>  del_cdev:
> @@ -1279,6 +1352,10 @@ static int pi433_probe(struct spi_device *spi)
>  static int pi433_remove(struct spi_device *spi)
>  {
>  	struct pi433_device	*device = spi_get_drvdata(spi);
> +	struct dentry *mod_entry = debugfs_lookup(KBUILD_MODNAME, NULL);
> +
> +	/* debugfs */

Delete comment.

> +	debugfs_remove(debugfs_lookup(dev_name(device->dev), mod_entry));
>  
>  	/* free GPIOs */
>  	free_gpio(device);

regards,
dan carpenter