2021-06-10 12:48:07

by Paul Cercueil

[permalink] [raw]
Subject: [PATCH 1/2] iio: core: Support removing extended name in attribute filename

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 <[email protected]>
---
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


2021-06-10 13:01:52

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename

On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <[email protected]> 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

2021-06-10 13:06:49

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename

On Thu, Jun 10, 2021 at 3:58 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <[email protected]> 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

2021-06-10 13:07:31

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename

On Thu, 10 Jun 2021 15:58:51 +0300
Andy Shevchenko <[email protected]> wrote:

> On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <[email protected]> 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

2021-06-10 13:08:05

by Paul Cercueil

[permalink] [raw]
Subject: Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename

Hi Andy,

Le jeu., juin 10 2021 at 15:58:51 +0300, Andy Shevchenko
<[email protected]> a ?crit :
> On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <[email protected]>
> 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


2021-06-10 13:11:39

by Paul Cercueil

[permalink] [raw]
Subject: Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename

Hi Jonathan,

Le jeu., juin 10 2021 at 14:04:12 +0100, Jonathan Cameron
<[email protected]> a ?crit :
> On Thu, 10 Jun 2021 15:58:51 +0300
> Andy Shevchenko <[email protected]> wrote:
>
>> On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil
>> <[email protected]> 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


2021-06-10 13:12:33

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename

On Thu, 10 Jun 2021 14:05:53 +0100
Paul Cercueil <[email protected]> wrote:

> Hi Andy,
>
> Le jeu., juin 10 2021 at 15:58:51 +0300, Andy Shevchenko
> <[email protected]> a ?crit :
> > On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <[email protected]>
> > 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
>
>

2021-06-10 13:17:59

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 1/2] iio: core: Support removing extended name in attribute filename

On Thu, Jun 10, 2021 at 4:11 PM Jonathan Cameron
<[email protected]> wrote:
> On Thu, 10 Jun 2021 14:05:53 +0100
> Paul Cercueil <[email protected]> wrote:
> > Le jeu., juin 10 2021 at 15:58:51 +0300, Andy Shevchenko
> > <[email protected]> a écrit :
> > > On Thu, Jun 10, 2021 at 3:47 PM Paul Cercueil <[email protected]>
> > > 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