* [PATCH 0/2] iio: Add "extended_name" attribute @ 2021-06-10 12:45 Paul Cercueil 2021-06-10 12:45 ` [PATCH 1/2] iio: core: Support removing extended name in attribute filename Paul Cercueil 2021-06-10 12:45 ` [PATCH 2/2] iio: core: Add "extended_name" attribute to all channels Paul Cercueil 0 siblings, 2 replies; 13+ messages in thread From: Paul Cercueil @ 2021-06-10 12:45 UTC (permalink / raw) To: Jonathan Cameron Cc: Lars-Peter Clausen, Michael Hennerich, linux-iio, linux-kernel, Paul Cercueil Hi Jonathan, Here's a small change to address an issue I've been facing since the beginning in libiio, namely that the IIO tree in sysfs is not easily parsable (read more about it in patch #2's summary). Any feedback welcome. Cheers, -Paul Paul Cercueil (2): iio: core: Support removing extended name in attribute filename iio: core: Add "extended_name" attribute to all channels drivers/iio/iio_core.h | 3 +- drivers/iio/industrialio-buffer.c | 12 +++-- drivers/iio/industrialio-core.c | 73 ++++++++++++++++++++++++++----- drivers/iio/industrialio-event.c | 3 +- 4 files changed, 74 insertions(+), 17 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] iio: core: Support removing extended name in attribute filename 2021-06-10 12:45 [PATCH 0/2] iio: Add "extended_name" attribute Paul Cercueil @ 2021-06-10 12:45 ` Paul Cercueil 2021-06-10 12:58 ` Andy Shevchenko 2021-06-10 12:45 ` [PATCH 2/2] iio: core: Add "extended_name" attribute to all channels Paul Cercueil 1 sibling, 1 reply; 13+ messages in thread From: Paul Cercueil @ 2021-06-10 12:45 UTC (permalink / raw) To: Jonathan Cameron Cc: Lars-Peter Clausen, Michael Hennerich, linux-iio, linux-kernel, Paul Cercueil By default, when a channel has an extended name, it will appear in the filename of channel attributes. E.g. if the extended name is "aux", the filename of a "sample_rate" attribute will be something like: in_voltage0_aux_sample_rate Add a mechanism to disable this feature. This will be used to add a "extended_name" channel attribute. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- drivers/iio/iio_core.h | 3 ++- drivers/iio/industrialio-buffer.c | 12 ++++++++---- drivers/iio/industrialio-core.c | 32 ++++++++++++++++++++----------- drivers/iio/industrialio-event.c | 3 ++- 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h index 8f4a9b264962..2e1a251d5550 100644 --- a/drivers/iio/iio_core.h +++ b/drivers/iio/iio_core.h @@ -50,7 +50,8 @@ int __iio_add_chan_devattr(const char *postfix, enum iio_shared_by shared_by, struct device *dev, struct iio_buffer *buffer, - struct list_head *attr_list); + struct list_head *attr_list, + bool extend_name); void iio_free_chan_devattr_list(struct list_head *attr_list); int iio_device_register_sysfs_group(struct iio_dev *indio_dev, diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 9a8e16c7e9af..053f8896c81c 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -492,7 +492,8 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, IIO_SEPARATE, &indio_dev->dev, buffer, - &buffer->buffer_attr_list); + &buffer->buffer_attr_list, + true); if (ret) return ret; attrcount++; @@ -504,7 +505,8 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, 0, &indio_dev->dev, buffer, - &buffer->buffer_attr_list); + &buffer->buffer_attr_list, + true); if (ret) return ret; attrcount++; @@ -517,7 +519,8 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, 0, &indio_dev->dev, buffer, - &buffer->buffer_attr_list); + &buffer->buffer_attr_list, + true); else ret = __iio_add_chan_devattr("en", chan, @@ -527,7 +530,8 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev, 0, &indio_dev->dev, buffer, - &buffer->buffer_attr_list); + &buffer->buffer_attr_list, + true); if (ret) return ret; attrcount++; diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 59efb36db2c7..ec34d930920c 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -981,7 +981,8 @@ int __iio_device_attr_init(struct device_attribute *dev_attr, struct device_attribute *attr, const char *buf, size_t len), - enum iio_shared_by shared_by) + enum iio_shared_by shared_by, + bool extend_name) { int ret = 0; char *name = NULL; @@ -990,25 +991,28 @@ int __iio_device_attr_init(struct device_attribute *dev_attr, /* Build up postfix of <extend_name>_<modifier>_postfix */ if (chan->modified && (shared_by == IIO_SEPARATE)) { - if (chan->extend_name) + if (extend_name && chan->extend_name) { full_postfix = kasprintf(GFP_KERNEL, "%s_%s_%s", iio_modifier_names[chan ->channel2], chan->extend_name, postfix); - else + } else { full_postfix = kasprintf(GFP_KERNEL, "%s_%s", iio_modifier_names[chan ->channel2], postfix); + } } else { - if (chan->extend_name == NULL || shared_by != IIO_SEPARATE) + if (!extend_name || chan->extend_name == NULL + || shared_by != IIO_SEPARATE) { full_postfix = kstrdup(postfix, GFP_KERNEL); - else + } else { full_postfix = kasprintf(GFP_KERNEL, "%s_%s", chan->extend_name, postfix); + } } if (full_postfix == NULL) return -ENOMEM; @@ -1118,7 +1122,8 @@ int __iio_add_chan_devattr(const char *postfix, enum iio_shared_by shared_by, struct device *dev, struct iio_buffer *buffer, - struct list_head *attr_list) + struct list_head *attr_list, + bool extend_name) { int ret; struct iio_dev_attr *iio_attr, *t; @@ -1128,7 +1133,8 @@ int __iio_add_chan_devattr(const char *postfix, return -ENOMEM; ret = __iio_device_attr_init(&iio_attr->dev_attr, postfix, chan, - readfunc, writefunc, shared_by); + readfunc, writefunc, + shared_by, extend_name); if (ret) goto error_iio_dev_attr_free; iio_attr->c = chan; @@ -1171,7 +1177,8 @@ static int iio_device_add_channel_label(struct iio_dev *indio_dev, IIO_SEPARATE, &indio_dev->dev, NULL, - &iio_dev_opaque->channel_attr_list); + &iio_dev_opaque->channel_attr_list, + true); if (ret < 0) return ret; @@ -1197,7 +1204,8 @@ static int iio_device_add_info_mask_type(struct iio_dev *indio_dev, shared_by, &indio_dev->dev, NULL, - &iio_dev_opaque->channel_attr_list); + &iio_dev_opaque->channel_attr_list, + true); if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE)) continue; else if (ret < 0) @@ -1234,7 +1242,8 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev, shared_by, &indio_dev->dev, NULL, - &iio_dev_opaque->channel_attr_list); + &iio_dev_opaque->channel_attr_list, + true); kfree(avail_postfix); if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE)) continue; @@ -1331,7 +1340,8 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev, ext_info->shared, &indio_dev->dev, NULL, - &iio_dev_opaque->channel_attr_list); + &iio_dev_opaque->channel_attr_list, + true); i++; if (ret == -EBUSY && ext_info->shared) continue; diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c index d0732eac0f0a..e693d1374c9b 100644 --- a/drivers/iio/industrialio-event.c +++ b/drivers/iio/industrialio-event.c @@ -387,7 +387,8 @@ static int iio_device_add_event(struct iio_dev *indio_dev, ret = __iio_add_chan_devattr(postfix, chan, show, store, (i << 16) | spec_index, shared_by, &indio_dev->dev, NULL, - &iio_dev_opaque->event_interface->dev_attr_list); + &iio_dev_opaque->event_interface->dev_attr_list, + true); kfree(postfix); if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE)) -- 2.30.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename 2021-06-10 12:45 ` [PATCH 1/2] iio: core: Support removing extended name in attribute filename Paul Cercueil @ 2021-06-10 12:58 ` Andy Shevchenko 2021-06-10 13:02 ` Andy Shevchenko ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Andy Shevchenko @ 2021-06-10 12:58 UTC (permalink / raw) To: Paul Cercueil Cc: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, Linux Kernel Mailing List On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <paul@crapouillou.net> wrote: > > By default, when a channel has an extended name, it will appear in the > filename of channel attributes. E.g. if the extended name is "aux", the > filename of a "sample_rate" attribute will be something like: > in_voltage0_aux_sample_rate > > Add a mechanism to disable this feature. This will be used to add a > "extended_name" channel attribute. I'm afraid, NAK. Otherwise, please put an explanation that clearly shows that it will be no ABI breakage. I.o.w. users for the existing drivers and devices will always get those attributes at the same platform configuration(s). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename 2021-06-10 12:58 ` Andy Shevchenko @ 2021-06-10 13:02 ` Andy Shevchenko 2021-06-10 13:04 ` Jonathan Cameron 2021-06-10 13:05 ` Paul Cercueil 2 siblings, 0 replies; 13+ messages in thread From: Andy Shevchenko @ 2021-06-10 13:02 UTC (permalink / raw) To: Paul Cercueil Cc: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, Linux Kernel Mailing List On Thu, Jun 10, 2021 at 3:58 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <paul@crapouillou.net> wrote: > > > > By default, when a channel has an extended name, it will appear in the > > filename of channel attributes. E.g. if the extended name is "aux", the > > filename of a "sample_rate" attribute will be something like: > > in_voltage0_aux_sample_rate > > > > Add a mechanism to disable this feature. This will be used to add a > > "extended_name" channel attribute. > > I'm afraid, NAK. Otherwise, please put an explanation that clearly > shows that it will be no ABI breakage. > I.o.w. users for the existing drivers and devices will always get > those attributes at the same platform configuration(s). Reading two patches doesn't show to me any breakage so far, am I right? Then amend the commit message, please. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename 2021-06-10 12:58 ` Andy Shevchenko 2021-06-10 13:02 ` Andy Shevchenko @ 2021-06-10 13:04 ` Jonathan Cameron 2021-06-10 13:07 ` Paul Cercueil 2021-06-10 13:05 ` Paul Cercueil 2 siblings, 1 reply; 13+ messages in thread From: Jonathan Cameron @ 2021-06-10 13:04 UTC (permalink / raw) To: Andy Shevchenko Cc: Paul Cercueil, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, Linux Kernel Mailing List On Thu, 10 Jun 2021 15:58:51 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <paul@crapouillou.net> wrote: > > > > By default, when a channel has an extended name, it will appear in the > > filename of channel attributes. E.g. if the extended name is "aux", the > > filename of a "sample_rate" attribute will be something like: > > in_voltage0_aux_sample_rate > > > > Add a mechanism to disable this feature. This will be used to add a > > "extended_name" channel attribute. > > I'm afraid, NAK. Otherwise, please put an explanation that clearly > shows that it will be no ABI breakage. > I.o.w. users for the existing drivers and devices will always get > those attributes at the same platform configuration(s). > What Andy said. This was a bad design decision a long time back, but we are stuck with it. We have the _label attribute today that is the preferred route forwards for new drivers but we can't touch the old ones however annoying it might be. Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename 2021-06-10 13:04 ` Jonathan Cameron @ 2021-06-10 13:07 ` Paul Cercueil 0 siblings, 0 replies; 13+ messages in thread From: Paul Cercueil @ 2021-06-10 13:07 UTC (permalink / raw) To: Jonathan Cameron Cc: Andy Shevchenko, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, Linux Kernel Mailing List Hi Jonathan, Le jeu., juin 10 2021 at 14:04:12 +0100, Jonathan Cameron <Jonathan.Cameron@Huawei.com> a écrit : > On Thu, 10 Jun 2021 15:58:51 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil >> <paul@crapouillou.net> wrote: >> > >> > By default, when a channel has an extended name, it will appear >> in the >> > filename of channel attributes. E.g. if the extended name is >> "aux", the >> > filename of a "sample_rate" attribute will be something like: >> > in_voltage0_aux_sample_rate >> > >> > Add a mechanism to disable this feature. This will be used to add >> a >> > "extended_name" channel attribute. >> >> I'm afraid, NAK. Otherwise, please put an explanation that clearly >> shows that it will be no ABI breakage. >> I.o.w. users for the existing drivers and devices will always get >> those attributes at the same platform configuration(s). >> > > What Andy said. This was a bad design decision a long time back, but > we are stuck with it. > > We have the _label attribute today that is the preferred route > forwards > for new drivers but we can't touch the old ones however annoying it > might > be. You're missing the point here. This patchset only adds a new channel attribute and doesn't change anything else. The "label" is good to have, but that doesn't help me in any way. The problem here is parseability. Cheers, -Paul ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename 2021-06-10 12:58 ` Andy Shevchenko 2021-06-10 13:02 ` Andy Shevchenko 2021-06-10 13:04 ` Jonathan Cameron @ 2021-06-10 13:05 ` Paul Cercueil 2021-06-10 13:10 ` Jonathan Cameron 2 siblings, 1 reply; 13+ messages in thread From: Paul Cercueil @ 2021-06-10 13:05 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, Linux Kernel Mailing List Hi Andy, Le jeu., juin 10 2021 at 15:58:51 +0300, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <paul@crapouillou.net> > wrote: >> >> By default, when a channel has an extended name, it will appear in >> the >> filename of channel attributes. E.g. if the extended name is "aux", >> the >> filename of a "sample_rate" attribute will be something like: >> in_voltage0_aux_sample_rate >> >> Add a mechanism to disable this feature. This will be used to add a >> "extended_name" channel attribute. > > I'm afraid, NAK. Otherwise, please put an explanation that clearly > shows that it will be no ABI breakage. > I.o.w. users for the existing drivers and devices will always get > those attributes at the same platform configuration(s). Well, the commit message says that I'm adding a mechanism to disable the feature. If it was actually doing anything else (like actually disabling it for any attribute) then I'd mention it in the commit message. I don't see how that possibly can be a ABI breakage. -Paul ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename 2021-06-10 13:05 ` Paul Cercueil @ 2021-06-10 13:10 ` Jonathan Cameron 2021-06-10 13:14 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Cameron @ 2021-06-10 13:10 UTC (permalink / raw) To: Paul Cercueil Cc: Andy Shevchenko, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, Linux Kernel Mailing List On Thu, 10 Jun 2021 14:05:53 +0100 Paul Cercueil <paul@crapouillou.net> wrote: > Hi Andy, > > Le jeu., juin 10 2021 at 15:58:51 +0300, Andy Shevchenko > <andy.shevchenko@gmail.com> a écrit : > > On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > >> > >> By default, when a channel has an extended name, it will appear in > >> the > >> filename of channel attributes. E.g. if the extended name is "aux", > >> the > >> filename of a "sample_rate" attribute will be something like: > >> in_voltage0_aux_sample_rate > >> > >> Add a mechanism to disable this feature. This will be used to add a > >> "extended_name" channel attribute. > > > > I'm afraid, NAK. Otherwise, please put an explanation that clearly > > shows that it will be no ABI breakage. > > I.o.w. users for the existing drivers and devices will always get > > those attributes at the same platform configuration(s). > > Well, the commit message says that I'm adding a mechanism to disable > the feature. If it was actually doing anything else (like actually > disabling it for any attribute) then I'd mention it in the commit > message. > > I don't see how that possibly can be a ABI breakage. Ah. Both Andy and I got taken in by 'removing' in the title. What you really mean is identifying the extended name. That's much more acceptable :) Jonathan > > -Paul > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename 2021-06-10 13:10 ` Jonathan Cameron @ 2021-06-10 13:14 ` Andy Shevchenko 0 siblings, 0 replies; 13+ messages in thread From: Andy Shevchenko @ 2021-06-10 13:14 UTC (permalink / raw) To: Jonathan Cameron Cc: Paul Cercueil, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, Linux Kernel Mailing List On Thu, Jun 10, 2021 at 4:11 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Thu, 10 Jun 2021 14:05:53 +0100 > Paul Cercueil <paul@crapouillou.net> wrote: > > Le jeu., juin 10 2021 at 15:58:51 +0300, Andy Shevchenko > > <andy.shevchenko@gmail.com> a écrit : > > > On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <paul@crapouillou.net> > > > wrote: ... > > > I'm afraid, NAK. Otherwise, please put an explanation that clearly > > > shows that it will be no ABI breakage. > > > I.o.w. users for the existing drivers and devices will always get > > > those attributes at the same platform configuration(s). > > > > Well, the commit message says that I'm adding a mechanism to disable > > the feature. If it was actually doing anything else (like actually > > disabling it for any attribute) then I'd mention it in the commit > > message. > > > > I don't see how that possibly can be a ABI breakage. > > Ah. Both Andy and I got taken in by 'removing' in the title. Yep. I have read the patch first and my impression was that we are going to disable *existing* attributes, reading the complete series suggests it's not true. That's why I recommend to massage the commit message to avoid this confusion. > What you really mean is identifying the extended name. That's > much more acceptable :) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] iio: core: Add "extended_name" attribute to all channels 2021-06-10 12:45 [PATCH 0/2] iio: Add "extended_name" attribute Paul Cercueil 2021-06-10 12:45 ` [PATCH 1/2] iio: core: Support removing extended name in attribute filename Paul Cercueil @ 2021-06-10 12:45 ` Paul Cercueil 2021-06-10 14:34 ` Jonathan Cameron 1 sibling, 1 reply; 13+ messages in thread From: Paul Cercueil @ 2021-06-10 12:45 UTC (permalink / raw) To: Jonathan Cameron Cc: Lars-Peter Clausen, Michael Hennerich, linux-iio, linux-kernel, Paul Cercueil The point of this new attribute is to make the IIO tree actually parsable. Before, given this attribute as a filename: in_voltage0_aux_sample_rate Userspace had no way to know if the attribute name was "aux_sample_rate" with no extended name, or "sample_rate" with "aux" as the extended name, or just "rate" with "aux_sample" as the extended name. This was somewhat possible to deduce when there was more than one attribute present for a given channel, e.g: in_voltage0_aux_sample_rate in_voltage0_aux_frequency There, it was possible to deduce that "aux" was the extended name. But even with more than one attribute, this wasn't very robust, as two attributes starting with the same prefix (e.g. "sample_rate" and "sample_size") would result in the first part of the prefix being interpreted as being part of the extended name. To address this issue, add an "extended_name" attribute to all channels that actually do have an extended name. For this attribute, the extended name is not present in the filename; so in this example, the file name would be "in_voltage0_extended_name", and reading it would return "aux". Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- drivers/iio/industrialio-core.c | 41 +++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index ec34d930920c..4cdf9f092d73 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -723,6 +723,16 @@ static ssize_t iio_read_channel_label(struct device *dev, return indio_dev->info->read_label(indio_dev, this_attr->c, buf); } +static ssize_t iio_read_channel_extended_name(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + const struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + const struct iio_chan_spec *chan = this_attr->c; + + return sprintf(buf, "%s\n", chan->extend_name); +} + static ssize_t iio_read_channel_info(struct device *dev, struct device_attribute *attr, char *buf) @@ -1185,6 +1195,32 @@ static int iio_device_add_channel_label(struct iio_dev *indio_dev, return 1; } +static int +iio_device_add_channel_extended_name(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan) +{ + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); + int ret; + + if (!chan->extend_name) + return 0; + + ret = __iio_add_chan_devattr("extended_name", + chan, + &iio_read_channel_extended_name, + NULL, + 0, + IIO_SEPARATE, + &indio_dev->dev, + NULL, + &iio_dev_opaque->channel_attr_list, + false); + if (ret < 0) + return ret; + + return 1; +} + static int iio_device_add_info_mask_type(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, enum iio_shared_by shared_by, @@ -1327,6 +1363,11 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev, return ret; attrcount += ret; + ret = iio_device_add_channel_extended_name(indio_dev, chan); + if (ret < 0) + return ret; + attrcount += ret; + if (chan->ext_info) { unsigned int i = 0; for (ext_info = chan->ext_info; ext_info->name; ext_info++) { -- 2.30.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: core: Add "extended_name" attribute to all channels 2021-06-10 12:45 ` [PATCH 2/2] iio: core: Add "extended_name" attribute to all channels Paul Cercueil @ 2021-06-10 14:34 ` Jonathan Cameron 2021-06-10 14:49 ` Paul Cercueil 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Cameron @ 2021-06-10 14:34 UTC (permalink / raw) To: Paul Cercueil Cc: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, linux-kernel On Thu, 10 Jun 2021 13:45:56 +0100 Paul Cercueil <paul@crapouillou.net> wrote: > The point of this new attribute is to make the IIO tree actually > parsable. > > Before, given this attribute as a filename: > in_voltage0_aux_sample_rate > > Userspace had no way to know if the attribute name was > "aux_sample_rate" with no extended name, or "sample_rate" with "aux" as > the extended name, or just "rate" with "aux_sample" as the extended > name. > > This was somewhat possible to deduce when there was more than one > attribute present for a given channel, e.g: > in_voltage0_aux_sample_rate > in_voltage0_aux_frequency > > There, it was possible to deduce that "aux" was the extended name. But > even with more than one attribute, this wasn't very robust, as two > attributes starting with the same prefix (e.g. "sample_rate" and > "sample_size") would result in the first part of the prefix being > interpreted as being part of the extended name. > > To address this issue, add an "extended_name" attribute to all channels > that actually do have an extended name. Change the patch title to make it clear that it only applies to those that have extended_name set. > For this attribute, the extended > name is not present in the filename; so in this example, the file name > would be "in_voltage0_extended_name", and reading it would return "aux". Ah. Now I see the slightly issue with my immediate thought that we should just put this in the label attribute (and not allow both extended_name and label to be provided). Hmm. It's a bit ugly but given it hopefully doesn't effect that many drivers I could probably live with it. However, needs a patch to Documentation/ABI/testing/sysfs-bus-iio and a clear statement that this is for backwards compatibility reasons. I don't want to see extended_name getting added to new drivers! Jonathan > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/iio/industrialio-core.c | 41 +++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index ec34d930920c..4cdf9f092d73 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -723,6 +723,16 @@ static ssize_t iio_read_channel_label(struct device *dev, > return indio_dev->info->read_label(indio_dev, this_attr->c, buf); > } > > +static ssize_t iio_read_channel_extended_name(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + const struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); > + const struct iio_chan_spec *chan = this_attr->c; > + > + return sprintf(buf, "%s\n", chan->extend_name); > +} > + > static ssize_t iio_read_channel_info(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -1185,6 +1195,32 @@ static int iio_device_add_channel_label(struct iio_dev *indio_dev, > return 1; > } > > +static int > +iio_device_add_channel_extended_name(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan) > +{ > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > + int ret; > + > + if (!chan->extend_name) > + return 0; > + > + ret = __iio_add_chan_devattr("extended_name", > + chan, > + &iio_read_channel_extended_name, > + NULL, > + 0, > + IIO_SEPARATE, > + &indio_dev->dev, > + NULL, > + &iio_dev_opaque->channel_attr_list, > + false); > + if (ret < 0) > + return ret; > + > + return 1; > +} > + > static int iio_device_add_info_mask_type(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > enum iio_shared_by shared_by, > @@ -1327,6 +1363,11 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev, > return ret; > attrcount += ret; > > + ret = iio_device_add_channel_extended_name(indio_dev, chan); > + if (ret < 0) > + return ret; > + attrcount += ret; > + > if (chan->ext_info) { > unsigned int i = 0; > for (ext_info = chan->ext_info; ext_info->name; ext_info++) { ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: core: Add "extended_name" attribute to all channels 2021-06-10 14:34 ` Jonathan Cameron @ 2021-06-10 14:49 ` Paul Cercueil 2021-06-11 17:50 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Paul Cercueil @ 2021-06-10 14:49 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, linux-kernel Le jeu., juin 10 2021 at 15:34:25 +0100, Jonathan Cameron <Jonathan.Cameron@Huawei.com> a écrit : > On Thu, 10 Jun 2021 13:45:56 +0100 > Paul Cercueil <paul@crapouillou.net> wrote: > >> The point of this new attribute is to make the IIO tree actually >> parsable. >> >> Before, given this attribute as a filename: >> in_voltage0_aux_sample_rate >> >> Userspace had no way to know if the attribute name was >> "aux_sample_rate" with no extended name, or "sample_rate" with >> "aux" as >> the extended name, or just "rate" with "aux_sample" as the extended >> name. >> >> This was somewhat possible to deduce when there was more than one >> attribute present for a given channel, e.g: >> in_voltage0_aux_sample_rate >> in_voltage0_aux_frequency >> >> There, it was possible to deduce that "aux" was the extended name. >> But >> even with more than one attribute, this wasn't very robust, as two >> attributes starting with the same prefix (e.g. "sample_rate" and >> "sample_size") would result in the first part of the prefix being >> interpreted as being part of the extended name. >> >> To address this issue, add an "extended_name" attribute to all >> channels >> that actually do have an extended name. > > Change the patch title to make it clear that it only applies to those > that have extended_name set. > >> For this attribute, the extended >> name is not present in the filename; so in this example, the file >> name >> would be "in_voltage0_extended_name", and reading it would return >> "aux". > > Ah. Now I see the slightly issue with my immediate thought that we > should > just put this in the label attribute (and not allow both extended_name > and label to be provided). Are there cases where extended_name and label are both used? If they are exclusive, then it would be fine to put it in the label attribute. Parsing would be a bit more awkward because of the extended name but still possible (e.g. libiio would read 'in_voltage0_foo_label' and check that it returns 'foo'). -Paul > Hmm. It's a bit ugly but given it hopefully doesn't effect that many > drivers > I could probably live with it. > > However, needs a patch to Documentation/ABI/testing/sysfs-bus-iio > and a clear statement that this is for backwards compatibility > reasons. > I don't want to see extended_name getting added to new drivers! > > Jonathan > >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> drivers/iio/industrialio-core.c | 41 >> +++++++++++++++++++++++++++++++++ >> 1 file changed, 41 insertions(+) >> >> diff --git a/drivers/iio/industrialio-core.c >> b/drivers/iio/industrialio-core.c >> index ec34d930920c..4cdf9f092d73 100644 >> --- a/drivers/iio/industrialio-core.c >> +++ b/drivers/iio/industrialio-core.c >> @@ -723,6 +723,16 @@ static ssize_t iio_read_channel_label(struct >> device *dev, >> return indio_dev->info->read_label(indio_dev, this_attr->c, buf); >> } >> >> +static ssize_t iio_read_channel_extended_name(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + const struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); >> + const struct iio_chan_spec *chan = this_attr->c; >> + >> + return sprintf(buf, "%s\n", chan->extend_name); >> +} >> + >> static ssize_t iio_read_channel_info(struct device *dev, >> struct device_attribute *attr, >> char *buf) >> @@ -1185,6 +1195,32 @@ static int >> iio_device_add_channel_label(struct iio_dev *indio_dev, >> return 1; >> } >> >> +static int >> +iio_device_add_channel_extended_name(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan) >> +{ >> + struct iio_dev_opaque *iio_dev_opaque = >> to_iio_dev_opaque(indio_dev); >> + int ret; >> + >> + if (!chan->extend_name) >> + return 0; >> + >> + ret = __iio_add_chan_devattr("extended_name", >> + chan, >> + &iio_read_channel_extended_name, >> + NULL, >> + 0, >> + IIO_SEPARATE, >> + &indio_dev->dev, >> + NULL, >> + &iio_dev_opaque->channel_attr_list, >> + false); >> + if (ret < 0) >> + return ret; >> + >> + return 1; >> +} >> + >> static int iio_device_add_info_mask_type(struct iio_dev *indio_dev, >> struct iio_chan_spec const *chan, >> enum iio_shared_by shared_by, >> @@ -1327,6 +1363,11 @@ static int >> iio_device_add_channel_sysfs(struct iio_dev *indio_dev, >> return ret; >> attrcount += ret; >> >> + ret = iio_device_add_channel_extended_name(indio_dev, chan); >> + if (ret < 0) >> + return ret; >> + attrcount += ret; >> + >> if (chan->ext_info) { >> unsigned int i = 0; >> for (ext_info = chan->ext_info; ext_info->name; ext_info++) { > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: core: Add "extended_name" attribute to all channels 2021-06-10 14:49 ` Paul Cercueil @ 2021-06-11 17:50 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2021-06-11 17:50 UTC (permalink / raw) To: Paul Cercueil Cc: Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, linux-iio, linux-kernel On Thu, 10 Jun 2021 15:49:58 +0100 Paul Cercueil <paul@crapouillou.net> wrote: > Le jeu., juin 10 2021 at 15:34:25 +0100, Jonathan Cameron > <Jonathan.Cameron@Huawei.com> a écrit : > > On Thu, 10 Jun 2021 13:45:56 +0100 > > Paul Cercueil <paul@crapouillou.net> wrote: > > > >> The point of this new attribute is to make the IIO tree actually > >> parsable. > >> > >> Before, given this attribute as a filename: > >> in_voltage0_aux_sample_rate > >> > >> Userspace had no way to know if the attribute name was > >> "aux_sample_rate" with no extended name, or "sample_rate" with > >> "aux" as > >> the extended name, or just "rate" with "aux_sample" as the extended > >> name. > >> > >> This was somewhat possible to deduce when there was more than one > >> attribute present for a given channel, e.g: > >> in_voltage0_aux_sample_rate > >> in_voltage0_aux_frequency > >> > >> There, it was possible to deduce that "aux" was the extended name. > >> But > >> even with more than one attribute, this wasn't very robust, as two > >> attributes starting with the same prefix (e.g. "sample_rate" and > >> "sample_size") would result in the first part of the prefix being > >> interpreted as being part of the extended name. > >> > >> To address this issue, add an "extended_name" attribute to all > >> channels > >> that actually do have an extended name. > > > > Change the patch title to make it clear that it only applies to those > > that have extended_name set. > > > >> For this attribute, the extended > >> name is not present in the filename; so in this example, the file > >> name > >> would be "in_voltage0_extended_name", and reading it would return > >> "aux". > > > > Ah. Now I see the slightly issue with my immediate thought that we > > should > > just put this in the label attribute (and not allow both extended_name > > and label to be provided). > > Are there cases where extended_name and label are both used? Not yet and if we add a check as part of your series we can stop it happening in future. Simple checking for the read_label callback should be enough. > > If they are exclusive, then it would be fine to put it in the label > attribute. Parsing would be a bit more awkward because of the extended > name but still possible (e.g. libiio would read 'in_voltage0_foo_label' > and check that it returns 'foo'). That would be great! Jonathan > > -Paul > > > Hmm. It's a bit ugly but given it hopefully doesn't effect that many > > drivers > > I could probably live with it. > > > > However, needs a patch to Documentation/ABI/testing/sysfs-bus-iio > > and a clear statement that this is for backwards compatibility > > reasons. > > I don't want to see extended_name getting added to new drivers! > > > > Jonathan > > > >> > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> > >> --- > >> drivers/iio/industrialio-core.c | 41 > >> +++++++++++++++++++++++++++++++++ > >> 1 file changed, 41 insertions(+) > >> > >> diff --git a/drivers/iio/industrialio-core.c > >> b/drivers/iio/industrialio-core.c > >> index ec34d930920c..4cdf9f092d73 100644 > >> --- a/drivers/iio/industrialio-core.c > >> +++ b/drivers/iio/industrialio-core.c > >> @@ -723,6 +723,16 @@ static ssize_t iio_read_channel_label(struct > >> device *dev, > >> return indio_dev->info->read_label(indio_dev, this_attr->c, buf); > >> } > >> > >> +static ssize_t iio_read_channel_extended_name(struct device *dev, > >> + struct device_attribute *attr, > >> + char *buf) > >> +{ > >> + const struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); > >> + const struct iio_chan_spec *chan = this_attr->c; > >> + > >> + return sprintf(buf, "%s\n", chan->extend_name); > >> +} > >> + > >> static ssize_t iio_read_channel_info(struct device *dev, > >> struct device_attribute *attr, > >> char *buf) > >> @@ -1185,6 +1195,32 @@ static int > >> iio_device_add_channel_label(struct iio_dev *indio_dev, > >> return 1; > >> } > >> > >> +static int > >> +iio_device_add_channel_extended_name(struct iio_dev *indio_dev, > >> + struct iio_chan_spec const *chan) > >> +{ > >> + struct iio_dev_opaque *iio_dev_opaque = > >> to_iio_dev_opaque(indio_dev); > >> + int ret; > >> + > >> + if (!chan->extend_name) > >> + return 0; > >> + > >> + ret = __iio_add_chan_devattr("extended_name", > >> + chan, > >> + &iio_read_channel_extended_name, > >> + NULL, > >> + 0, > >> + IIO_SEPARATE, > >> + &indio_dev->dev, > >> + NULL, > >> + &iio_dev_opaque->channel_attr_list, > >> + false); > >> + if (ret < 0) > >> + return ret; > >> + > >> + return 1; > >> +} > >> + > >> static int iio_device_add_info_mask_type(struct iio_dev *indio_dev, > >> struct iio_chan_spec const *chan, > >> enum iio_shared_by shared_by, > >> @@ -1327,6 +1363,11 @@ static int > >> iio_device_add_channel_sysfs(struct iio_dev *indio_dev, > >> return ret; > >> attrcount += ret; > >> > >> + ret = iio_device_add_channel_extended_name(indio_dev, chan); > >> + if (ret < 0) > >> + return ret; > >> + attrcount += ret; > >> + > >> if (chan->ext_info) { > >> unsigned int i = 0; > >> for (ext_info = chan->ext_info; ext_info->name; ext_info++) { > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-06-11 17:48 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-10 12:45 [PATCH 0/2] iio: Add "extended_name" attribute Paul Cercueil 2021-06-10 12:45 ` [PATCH 1/2] iio: core: Support removing extended name in attribute filename Paul Cercueil 2021-06-10 12:58 ` Andy Shevchenko 2021-06-10 13:02 ` Andy Shevchenko 2021-06-10 13:04 ` Jonathan Cameron 2021-06-10 13:07 ` Paul Cercueil 2021-06-10 13:05 ` Paul Cercueil 2021-06-10 13:10 ` Jonathan Cameron 2021-06-10 13:14 ` Andy Shevchenko 2021-06-10 12:45 ` [PATCH 2/2] iio: core: Add "extended_name" attribute to all channels Paul Cercueil 2021-06-10 14:34 ` Jonathan Cameron 2021-06-10 14:49 ` Paul Cercueil 2021-06-11 17:50 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).