Re: [PATCH v2 2/2] iio: core: Support reading extended name as label

From: Paul Cercueil
Date: Thu Jun 17 2021 - 05:08:16 EST

Hi Alexandru,

Le jeu., juin 17 2021 at 10:22:35 +0300, Alexandru Ardelean <ardeleanalex@xxxxxxxxx> a écrit :
On Wed, Jun 16, 2021 at 7:00 PM Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:

The point of this new change is to make the IIO tree actually parsable.

Before, given this attribute as a filename:

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

This was somewhat possible to deduce when there was more than one
attribute present for a given channel, e.g:

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 the issue, knowing that channels will never have both a label
and an extended name, set the channel's label to the extended name.
In this case, the label's attribute will also have the extended name in
its filename, but we can live with that - userspace can open
in_voltage0_<prefix>_label and verify that it returns <prefix> to obtain
the extended name.

The best way would have been for all drivers [using extend_name] to
implement their own read_label hook.

Let's agree to disagree :)

But this can work fine as well [as the other method would add some duplication].

Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
drivers/iio/industrialio-core.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 81f40dab778a..9b37e96538c2 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -717,8 +717,12 @@ static ssize_t iio_read_channel_label(struct device *dev,
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);

- if (!indio_dev->info->read_label)
- return -EINVAL;
+ if (!indio_dev->info->read_label) {
+ if (this_attr->c->extend_name)
+ return sprintf(buf, "%s\n", this_attr->c->extend_name);
+ else

nitpick: else statement has no effect

well, this block could be reworked a bit as:

if (indio_dev->info->read_label)
return indio_dev->info->read_label(indio_dev, this_attr->c, buf);

if (this_attr->c->extend_name)
return sprintf(buf, "%s\n", this_attr->c->extend_name);

return -EINVAL;

I generally prefer to make the diff as small as possible so that the changes are more obvious. But this does look better.


+ return -EINVAL;
+ }

return indio_dev->info->read_label(indio_dev, this_attr->c, buf);
@@ -1160,7 +1164,7 @@ static int iio_device_add_channel_label(struct iio_dev *indio_dev,
struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
int ret;

- if (!indio_dev->info->read_label)
+ if (!indio_dev->info->read_label && !chan->extend_name)
return 0;

ret = __iio_add_chan_devattr("label",