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/[email protected]/
- https://lore.kernel.org/lkml/[email protected]/
- https://lore.kernel.org/lkml/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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/[email protected]/
- https://lore.kernel.org/lkml/[email protected]/
- https://lore.kernel.org/lkml/[email protected]/
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/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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,
| ^~~~~~~~~~~~~~~~~~~~~~~
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
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.
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.
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 <[email protected]>
---
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/[email protected]/
---
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
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
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
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
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 <[email protected]>
---
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/[email protected]/
---
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
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 <[email protected]>
> ---
Thanks!
Reviewed-by: Dan Carpenter <[email protected]>
regards,
dan carpenter