[PATCH 0/2] staging: pi433: add debugfs interface

[PATCH 0/2] staging: pi433: add debugfs interface

[Paulo Miguel Almeida]

When troubleshooting RF applications, one of the most common approaches is to
ensure that both sides of the communication path are using the same 
configuration such as bit rate, frequency deviation, encryption key, sync words
and so on.

The existing driver implementation doesn't allow the user to see which values
have been configured onto the uC which makes trobleshooting more painful than
it needs to be.

This patchset adds debugfs interface to this driver and exposes a read-only
access to uC reg values to address that problem.

Patch dependency:

This series depend on these patches as they change the same set of files:

- https://lore.kernel.org/lkml/20220108212728.GA7784@mail.google.com/
- https://lore.kernel.org/lkml/20220114221643.GA7843@mail.google.com/ 
- https://lore.kernel.org/lkml/20220118230312.GA4826@mail.google.com/

Paulo Miguel Almeida (2):
  staging: pi433: add missing register contants
  staging: pi433: add debugfs interface

 drivers/staging/pi433/Kconfig          |  2 +-
 drivers/staging/pi433/pi433_if.c       | 82 ++++++++++++++++++++++++++
 drivers/staging/pi433/rf69.c           |  2 +-
 drivers/staging/pi433/rf69.h           |  1 +
 drivers/staging/pi433/rf69_registers.h |  2 +
 5 files changed, 87 insertions(+), 2 deletions(-)

-- 
2.25.4

[PATCH 1/2] staging: pi433: add missing register contants

[Paulo Miguel Almeida]

add missing register constants present in RFM69 and/or RFM69HW so that
we don't need to hardcode values when referencing them.

this patch adds REG_TESTLNA, REG_TESTAFC constants

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 drivers/staging/pi433/rf69_registers.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
index a170c66c3d5b..0d6737738841 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -89,9 +89,11 @@
 #define  REG_AESKEY16			0x4D
 #define  REG_TEMP1			0x4E
 #define  REG_TEMP2			0x4F
+#define  REG_TESTLNA			0x58
 #define  REG_TESTPA1			0x5A /* only present on RFM69HW */
 #define  REG_TESTPA2			0x5C /* only present on RFM69HW */
 #define  REG_TESTDAGC			0x6F
+#define  REG_TESTAFC			0x71
 
 /******************************************************/
 /* RF69/SX1231 bit definition				*/
-- 
2.25.4

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

[Paulo Miguel Almeida]

This adds debugfs interface that can be used for debugging possible
hardware/software issues.

It currently exposes the following debugfs entries for each SPI device
probed:

  /sys/kernel/debug/pi433/<DEVICE>/regs
  ...

The 'regs' file contains all rf69 uC registers values that are useful
for troubleshooting misconfigurations between 2 devices. It contains one
register per line so it should be easy to use normal filtering tools to
find the registers of interest if needed.

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Meta-comments:

- I'm not entirely sure if I'm allowed to add additional dependencies to Kconfig
the way I did or if I should surround debugfs routines with 
#ifdef CONFIG_DEBUG_FS. I saw both approaches couldn't put my finger on which 
one is the 'right' way here. I'm taking suggestions :)

- I saw that in some other drivers there is a tendency to have debugfs routines
in a separate file such as debugfs.c and in that way this allows for smaller 
files (which I do like) and Makefile that build files based on selected 
configs such as:

pi433-$(CONFIG_DEBUG_FS) += debugfs.o 

The only way I could achieve such thing would be if I moved pi433_device struct
to pi433_if.h but I wanted to double check if reviewers would agree with this 
approach first.

---
 drivers/staging/pi433/Kconfig    |  2 +-
 drivers/staging/pi433/pi433_if.c | 82 ++++++++++++++++++++++++++++++++
 drivers/staging/pi433/rf69.c     |  2 +-
 drivers/staging/pi433/rf69.h     |  1 +
 4 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/pi433/Kconfig b/drivers/staging/pi433/Kconfig
index dd9e4709d1a8..9a8b7ef3e670 100644
--- a/drivers/staging/pi433/Kconfig
+++ b/drivers/staging/pi433/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 config PI433
 	tristate "Pi433 - a 433MHz radio module for Raspberry Pi"
-	depends on SPI
+	depends on SPI && DEBUG_FS
 	help
 	  This option allows you to enable support for the radio module Pi433.
 
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 17ff51f6a9da..e3a0d78385c0 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -41,6 +41,9 @@
 #ifdef CONFIG_COMPAT
 #include <linux/compat.h>
 #endif
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
 
 #include "pi433_if.h"
 #include "rf69.h"
@@ -56,6 +59,8 @@ static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
 
 static struct class *pi433_class; /* mainly for udev to create /dev/pi433 */
 
+static struct dentry *dbgfs_root_entry;
+
 /*
  * tx config is instance specific
  * so with each open a new tx config struct is needed
@@ -103,6 +108,9 @@ struct pi433_device {
 	bool			rx_active;
 	bool			tx_active;
 	bool			interrupt_rx_allowed;
+
+	/* debug fs */
+	struct dentry		*entry;
 };
 
 struct pi433_instance {
@@ -1102,6 +1110,72 @@ static const struct file_operations pi433_fops = {
 	.llseek =	no_llseek,
 };
 
+static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
+{
+	struct pi433_device *dev;
+	u8 reg_data[114];
+	size_t i;
+	char *fmt = "0x%02x, 0x%02x\n";
+
+	// obtain pi433_device reference
+	dev = m->private;
+
+	// acquire locks to avoid race conditions
+	mutex_lock(&dev->tx_fifo_lock);
+	mutex_lock(&dev->rx_lock);
+
+	// wait for on-going operations to finish
+	if (dev->tx_active)
+		wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
+
+	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
+	for (i = 1; i < 0x50; i++)
+		seq_printf(m, fmt, i, reg_data[i]);
+
+	// print non-contiguous regs
+	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
+	mutex_unlock(&dev->tx_fifo_lock);
+	mutex_unlock(&dev->rx_lock);
+
+	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)
@@ -1256,6 +1330,10 @@ static int pi433_probe(struct spi_device *spi)
 	/* spi setup */
 	spi_set_drvdata(spi, device);
 
+	/* debugfs setup */
+	device->entry = debugfs_create_dir(dev_name(device->dev), dbgfs_root_entry);
+	debugfs_create_file("regs", 0400, device->entry, device, &debugfs_fops);
+
 	return 0;
 
 del_cdev:
@@ -1353,6 +1431,8 @@ static int __init pi433_init(void)
 		return PTR_ERR(pi433_class);
 	}
 
+	dbgfs_root_entry = debugfs_create_dir("pi433", NULL);
+
 	status = spi_register_driver(&pi433_spi_driver);
 	if (status < 0) {
 		class_destroy(pi433_class);
@@ -1370,6 +1450,8 @@ static void __exit pi433_exit(void)
 	spi_unregister_driver(&pi433_spi_driver);
 	class_destroy(pi433_class);
 	unregister_chrdev(MAJOR(pi433_dev), pi433_spi_driver.driver.name);
+	debugfs_remove_recursive(dbgfs_root_entry);
+
 }
 module_exit(pi433_exit);
 
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index d60514a840c2..b1a3382f4042 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -24,7 +24,7 @@
 
 /*-------------------------------------------------------------------------*/
 
-static u8 rf69_read_reg(struct spi_device *spi, u8 addr)
+u8 rf69_read_reg(struct spi_device *spi, u8 addr)
 {
 	int retval;
 
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index c25942f142a6..3ff5609ecf2e 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -17,6 +17,7 @@
 #define FIFO_SIZE	66		/* bytes */
 #define FIFO_THRESHOLD	15		/* bytes */
 
+u8 rf69_read_reg(struct spi_device *spi, u8 addr);
 int rf69_get_version(struct spi_device *spi);
 int rf69_set_mode(struct spi_device *spi, enum mode mode);
 int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
-- 
2.25.4

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

[Greg KH]

On Sun, Jan 23, 2022 at 08:40:29PM +1300, Paulo Miguel Almeida wrote:
> This adds debugfs interface that can be used for debugging possible
> hardware/software issues.
> 
> It currently exposes the following debugfs entries for each SPI device
> probed:
> 
>   /sys/kernel/debug/pi433/<DEVICE>/regs
>   ...
> 
> The 'regs' file contains all rf69 uC registers values that are useful
> for troubleshooting misconfigurations between 2 devices. It contains one
> register per line so it should be easy to use normal filtering tools to
> find the registers of interest if needed.
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> Meta-comments:
> 
> - I'm not entirely sure if I'm allowed to add additional dependencies to Kconfig
> the way I did or if I should surround debugfs routines with 
> #ifdef CONFIG_DEBUG_FS. I saw both approaches couldn't put my finger on which 
> one is the 'right' way here. I'm taking suggestions :)

Neither is really needed at all.  The debugfs api will work properly if
the main config option is not enabled, and the code will be compiled
away properly.

So no need to add any dependancy to your driver at all.

debugfs was designed to be simple to use, and adding dependancies is not
simple.  Same goes for my comments below, the goal is to keep it simple
and not worry about any error handling.

> - I saw that in some other drivers there is a tendency to have debugfs routines
> in a separate file such as debugfs.c and in that way this allows for smaller 
> files (which I do like) and Makefile that build files based on selected 
> configs such as:
> 
> pi433-$(CONFIG_DEBUG_FS) += debugfs.o 

Again, not needed.

> The only way I could achieve such thing would be if I moved pi433_device struct
> to pi433_if.h but I wanted to double check if reviewers would agree with this 
> approach first.
> 
> ---
>  drivers/staging/pi433/Kconfig    |  2 +-
>  drivers/staging/pi433/pi433_if.c | 82 ++++++++++++++++++++++++++++++++
>  drivers/staging/pi433/rf69.c     |  2 +-
>  drivers/staging/pi433/rf69.h     |  1 +
>  4 files changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/pi433/Kconfig b/drivers/staging/pi433/Kconfig
> index dd9e4709d1a8..9a8b7ef3e670 100644
> --- a/drivers/staging/pi433/Kconfig
> +++ b/drivers/staging/pi433/Kconfig
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  config PI433
>  	tristate "Pi433 - a 433MHz radio module for Raspberry Pi"
> -	depends on SPI
> +	depends on SPI && DEBUG_FS

You can drop this.

>  	help
>  	  This option allows you to enable support for the radio module Pi433.
>  
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 17ff51f6a9da..e3a0d78385c0 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -41,6 +41,9 @@
>  #ifdef CONFIG_COMPAT
>  #include <linux/compat.h>
>  #endif
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +
>  
>  #include "pi433_if.h"
>  #include "rf69.h"
> @@ -56,6 +59,8 @@ static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
>  
>  static struct class *pi433_class; /* mainly for udev to create /dev/pi433 */
>  
> +static struct dentry *dbgfs_root_entry;

There is no need for this dentry.  Just look it up if you care about it.

> +
>  /*
>   * tx config is instance specific
>   * so with each open a new tx config struct is needed
> @@ -103,6 +108,9 @@ struct pi433_device {
>  	bool			rx_active;
>  	bool			tx_active;
>  	bool			interrupt_rx_allowed;
> +
> +	/* debug fs */
> +	struct dentry		*entry;

Again, no need for this, look it up if you need it.

>  };
>  
>  struct pi433_instance {
> @@ -1102,6 +1110,72 @@ static const struct file_operations pi433_fops = {
>  	.llseek =	no_llseek,
>  };
>  
> +static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
> +{
> +	struct pi433_device *dev;
> +	u8 reg_data[114];
> +	size_t i;
> +	char *fmt = "0x%02x, 0x%02x\n";
> +
> +	// obtain pi433_device reference
> +	dev = m->private;

That is not a "reference", that is just a normal empty pointer.  No need
to call it something else, that's just confusing.

> +
> +	// acquire locks to avoid race conditions
> +	mutex_lock(&dev->tx_fifo_lock);
> +	mutex_lock(&dev->rx_lock);
> +
> +	// wait for on-going operations to finish
> +	if (dev->tx_active)
> +		wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
> +
> +	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
> +	for (i = 1; i < 0x50; i++)
> +		seq_printf(m, fmt, i, reg_data[i]);
> +
> +	// print non-contiguous regs
> +	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
> +	mutex_unlock(&dev->tx_fifo_lock);
> +	mutex_unlock(&dev->rx_lock);
> +
> +	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)
> @@ -1256,6 +1330,10 @@ static int pi433_probe(struct spi_device *spi)
>  	/* spi setup */
>  	spi_set_drvdata(spi, device);
>  
> +	/* debugfs setup */
> +	device->entry = debugfs_create_dir(dev_name(device->dev), dbgfs_root_entry);

Make "entry" a local variable, and then pass it to the next call.

And look up dbgfs_root_entry as well.  This can be rewritten as:
	entry = debugfs_create_dir(dev_name(device->dev,
					    debugfs_lookup("pi433", NULL);

> +	debugfs_create_file("regs", 0400, device->entry, device, &debugfs_fops);

When do you ever remove the debugfs entry for the device?  I do not see
that in any release function here.  Did you forget about that?

> +
>  	return 0;
>  
>  del_cdev:
> @@ -1353,6 +1431,8 @@ static int __init pi433_init(void)
>  		return PTR_ERR(pi433_class);
>  	}
>  
> +	dbgfs_root_entry = debugfs_create_dir("pi433", NULL);

Again, no need to keep this around, see above.

> +
>  	status = spi_register_driver(&pi433_spi_driver);
>  	if (status < 0) {
>  		class_destroy(pi433_class);
> @@ -1370,6 +1450,8 @@ static void __exit pi433_exit(void)
>  	spi_unregister_driver(&pi433_spi_driver);
>  	class_destroy(pi433_class);
>  	unregister_chrdev(MAJOR(pi433_dev), pi433_spi_driver.driver.name);
> +	debugfs_remove_recursive(dbgfs_root_entry);

Can be rewritten as:
	debugfs_remove_recursive(debugfs_lookup("pi433", NULL));

Or better yet:
	debugfs_remove_recursive(debugfs_lookup(KBUILD_MODULE_NAME, NULL));

thanks,

greg k-h

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

[Paulo Miguel Almeida]

On Sun, Jan 23, 2022 at 12:20:57PM +0100, Greg KH wrote:
> On Sun, Jan 23, 2022 at 08:40:29PM +1300, Paulo Miguel Almeida wrote:
> > This adds debugfs interface that can be used for debugging possible
> > hardware/software issues.
> > 
> > It currently exposes the following debugfs entries for each SPI device
> > probed:
> > 
> >   /sys/kernel/debug/pi433/<DEVICE>/regs
> >   ...
> > 
> > The 'regs' file contains all rf69 uC registers values that are useful
> > for troubleshooting misconfigurations between 2 devices. It contains one
> > register per line so it should be easy to use normal filtering tools to
> > find the registers of interest if needed.
> > 
> > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> > ---
> > Meta-comments:
> > 
> > - I'm not entirely sure if I'm allowed to add additional dependencies to Kconfig
> > the way I did or if I should surround debugfs routines with 
> > #ifdef CONFIG_DEBUG_FS. I saw both approaches couldn't put my finger on which 
> > one is the 'right' way here. I'm taking suggestions :)
> 
> Neither is really needed at all.  The debugfs api will work properly if
> the main config option is not enabled, and the code will be compiled
> away properly.
> 
> So no need to add any dependancy to your driver at all.
> 
> debugfs was designed to be simple to use, and adding dependancies is not
> simple.  Same goes for my comments below, the goal is to keep it simple
> and not worry about any error handling.
> 
> > - I saw that in some other drivers there is a tendency to have debugfs routines
> > in a separate file such as debugfs.c and in that way this allows for smaller 
> > files (which I do like) and Makefile that build files based on selected 
> > configs such as:
> > 
> > pi433-$(CONFIG_DEBUG_FS) += debugfs.o 
> 
> Again, not needed.
> 
> > The only way I could achieve such thing would be if I moved pi433_device struct
> > to pi433_if.h but I wanted to double check if reviewers would agree with this 
> > approach first.
> > 
> > ---
> >  drivers/staging/pi433/Kconfig    |  2 +-
> >  drivers/staging/pi433/pi433_if.c | 82 ++++++++++++++++++++++++++++++++
> >  drivers/staging/pi433/rf69.c     |  2 +-
> >  drivers/staging/pi433/rf69.h     |  1 +
> >  4 files changed, 85 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/pi433/Kconfig b/drivers/staging/pi433/Kconfig
> > index dd9e4709d1a8..9a8b7ef3e670 100644
> > --- a/drivers/staging/pi433/Kconfig
> > +++ b/drivers/staging/pi433/Kconfig
> > @@ -1,7 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  config PI433
> >  	tristate "Pi433 - a 433MHz radio module for Raspberry Pi"
> > -	depends on SPI
> > +	depends on SPI && DEBUG_FS
> 
> You can drop this.
> 
> >  	help
> >  	  This option allows you to enable support for the radio module Pi433.
> >  
> > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > index 17ff51f6a9da..e3a0d78385c0 100644
> > --- a/drivers/staging/pi433/pi433_if.c
> > +++ b/drivers/staging/pi433/pi433_if.c
> > @@ -41,6 +41,9 @@
> >  #ifdef CONFIG_COMPAT
> >  #include <linux/compat.h>
> >  #endif
> > +#include <linux/debugfs.h>
> > +#include <linux/seq_file.h>
> > +
> >  
> >  #include "pi433_if.h"
> >  #include "rf69.h"
> > @@ -56,6 +59,8 @@ static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
> >  
> >  static struct class *pi433_class; /* mainly for udev to create /dev/pi433 */
> >  
> > +static struct dentry *dbgfs_root_entry;
> 
> There is no need for this dentry.  Just look it up if you care about it.
> 
> > +
> >  /*
> >   * tx config is instance specific
> >   * so with each open a new tx config struct is needed
> > @@ -103,6 +108,9 @@ struct pi433_device {
> >  	bool			rx_active;
> >  	bool			tx_active;
> >  	bool			interrupt_rx_allowed;
> > +
> > +	/* debug fs */
> > +	struct dentry		*entry;
> 
> Again, no need for this, look it up if you need it.
> 
> >  };
> >  
> >  struct pi433_instance {
> > @@ -1102,6 +1110,72 @@ static const struct file_operations pi433_fops = {
> >  	.llseek =	no_llseek,
> >  };
> >  
> > +static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
> > +{
> > +	struct pi433_device *dev;
> > +	u8 reg_data[114];
> > +	size_t i;
> > +	char *fmt = "0x%02x, 0x%02x\n";
> > +
> > +	// obtain pi433_device reference
> > +	dev = m->private;
> 
> That is not a "reference", that is just a normal empty pointer.  No need
> to call it something else, that's just confusing.
> 
> > +
> > +	// acquire locks to avoid race conditions
> > +	mutex_lock(&dev->tx_fifo_lock);
> > +	mutex_lock(&dev->rx_lock);
> > +
> > +	// wait for on-going operations to finish
> > +	if (dev->tx_active)
> > +		wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
> > +
> > +	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
> > +	for (i = 1; i < 0x50; i++)
> > +		seq_printf(m, fmt, i, reg_data[i]);
> > +
> > +	// print non-contiguous regs
> > +	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
> > +	mutex_unlock(&dev->tx_fifo_lock);
> > +	mutex_unlock(&dev->rx_lock);
> > +
> > +	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)
> > @@ -1256,6 +1330,10 @@ static int pi433_probe(struct spi_device *spi)
> >  	/* spi setup */
> >  	spi_set_drvdata(spi, device);
> >  
> > +	/* debugfs setup */
> > +	device->entry = debugfs_create_dir(dev_name(device->dev), dbgfs_root_entry);
> 
> Make "entry" a local variable, and then pass it to the next call.
> 
> And look up dbgfs_root_entry as well.  This can be rewritten as:
> 	entry = debugfs_create_dir(dev_name(device->dev,
> 					    debugfs_lookup("pi433", NULL);
> 
> > +	debugfs_create_file("regs", 0400, device->entry, device, &debugfs_fops);
> 
> When do you ever remove the debugfs entry for the device?  I do not see
> that in any release function here.  Did you forget about that?
> 
> > +
> >  	return 0;
> >  
> >  del_cdev:
> > @@ -1353,6 +1431,8 @@ static int __init pi433_init(void)
> >  		return PTR_ERR(pi433_class);
> >  	}
> >  
> > +	dbgfs_root_entry = debugfs_create_dir("pi433", NULL);
> 
> Again, no need to keep this around, see above.
> 
> > +
> >  	status = spi_register_driver(&pi433_spi_driver);
> >  	if (status < 0) {
> >  		class_destroy(pi433_class);
> > @@ -1370,6 +1450,8 @@ static void __exit pi433_exit(void)
> >  	spi_unregister_driver(&pi433_spi_driver);
> >  	class_destroy(pi433_class);
> >  	unregister_chrdev(MAJOR(pi433_dev), pi433_spi_driver.driver.name);
> > +	debugfs_remove_recursive(dbgfs_root_entry);
> 
> Can be rewritten as:
> 	debugfs_remove_recursive(debugfs_lookup("pi433", NULL));
> 
> Or better yet:
> 	debugfs_remove_recursive(debugfs_lookup(KBUILD_MODULE_NAME, NULL));
> 
> thanks,
> 
> greg k-h

thanks for taking the time to review this patchset. 

you are right, I will make the changes you pointed out and submit a new 
version of the patchset shortly.

thanks,

Paulo Almeida

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

[Paulo Miguel Almeida]

When troubleshooting RF applications, one of the most common approaches 
is to ensure that both sides of the communication path are using the 
same configuration such as bit rate, frequency deviation, encryption key,
sync words and so on.

The existing driver implementation doesn't allow the user to see which 
values have been configured onto the uC which makes trobleshooting more 
painful than it needs to be.

This patchset adds debugfs interface to this driver and exposes a 
read-only access to uC reg values to address that problem.

Patch dependency:

This series depend on these patches as they change the same set of files:

- https://lore.kernel.org/lkml/20220108212728.GA7784@mail.google.com/
- https://lore.kernel.org/lkml/20220114221643.GA7843@mail.google.com/ 
- https://lore.kernel.org/lkml/20220118230312.GA4826@mail.google.com/

Changelog:

v2: remove redudant references to dentry pointers in the code and perform 
     debugsfs_lookup instead. Req: Greg k-h  
v1: https://lore.kernel.org/lkml/20220123073855.GA79453@mail.google.com/

Paulo Miguel Almeida (2):
  staging: pi433: add missing register contants
  staging: pi433: add debugfs interface

 drivers/staging/pi433/pi433_if.c       | 80 ++++++++++++++++++++++++++
 drivers/staging/pi433/rf69.c           |  2 +-
 drivers/staging/pi433/rf69.h           |  1 +
 drivers/staging/pi433/rf69_registers.h |  2 +
 4 files changed, 84 insertions(+), 1 deletion(-)

-- 
2.25.4

[PATCH v2 1/2] staging: pi433: add missing register contants

[Paulo Miguel Almeida]

add missing register constants present in RFM69 and/or RFM69HW so that
we don't need to hardcode values when referencing them.

this patch adds REG_TESTLNA, REG_TESTAFC constants

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 drivers/staging/pi433/rf69_registers.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
index a170c66c3d5b..0d6737738841 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -89,9 +89,11 @@
 #define  REG_AESKEY16			0x4D
 #define  REG_TEMP1			0x4E
 #define  REG_TEMP2			0x4F
+#define  REG_TESTLNA			0x58
 #define  REG_TESTPA1			0x5A /* only present on RFM69HW */
 #define  REG_TESTPA2			0x5C /* only present on RFM69HW */
 #define  REG_TESTDAGC			0x6F
+#define  REG_TESTAFC			0x71
 
 /******************************************************/
 /* RF69/SX1231 bit definition				*/
-- 
2.25.4

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

[Paulo Miguel Almeida]

This adds debugfs interface that can be used for debugging possible
hardware/software issues.

It currently exposes the following debugfs entries for each SPI device
probed:

  /sys/kernel/debug/pi433/<DEVICE>/regs
  ...

The 'regs' file contains all rf69 uC registers values that are useful
for troubleshooting misconfigurations between 2 devices. It contains one
register per line so it should be easy to use normal filtering tools to
find the registers of interest if needed.

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 drivers/staging/pi433/pi433_if.c | 80 ++++++++++++++++++++++++++++++++
 drivers/staging/pi433/rf69.c     |  2 +-
 drivers/staging/pi433/rf69.h     |  1 +
 3 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 17ff51f6a9da..54bb2af2c2ea 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -41,6 +41,8 @@
 #ifdef CONFIG_COMPAT
 #include <linux/compat.h>
 #endif
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #include "pi433_if.h"
 #include "rf69.h"
@@ -1102,11 +1104,77 @@ static const struct file_operations pi433_fops = {
 	.llseek =	no_llseek,
 };
 
+static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
+{
+	struct pi433_device *dev;
+	u8 reg_data[114];
+	size_t i;
+	char *fmt = "0x%02x, 0x%02x\n";
+
+	dev = m->private;
+
+	// acquire locks to avoid race conditions
+	mutex_lock(&dev->tx_fifo_lock);
+	mutex_lock(&dev->rx_lock);
+
+	// wait for on-going operations to finish
+	if (dev->tx_active)
+		wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
+
+	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
+	for (i = 1; i < 0x50; i++)
+		seq_printf(m, fmt, i, reg_data[i]);
+
+	// print non-contiguous regs
+	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
+	mutex_unlock(&dev->tx_fifo_lock);
+	mutex_unlock(&dev->rx_lock);
+
+	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 */
 	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 */
+	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 */
+	debugfs_remove(debugfs_lookup(dev_name(device->dev), mod_entry));
 
 	/* free GPIOs */
 	free_gpio(device);
@@ -1353,6 +1430,8 @@ static int __init pi433_init(void)
 		return PTR_ERR(pi433_class);
 	}
 
+	debugfs_create_dir(KBUILD_MODNAME, NULL);
+
 	status = spi_register_driver(&pi433_spi_driver);
 	if (status < 0) {
 		class_destroy(pi433_class);
@@ -1370,6 +1449,7 @@ static void __exit pi433_exit(void)
 	spi_unregister_driver(&pi433_spi_driver);
 	class_destroy(pi433_class);
 	unregister_chrdev(MAJOR(pi433_dev), pi433_spi_driver.driver.name);
+	debugfs_remove_recursive(debugfs_lookup(KBUILD_MODNAME, NULL));
 }
 module_exit(pi433_exit);
 
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index d60514a840c2..b1a3382f4042 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -24,7 +24,7 @@
 
 /*-------------------------------------------------------------------------*/
 
-static u8 rf69_read_reg(struct spi_device *spi, u8 addr)
+u8 rf69_read_reg(struct spi_device *spi, u8 addr)
 {
 	int retval;
 
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index c25942f142a6..3ff5609ecf2e 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -17,6 +17,7 @@
 #define FIFO_SIZE	66		/* bytes */
 #define FIFO_THRESHOLD	15		/* bytes */
 
+u8 rf69_read_reg(struct spi_device *spi, u8 addr);
 int rf69_get_version(struct spi_device *spi);
 int rf69_set_mode(struct spi_device *spi, enum mode mode);
 int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
-- 
2.25.4

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

[Greg KH]

On Mon, Jan 24, 2022 at 05:27:21PM +1300, Paulo Miguel Almeida wrote:
> This adds debugfs interface that can be used for debugging possible
> hardware/software issues.
> 
> It currently exposes the following debugfs entries for each SPI device
> probed:
> 
>   /sys/kernel/debug/pi433/<DEVICE>/regs
>   ...
> 
> The 'regs' file contains all rf69 uC registers values that are useful
> for troubleshooting misconfigurations between 2 devices. It contains one
> register per line so it should be easy to use normal filtering tools to
> find the registers of interest if needed.
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
>  drivers/staging/pi433/pi433_if.c | 80 ++++++++++++++++++++++++++++++++
>  drivers/staging/pi433/rf69.c     |  2 +-
>  drivers/staging/pi433/rf69.h     |  1 +
>  3 files changed, 82 insertions(+), 1 deletion(-)

Breaks the build:

drivers/staging/pi433/pi433_if.c:1166:25: error: initialization of ‘int (*)(struct inode *, struct file *)’ from incompatible pointer type ‘ssize_t (*)(struct inode *, struct file *)’ {aka ‘long int (*)(struct inode *, struct file *)’} [-Werror=incompatible-pointer-types]
 1166 |         .open =         pi433_debugfs_regs_open,
      |                         ^~~~~~~~~~~~~~~~~~~~~~~

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

[Dan Carpenter]

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

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

[Paulo Miguel Almeida]

On Wed, Jan 26, 2022 at 04:21:16PM +0300, Dan Carpenter wrote:
> Since you're going to have to redo these anyway can you make some
> additional changes?
> ...... 
> 

Hi Dan, thanks for reviewing this patch. I will make those changes 
and submit a new version of the patch shortly.

thanks,

Paulo A.

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

[Paulo Miguel Almeida]

On Wed, Jan 26, 2022 at 01:03:03PM +0100, Greg KH wrote:
> 
> Breaks the build:
> 
> drivers/staging/pi433/pi433_if.c:1166:25: error: initialization of ‘int (*)(struct inode *, struct file *)’ from incompatible pointer type ‘ssize_t (*)(struct inode *, struct file *)’ {aka ‘long int (*)(struct inode *, struct file *)’} [-Werror=incompatible-pointer-types]
>  1166 |         .open =         pi433_debugfs_regs_open,
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~
> 
> 

My apologies, I will be more careful next time. I must have missed
something out on my module-specific makefile. 

I will incorporate the changes that Dan Carpenter suggested and submit a
new version of this patch shortly.

thanks,

Paulo A.

[PATCH v3] staging: pi433: add debugfs interface

[Paulo Miguel Almeida]

This adds debugfs interface that can be used for debugging possible
hardware/software issues.

It currently exposes the following debugfs entries for each SPI device
probed:

  /sys/kernel/debug/pi433/<DEVICE>/regs
  ...

The 'regs' file contains all rf69 uC registers values that are useful
for troubleshooting misconfigurations between 2 devices. It contains one
register per line so it should be easy to use normal filtering tools to
find the registers of interest if needed.

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Changelog:

v3: fix build error, remove redundant comments and validate return values
	from wait_event_interruptible routines: Req: Greg K-h, Dan Carpenter
v2: remove redudant references to dentry pointers in the code and perform 
     debugsfs_lookup instead. Req: Greg k-h  
v1: https://lore.kernel.org/lkml/20220123073855.GA79453@mail.google.com/
---
 drivers/staging/pi433/pi433_if.c | 75 ++++++++++++++++++++++++++++++++
 drivers/staging/pi433/rf69.c     |  2 +-
 drivers/staging/pi433/rf69.h     |  1 +
 3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 17ff51f6a..74748d3cd 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -41,6 +41,8 @@
 #ifdef CONFIG_COMPAT
 #include <linux/compat.h>
 #endif
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #include "pi433_if.h"
 #include "rf69.h"
@@ -1102,12 +1104,75 @@ static const struct file_operations pi433_fops = {
 	.llseek =	no_llseek,
 };
 
+static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
+{
+	struct pi433_device *dev;
+	u8 reg_data[114];
+	int i;
+	char *fmt = "0x%02x, 0x%02x\n";
+	int ret = 0;
+
+	dev = m->private;
+
+	mutex_lock(&dev->tx_fifo_lock);
+	mutex_lock(&dev->rx_lock);
+
+	// wait for on-going operations to finish
+	ret = wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
+	if (ret)
+		return ret;
+
+	ret = wait_event_interruptible(dev->tx_wait_queue, !dev->rx_active);
+	if (ret)
+		return ret;
+
+	// 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);
+
+	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");
+
+	for (i = 1; i < 0x50; i++)
+		seq_printf(m, fmt, i, reg_data[i]);
+
+	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]);
+
+	mutex_unlock(&dev->rx_lock);
+	mutex_unlock(&dev->tx_fifo_lock);
+
+	return ret;
+}
+
+static int 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;
 	int			retval;
+	struct dentry		*entry;
 
 	/* setup spi parameters */
 	spi->mode = 0x00;
@@ -1256,6 +1321,10 @@ static int pi433_probe(struct spi_device *spi)
 	/* spi setup */
 	spi_set_drvdata(spi, device);
 
+	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 +1348,9 @@ 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_remove(debugfs_lookup(dev_name(device->dev), mod_entry));
 
 	/* free GPIOs */
 	free_gpio(device);
@@ -1353,6 +1425,8 @@ static int __init pi433_init(void)
 		return PTR_ERR(pi433_class);
 	}
 
+	debugfs_create_dir(KBUILD_MODNAME, NULL);
+
 	status = spi_register_driver(&pi433_spi_driver);
 	if (status < 0) {
 		class_destroy(pi433_class);
@@ -1370,6 +1444,7 @@ static void __exit pi433_exit(void)
 	spi_unregister_driver(&pi433_spi_driver);
 	class_destroy(pi433_class);
 	unregister_chrdev(MAJOR(pi433_dev), pi433_spi_driver.driver.name);
+	debugfs_remove_recursive(debugfs_lookup(KBUILD_MODNAME, NULL));
 }
 module_exit(pi433_exit);
 
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index d60514a84..b1a3382f4 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -24,7 +24,7 @@
 
 /*-------------------------------------------------------------------------*/
 
-static u8 rf69_read_reg(struct spi_device *spi, u8 addr)
+u8 rf69_read_reg(struct spi_device *spi, u8 addr)
 {
 	int retval;
 
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index c25942f14..3ff5609ec 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -17,6 +17,7 @@
 #define FIFO_SIZE	66		/* bytes */
 #define FIFO_THRESHOLD	15		/* bytes */
 
+u8 rf69_read_reg(struct spi_device *spi, u8 addr);
 int rf69_get_version(struct spi_device *spi);
 int rf69_set_mode(struct spi_device *spi, enum mode mode);
 int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
-- 
2.34.1

Re: [PATCH v3] staging: pi433: add debugfs interface

[Dan Carpenter]

On Sun, Jan 30, 2022 at 03:57:26PM +1300, Paulo Miguel Almeida wrote:
> @@ -1102,12 +1104,75 @@ static const struct file_operations pi433_fops = {
>  	.llseek =	no_llseek,
>  };
>  
> +static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
> +{
> +	struct pi433_device *dev;
> +	u8 reg_data[114];
> +	int i;
> +	char *fmt = "0x%02x, 0x%02x\n";
> +	int ret = 0;

No need to initialize.  Bogus initializers just disable ten thousand
person hours spent developing static analysis.

> +
> +	dev = m->private;
> +
> +	mutex_lock(&dev->tx_fifo_lock);
> +	mutex_lock(&dev->rx_lock);
> +
> +	// wait for on-going operations to finish
> +	ret = wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
> +	if (ret)
> +		return ret;

Drop the two mutexes before returning.

> +
> +	ret = wait_event_interruptible(dev->tx_wait_queue, !dev->rx_active);
> +	if (ret)
> +		return ret;

Drop the mutexes.

> +
> +	// 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);
> +
> +	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");
> +
> +	for (i = 1; i < 0x50; i++)
> +		seq_printf(m, fmt, i, reg_data[i]);
> +
> +	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]);
> +
> +	mutex_unlock(&dev->rx_lock);
> +	mutex_unlock(&dev->tx_fifo_lock);
> +
> +	return ret;
> +}

regards,
dan carpenter

Re: [PATCH v3] staging: pi433: add debugfs interface

[Greg KH]

On Mon, Jan 31, 2022 at 04:45:58PM +0300, Dan Carpenter wrote:
> On Sun, Jan 30, 2022 at 03:57:26PM +1300, Paulo Miguel Almeida wrote:
> > @@ -1102,12 +1104,75 @@ static const struct file_operations pi433_fops = {
> >  	.llseek =	no_llseek,
> >  };
> >  
> > +static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
> > +{
> > +	struct pi433_device *dev;
> > +	u8 reg_data[114];
> > +	int i;
> > +	char *fmt = "0x%02x, 0x%02x\n";
> > +	int ret = 0;
> 
> No need to initialize.  Bogus initializers just disable ten thousand
> person hours spent developing static analysis.
> 
> > +
> > +	dev = m->private;
> > +
> > +	mutex_lock(&dev->tx_fifo_lock);
> > +	mutex_lock(&dev->rx_lock);
> > +
> > +	// wait for on-going operations to finish
> > +	ret = wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
> > +	if (ret)
> > +		return ret;
> 
> Drop the two mutexes before returning.

Ick, I missed that.  I'll go drop this patch from my tree now, good
catch.

greg k-h

Re: [PATCH v3] staging: pi433: add debugfs interface

[Paulo Miguel Almeida]

On Mon, Jan 31, 2022 at 04:45:58PM +0300, Dan Carpenter wrote:
> On Sun, Jan 30, 2022 at 03:57:26PM +1300, Paulo Miguel Almeida wrote:
> > +	dev = m->private;
> > +
> > +	mutex_lock(&dev->tx_fifo_lock);
> > +	mutex_lock(&dev->rx_lock);
> > +
> > +	// wait for on-going operations to finish
> > +	ret = wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
> > +	if (ret)
> > +		return ret;
> 
> Drop the two mutexes before returning.
> 


thanks for taking the time for reviewing this patch.

good catch, I completely missed it. Thanks a lot!

thanks,

Paulo Almeida

[PATCH v4] staging: pi433: add debugfs interface

[Paulo Miguel Almeida]

This adds debugfs interface that can be used for debugging possible
hardware/software issues.

It currently exposes the following debugfs entries for each SPI device
probed:

  /sys/kernel/debug/pi433/<DEVICE>/regs
  ...

The 'regs' file contains all rf69 uC registers values that are useful
for troubleshooting misconfigurations between 2 devices. It contains one
register per line so it should be easy to use normal filtering tools to
find the registers of interest if needed.

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Changelog:

v4: remove unnecessary variable initializer, ensure mutex locks are
	released before returning from routine. Req: Dan Carpenter
v3: fix build error, remove redundant comments and validate return values
        from wait_event_interruptible routines: Req: Greg K-h, Dan Carpenter
v2: remove redudant references to dentry pointers in the code and perform
     debugsfs_lookup instead. Req: Greg k-h
v1: https://lore.kernel.org/lkml/20220123073855.GA79453@mail.google.com/
---
 drivers/staging/pi433/pi433_if.c | 76 ++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 86ad497417f7..4fbac3ccef74 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -41,6 +41,8 @@
 #ifdef CONFIG_COMPAT
 #include <linux/compat.h>
 #endif
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #include "pi433_if.h"
 #include "rf69.h"
@@ -1098,12 +1100,76 @@ static const struct file_operations pi433_fops = {
 	.llseek =	no_llseek,
 };
 
+static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
+{
+	struct pi433_device *dev;
+	u8 reg_data[114];
+	int i;
+	char *fmt = "0x%02x, 0x%02x\n";
+	int ret;
+
+	dev = m->private;
+
+	mutex_lock(&dev->tx_fifo_lock);
+	mutex_lock(&dev->rx_lock);
+
+	// wait for on-going operations to finish
+	ret = wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
+	if (ret)
+		goto out_unlock;
+
+	ret = wait_event_interruptible(dev->tx_wait_queue, !dev->rx_active);
+	if (ret)
+		goto out_unlock;
+
+	// 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);
+
+	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");
+
+	for (i = 1; i < 0x50; i++)
+		seq_printf(m, fmt, i, reg_data[i]);
+
+	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]);
+
+out_unlock:
+	mutex_unlock(&dev->rx_lock);
+	mutex_unlock(&dev->tx_fifo_lock);
+
+	return ret;
+}
+
+static int 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;
 	int			retval;
+	struct dentry		*entry;
 
 	/* setup spi parameters */
 	spi->mode = 0x00;
@@ -1252,6 +1318,10 @@ static int pi433_probe(struct spi_device *spi)
 	/* spi setup */
 	spi_set_drvdata(spi, device);
 
+	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:
@@ -1275,6 +1345,9 @@ 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_remove(debugfs_lookup(dev_name(device->dev), mod_entry));
 
 	/* free GPIOs */
 	free_gpio(device);
@@ -1349,6 +1422,8 @@ static int __init pi433_init(void)
 		return PTR_ERR(pi433_class);
 	}
 
+	debugfs_create_dir(KBUILD_MODNAME, NULL);
+
 	status = spi_register_driver(&pi433_spi_driver);
 	if (status < 0) {
 		class_destroy(pi433_class);
@@ -1366,6 +1441,7 @@ static void __exit pi433_exit(void)
 	spi_unregister_driver(&pi433_spi_driver);
 	class_destroy(pi433_class);
 	unregister_chrdev(MAJOR(pi433_dev), pi433_spi_driver.driver.name);
+	debugfs_remove_recursive(debugfs_lookup(KBUILD_MODNAME, NULL));
 }
 module_exit(pi433_exit);
 
-- 
2.34.1

[PATCH v5] staging: pi433: add debugfs interface

[Paulo Miguel Almeida]

This adds debugfs interface that can be used for debugging possible
hardware/software issues.

It currently exposes the following debugfs entries for each SPI device
probed:

  /sys/kernel/debug/pi433/<DEVICE>/regs
  ...

The 'regs' file contains all rf69 uC registers values that are useful
for troubleshooting misconfigurations between 2 devices. It contains one
register per line so it should be easy to use normal filtering tools to
find the registers of interest if needed.

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Changelog:

v5: I had mistakenly forgotten to git add changes to the rf69.(c|h) in
    the previous patch
v4: remove unnecessary variable initializer, ensure mutex locks are
        released before returning from routine. Req: Dan Carpenter
v3: fix build error, remove redundant comments and validate return values
        from wait_event_interruptible routines: Req: Greg K-h, Dan Carpenter
v2: remove redudant references to dentry pointers in the code and perform
     debugsfs_lookup instead. Req: Greg k-h
v1: https://lore.kernel.org/lkml/20220123073855.GA79453@mail.google.com/
---
drivers/staging/pi433/pi433_if.c | 76 ++++++++++++++++++++++++++++++++
 drivers/staging/pi433/rf69.c     |  2 +-
 drivers/staging/pi433/rf69.h     |  1 +
 3 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 86ad497417f7..4fbac3ccef74 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -41,6 +41,8 @@
 #ifdef CONFIG_COMPAT
 #include <linux/compat.h>
 #endif
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #include "pi433_if.h"
 #include "rf69.h"
@@ -1098,12 +1100,76 @@ static const struct file_operations pi433_fops = {
 	.llseek =	no_llseek,
 };
 
+static int pi433_debugfs_regs_show(struct seq_file *m, void *p)
+{
+	struct pi433_device *dev;
+	u8 reg_data[114];
+	int i;
+	char *fmt = "0x%02x, 0x%02x\n";
+	int ret;
+
+	dev = m->private;
+
+	mutex_lock(&dev->tx_fifo_lock);
+	mutex_lock(&dev->rx_lock);
+
+	// wait for on-going operations to finish
+	ret = wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
+	if (ret)
+		goto out_unlock;
+
+	ret = wait_event_interruptible(dev->tx_wait_queue, !dev->rx_active);
+	if (ret)
+		goto out_unlock;
+
+	// 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);
+
+	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");
+
+	for (i = 1; i < 0x50; i++)
+		seq_printf(m, fmt, i, reg_data[i]);
+
+	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]);
+
+out_unlock:
+	mutex_unlock(&dev->rx_lock);
+	mutex_unlock(&dev->tx_fifo_lock);
+
+	return ret;
+}
+
+static int 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;
 	int			retval;
+	struct dentry		*entry;
 
 	/* setup spi parameters */
 	spi->mode = 0x00;
@@ -1252,6 +1318,10 @@ static int pi433_probe(struct spi_device *spi)
 	/* spi setup */
 	spi_set_drvdata(spi, device);
 
+	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:
@@ -1275,6 +1345,9 @@ 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_remove(debugfs_lookup(dev_name(device->dev), mod_entry));
 
 	/* free GPIOs */
 	free_gpio(device);
@@ -1349,6 +1422,8 @@ static int __init pi433_init(void)
 		return PTR_ERR(pi433_class);
 	}
 
+	debugfs_create_dir(KBUILD_MODNAME, NULL);
+
 	status = spi_register_driver(&pi433_spi_driver);
 	if (status < 0) {
 		class_destroy(pi433_class);
@@ -1366,6 +1441,7 @@ static void __exit pi433_exit(void)
 	spi_unregister_driver(&pi433_spi_driver);
 	class_destroy(pi433_class);
 	unregister_chrdev(MAJOR(pi433_dev), pi433_spi_driver.driver.name);
+	debugfs_remove_recursive(debugfs_lookup(KBUILD_MODNAME, NULL));
 }
 module_exit(pi433_exit);
 
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index c68ad4809e91..2ab3bf46e37d 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -24,7 +24,7 @@
 
 /*-------------------------------------------------------------------------*/
 
-static u8 rf69_read_reg(struct spi_device *spi, u8 addr)
+u8 rf69_read_reg(struct spi_device *spi, u8 addr)
 {
 	int retval;
 
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index c25942f142a6..3ff5609ecf2e 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -17,6 +17,7 @@
 #define FIFO_SIZE	66		/* bytes */
 #define FIFO_THRESHOLD	15		/* bytes */
 
+u8 rf69_read_reg(struct spi_device *spi, u8 addr);
 int rf69_get_version(struct spi_device *spi);
 int rf69_set_mode(struct spi_device *spi, enum mode mode);
 int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
-- 
2.34.1

Re: [PATCH v4] staging: pi433: add debugfs interface

[Dan Carpenter]

On Fri, Feb 04, 2022 at 06:06:09PM +1300, Paulo Miguel Almeida wrote:
> This adds debugfs interface that can be used for debugging possible
> hardware/software issues.
> 
> It currently exposes the following debugfs entries for each SPI device
> probed:
> 
>   /sys/kernel/debug/pi433/<DEVICE>/regs
>   ...
> 
> The 'regs' file contains all rf69 uC registers values that are useful
> for troubleshooting misconfigurations between 2 devices. It contains one
> register per line so it should be easy to use normal filtering tools to
> find the registers of interest if needed.
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH v5] staging: pi433: add debugfs interface

[Dan Carpenter]

On Fri, Feb 04, 2022 at 09:04:32PM +1300, Paulo Miguel Almeida wrote:
> This adds debugfs interface that can be used for debugging possible
> hardware/software issues.
> 
> It currently exposes the following debugfs entries for each SPI device
> probed:
> 
>   /sys/kernel/debug/pi433/<DEVICE>/regs
>   ...
> 
> The 'regs' file contains all rf69 uC registers values that are useful
> for troubleshooting misconfigurations between 2 devices. It contains one
> register per line so it should be easy to use normal filtering tools to
> find the registers of interest if needed.
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> Changelog:
> 
> v5: I had mistakenly forgotten to git add changes to the rf69.(c|h) in
>     the previous patch

Oops.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter