| Message ID | 20240709-dev-iio-backend-add-debugfs-v1-3-fb4b8f2373c7@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad9467: add debugFS test mode support | expand |
On Tue, 9 Jul 2024 13:14:30 +0200 Nuno Sa <nuno.sa@analog.com> wrote: > This adds a basic debugfs interface for backends. Two new ops are being > added: > > * debugfs_reg_access: Analogous to the core IIO one but for backend > devices. > * debugfs_print_chan_status: One useful usecase for this one is for > testing test tones in a digital interface and "ask" the backend to > dump more details on why a test tone might have errors. > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> Debugfs deserved docs as well as sysfs. Same place in Documentation/ABI/ Obviously we've neglected this in the past, but nice to do it right nor new stuff. one trivial comment below. > + > +/** > + * iio_backend_debugfs_add - Add debugfs interfaces for Backends > + * @back: Backend device > + * @indio_dev: IIO device > + */ > +void iio_backend_debugfs_add(struct iio_backend *back, > + struct iio_dev *indio_dev) > +{ > + struct dentry *d = iio_get_debugfs_dentry(indio_dev); > + char attr_name[128]; > + > + if (!IS_ENABLED(CONFIG_DEBUG_FS)) > + return; If this happens, d will be null anyway... Maybe it's worth keeping as a form of local docs though. > + if (!back->ops->debugfs_reg_access || !d) > + return; > + > + snprintf(attr_name, sizeof(attr_name), "%s_direct_reg_access", > + back->name); > + > + debugfs_create_file(attr_name, 0644, d, back, > + &iio_backend_debugfs_reg_fops); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_debugfs_add, IIO_BACKEND); >
On Tue, 2024-07-16 at 19:14 +0100, Jonathan Cameron wrote: > On Tue, 9 Jul 2024 13:14:30 +0200 > Nuno Sa <nuno.sa@analog.com> wrote: > > > This adds a basic debugfs interface for backends. Two new ops are being > > added: > > > > * debugfs_reg_access: Analogous to the core IIO one but for backend > > devices. > > * debugfs_print_chan_status: One useful usecase for this one is for > > testing test tones in a digital interface and "ask" the backend to > > dump more details on why a test tone might have errors. > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > Debugfs deserved docs as well as sysfs. > Same place in Documentation/ABI/ > > Obviously we've neglected this in the past, but nice to do it right > nor new stuff. > I see. So you mean adding debugfs-iio? There's one thing I'm not sure though... I'm contemplating the case where one device may have multiple backends in which case I'm doing: back->name = name; where name comes from FW (DT usually). That obviously means the interface won't be always consistent which I guess it's not a real problem for debugfs? How would the interface look in the file? Something like? /sys/kernel/debug/iio/iio:deviceX/<backend_name>_direct_reg_access Or should we think in a more reliable naming? One option that came to mind is /sys/kernel/debug/iio/iio:deviceX/backendY_direct_reg_access where Y would be the corresponding index in io-backend-names. One thing not optimal with the above would be identifying the actual backend device. It would then maybe make sense having a 'backend_name' interface which I think is likely too much just for this? - Nuno Sá
On Thu, 18 Jul 2024 16:32:33 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Tue, 2024-07-16 at 19:14 +0100, Jonathan Cameron wrote: > > On Tue, 9 Jul 2024 13:14:30 +0200 > > Nuno Sa <nuno.sa@analog.com> wrote: > > > > > This adds a basic debugfs interface for backends. Two new ops are being > > > added: > > > > > > * debugfs_reg_access: Analogous to the core IIO one but for backend > > > devices. > > > * debugfs_print_chan_status: One useful usecase for this one is for > > > testing test tones in a digital interface and "ask" the backend to > > > dump more details on why a test tone might have errors. > > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > Debugfs deserved docs as well as sysfs. > > Same place in Documentation/ABI/ > > > > Obviously we've neglected this in the past, but nice to do it right > > nor new stuff. > > > > I see. So you mean adding debugfs-iio? Probably debugfs-iio-backend for this stuff, though we should have a more general doc as well. > > There's one thing I'm not sure though... I'm contemplating the case where one device > may have multiple backends in which case I'm doing: > > back->name = name; > > where name comes from FW (DT usually). That obviously means the interface won't be > always consistent which I guess it's not a real problem for debugfs? > > How would the interface look in the file? Something like? > > /sys/kernel/debug/iio/iio:deviceX/<backend_name>_direct_reg_access That's fine - fairly common sort of thing to see in debugfs. > > Or should we think in a more reliable naming? One option that came to mind is > > /sys/kernel/debug/iio/iio:deviceX/backendY_direct_reg_access If you were doing this it might be better as a directory. e.g. backendY/direct_reg_access > > where Y would be the corresponding index in io-backend-names. > > One thing not optimal with the above would be identifying the actual backend device. > It would then maybe make sense having a 'backend_name' interface which I think is > likely too much just for this? It kind of depends on your expected usecase. These are in debugfs so there is an assumption they aren't a 'normal operation' thing. So if they are going to typically be poked by a user, then complex file names are fine. If it's going to be scripted, then stable names something like backendY/name backendY/direct_reg_access etc would be easier to use. I'm not bothered as much about consistency of this debug interface as I would be about sysfs, so up to you (or other reviewers) for which you prefer. Jonathan > > - Nuno Sá > >
On Sat, 2024-07-20 at 10:43 +0100, Jonathan Cameron wrote: > On Thu, 18 Jul 2024 16:32:33 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Tue, 2024-07-16 at 19:14 +0100, Jonathan Cameron wrote: > > > On Tue, 9 Jul 2024 13:14:30 +0200 > > > Nuno Sa <nuno.sa@analog.com> wrote: > > > > > > > This adds a basic debugfs interface for backends. Two new ops are being > > > > added: > > > > > > > > * debugfs_reg_access: Analogous to the core IIO one but for backend > > > > devices. > > > > * debugfs_print_chan_status: One useful usecase for this one is for > > > > testing test tones in a digital interface and "ask" the backend to > > > > dump more details on why a test tone might have errors. > > > > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > > Debugfs deserved docs as well as sysfs. > > > Same place in Documentation/ABI/ > > > > > > Obviously we've neglected this in the past, but nice to do it right > > > nor new stuff. > > > > > > > I see. So you mean adding debugfs-iio? > > Probably debugfs-iio-backend for this stuff, though we should have > a more general doc as well. > > > > > There's one thing I'm not sure though... I'm contemplating the case where > > one device > > may have multiple backends in which case I'm doing: > > > > back->name = name; > > > > where name comes from FW (DT usually). That obviously means the interface > > won't be > > always consistent which I guess it's not a real problem for debugfs? > > > > How would the interface look in the file? Something like? > > > > /sys/kernel/debug/iio/iio:deviceX/<backend_name>_direct_reg_access > > That's fine - fairly common sort of thing to see in debugfs. > > > > > Or should we think in a more reliable naming? One option that came to mind > > is > > > > /sys/kernel/debug/iio/iio:deviceX/backendY_direct_reg_access > If you were doing this it might be better as a directory. > e.g. backendY/direct_reg_access > > > > where Y would be the corresponding index in io-backend-names. > > > > One thing not optimal with the above would be identifying the actual backend > > device. > > It would then maybe make sense having a 'backend_name' interface which I > > think is > > likely too much just for this? > It kind of depends on your expected usecase. These are in debugfs so there > is an assumption they aren't a 'normal operation' thing. So if they > are going to typically be poked by a user, then complex file names are fine. > If it's going to be scripted, then stable names something like > backendY/name > backendY/direct_reg_access etc > would be easier to use. > Yeah, I think the main usage (the one I do at least) is for the user to directly play and poke with this. However I don't think that the scripting usecase to be that crazy (or unlikely) and I do like stable things :). Also liked your suggestion about grouping the interfaces in a backendY directory. We'll likely need an iio_backend_info struct interface for backends to pass in when registering. Maybe too much for a debug interface but this kind of 'info' structure may very well be something we'll need in the future. Anyways, I think I'll give this a try for v2 so we can see how it looks like in practise. > I'm not bothered as much about consistency of this debug interface as I would > be about sysfs, so up to you (or other reviewers) for which you prefer. > Yeah, I was kind of expecting that (no one should blindly rely on debugFS) :) - Nuno Sá
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c index f9da635cdfea..52cbde0d5885 100644 --- a/drivers/iio/industrialio-backend.c +++ b/drivers/iio/industrialio-backend.c @@ -32,6 +32,7 @@ #define dev_fmt(fmt) "iio-backend: " fmt #include <linux/cleanup.h> +#include <linux/debugfs.h> #include <linux/device.h> #include <linux/err.h> #include <linux/errno.h> @@ -46,6 +47,8 @@ #include <linux/iio/backend.h> #include <linux/iio/iio.h> +#define IIO_BACKEND_DEFAULT_NAME "backend" + struct iio_backend { struct list_head entry; const struct iio_backend_ops *ops; @@ -53,6 +56,8 @@ struct iio_backend { struct device *dev; struct module *owner; void *priv; + const char *name; + unsigned int cached_reg_addr; }; /* @@ -117,6 +122,111 @@ static DEFINE_MUTEX(iio_back_lock); __stringify(op)); \ } +static ssize_t iio_backend_debugfs_read_reg(struct file *file, + char __user *userbuf, + size_t count, loff_t *ppos) +{ + struct iio_backend *back = file->private_data; + char read_buf[20]; + unsigned int val; + int ret, len; + + ret = iio_backend_op_call(back, debugfs_reg_access, + back->cached_reg_addr, 0, &val); + if (ret) + return ret; + + len = scnprintf(read_buf, sizeof(read_buf), "0x%X\n", val); + + return simple_read_from_buffer(userbuf, count, ppos, read_buf, len); +} + +static ssize_t iio_backend_debugfs_write_reg(struct file *file, + const char __user *userbuf, + size_t count, loff_t *ppos) +{ + struct iio_backend *back = file->private_data; + unsigned int val; + char buf[80]; + ssize_t rc; + int ret; + + rc = simple_write_to_buffer(buf, sizeof(buf), ppos, userbuf, count); + if (rc < 0) + return rc; + + ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); + + switch (ret) { + case 1: + return count; + case 2: + ret = iio_backend_op_call(back, debugfs_reg_access, + back->cached_reg_addr, val, NULL); + if (ret) + return ret; + return count; + default: + return -EINVAL; + } +} + +static const struct file_operations iio_backend_debugfs_reg_fops = { + .open = simple_open, + .read = iio_backend_debugfs_read_reg, + .write = iio_backend_debugfs_write_reg, +}; + +/** + * iio_backend_debugfs_add - Add debugfs interfaces for Backends + * @back: Backend device + * @indio_dev: IIO device + */ +void iio_backend_debugfs_add(struct iio_backend *back, + struct iio_dev *indio_dev) +{ + struct dentry *d = iio_get_debugfs_dentry(indio_dev); + char attr_name[128]; + + if (!IS_ENABLED(CONFIG_DEBUG_FS)) + return; + if (!back->ops->debugfs_reg_access || !d) + return; + + snprintf(attr_name, sizeof(attr_name), "%s_direct_reg_access", + back->name); + + debugfs_create_file(attr_name, 0644, d, back, + &iio_backend_debugfs_reg_fops); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_debugfs_add, IIO_BACKEND); + +/** + * iio_backend_debugfs_print_chan_status - Print channel status + * @back: Backend device + * @chan: Channel number + * @buf: Buffer where to print the status + * @len: Available space + * + * One usecase where this is useful is for testing test tones in a digital + * interface and "ask" the backend to dump more details on why a test tone might + * have errors. + * + * RETURNS: + * Number of copied bytes on success, negative error code on failure. + */ +ssize_t iio_backend_debugfs_print_chan_status(struct iio_backend *back, + unsigned int chan, char *buf, + size_t len) +{ + if (!IS_ENABLED(CONFIG_DEBUG_FS)) + return -ENODEV; + + return iio_backend_op_call(back, debugfs_print_chan_status, chan, buf, + len); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_debugfs_print_chan_status, IIO_BACKEND); + /** * iio_backend_chan_enable - Enable a backend channel * @back: Backend device @@ -577,6 +687,11 @@ struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name) if (ret) return ERR_PTR(ret); + if (name) + back->name = name; + else + back->name = IIO_BACKEND_DEFAULT_NAME; + return back; } diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h index 4e81931703ab..a643d86c7487 100644 --- a/include/linux/iio/backend.h +++ b/include/linux/iio/backend.h @@ -9,6 +9,7 @@ struct fwnode_handle; struct iio_backend; struct device; struct iio_dev; +struct dentry *d; enum iio_backend_data_type { IIO_BACKEND_TWOS_COMPLEMENT, @@ -22,6 +23,8 @@ enum iio_backend_data_source { IIO_BACKEND_DATA_SOURCE_MAX }; +#define iio_backend_debugfs_ptr(ptr) PTR_IF(IS_ENABLED(CONFIG_DEBUG_FS), ptr) + /** * IIO_BACKEND_EX_INFO - Helper for an IIO extended channel attribute * @_name: Attribute name @@ -81,6 +84,8 @@ enum iio_backend_sample_trigger { * @extend_chan_spec: Extend an IIO channel. * @ext_info_set: Extended info setter. * @ext_info_get: Extended info getter. + * @debugfs_print_chan_status: Print channel status into a buffer. + * @debugfs_reg_access: Read or write register value of backend. **/ struct iio_backend_ops { int (*enable)(struct iio_backend *back); @@ -113,6 +118,11 @@ struct iio_backend_ops { const char *buf, size_t len); int (*ext_info_get)(struct iio_backend *back, uintptr_t private, const struct iio_chan_spec *chan, char *buf); + int (*debugfs_print_chan_status)(struct iio_backend *back, + unsigned int chan, char *buf, + size_t len); + int (*debugfs_reg_access)(struct iio_backend *back, unsigned int reg, + unsigned int writeval, unsigned int *readval); }; int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan); @@ -152,5 +162,9 @@ __devm_iio_backend_get_from_fwnode_lookup(struct device *dev, int devm_iio_backend_register(struct device *dev, const struct iio_backend_ops *ops, void *priv); - +ssize_t iio_backend_debugfs_print_chan_status(struct iio_backend *back, + unsigned int chan, char *buf, + size_t len); +void iio_backend_debugfs_add(struct iio_backend *back, + struct iio_dev *indio_dev); #endif
This adds a basic debugfs interface for backends. Two new ops are being added: * debugfs_reg_access: Analogous to the core IIO one but for backend devices. * debugfs_print_chan_status: One useful usecase for this one is for testing test tones in a digital interface and "ask" the backend to dump more details on why a test tone might have errors. Signed-off-by: Nuno Sa <nuno.sa@analog.com> --- drivers/iio/industrialio-backend.c | 115 +++++++++++++++++++++++++++++++++++++ include/linux/iio/backend.h | 16 +++++- 2 files changed, 130 insertions(+), 1 deletion(-)