[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <ED9UUQ.EDOA8S56T8BP@crapouillou.net>
Date: Thu, 17 Jun 2021 10:08:02 +0100
From: Paul Cercueil <paul@...pouillou.net>
To: Alexandru Ardelean <ardeleanalex@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
linux-iio <linux-iio@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: core: Support reading extended name as label
Hi Alexandru,
Le jeu., juin 17 2021 at 10:22:35 +0300, Alexandru Ardelean
<ardeleanalex@...il.com> a écrit :
> On Wed, Jun 16, 2021 at 7:00 PM Paul Cercueil <paul@...pouillou.net>
> wrote:
>>
>> The point of this new change 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 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@...pouillou.net>
>> ---
>> 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.
-Paul
>
>> + 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",
>> --
>> 2.30.2
>>
Powered by blists - more mailing lists