[PATCH 0/1] iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible

[PATCH 0/1] iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible

[Hans de Goede]

Hi All,

I wrote this because I was planning on using is_visible in a driver's
attribute group myself (1). But in the end it looks like I'm going to
do things differently.

This is still useful to have though, both for possible future use of
is_visible in driver's attribute groups as well as to make the current
usage of is_visible in adi-axi-adc.c actually work.

Regards,

Hans

1) I was planning to add an (optional) in_accel_location attribute which
would contain "lid" or "base" on 360 degree hinges (yoga) style 2-in-1s
with 2 accelerometers and where the kernel knows the location. But given
the recent discussion to use label-s for this for proximity sensors,
I believe that using label-s here makes more sense too. I will write
a patch-set for this when I can find / make some time for this.

[PATCH] iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible

[Hans de Goede]

The iio-core extends the attr_group provided by the driver with its
own attributes. To be able to do this it:

1. Has its own (non const) io_dev_opaque.chan_attr_group attr_group struct
2. It allocates a new attrs array with room for both the drivers and its
   own attributes
3. It copies over the driver provided attributes into the newly allocated
   attrs array.

But the drivers attr_group may contain more then just the attrs array, it
may also contain an is_visible callback and at least the adi-axi-adc.c
is currently defining such a callback.

Change the attr_group copying code to also copy over the is_visible
callback, so that drivers can define one and have it workins as is
normal for attr_group-s all over the kernel.

Note that the is_visible callback takes an index into the array as
argument, so that indices of the driver's attributes must not change,
this is not a problem as the driver's own attributes are added first
to the newly allocated attrs array and the attributes handled by the
core are appended after the driver's attributes.

Cc: Michael Hennerich <michael.hennerich@analog.com>
Cc: Alexandru Ardelean <alexandru.ardelean@analog.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/industrialio-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 261d3b17edc9..7943d0545b61 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1466,11 +1466,14 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 		goto error_clear_attrs;
 	}
 	/* Copy across original attributes */
-	if (indio_dev->info->attrs)
+	if (indio_dev->info->attrs) {
 		memcpy(iio_dev_opaque->chan_attr_group.attrs,
 		       indio_dev->info->attrs->attrs,
 		       sizeof(iio_dev_opaque->chan_attr_group.attrs[0])
 		       *attrcount_orig);
+		iio_dev_opaque->chan_attr_group.is_visible =
+			indio_dev->info->attrs->is_visible;
+	}
 	attrn = attrcount_orig;
 	/* Add all elements from the list. */
 	list_for_each_entry(p, &iio_dev_opaque->channel_attr_list, l)
-- 
2.28.0

Re: [PATCH] iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible

[Alexandru Ardelean]

On Wed, Nov 25, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The iio-core extends the attr_group provided by the driver with its
> own attributes. To be able to do this it:
>
> 1. Has its own (non const) io_dev_opaque.chan_attr_group attr_group struct
> 2. It allocates a new attrs array with room for both the drivers and its
>    own attributes
> 3. It copies over the driver provided attributes into the newly allocated
>    attrs array.
>
> But the drivers attr_group may contain more then just the attrs array, it
> may also contain an is_visible callback and at least the adi-axi-adc.c
> is currently defining such a callback.
>
> Change the attr_group copying code to also copy over the is_visible
> callback, so that drivers can define one and have it workins as is
> normal for attr_group-s all over the kernel.
>
> Note that the is_visible callback takes an index into the array as
> argument, so that indices of the driver's attributes must not change,
> this is not a problem as the driver's own attributes are added first
> to the newly allocated attrs array and the attributes handled by the
> core are appended after the driver's attributes.
>

Sorry for missing this earlier.
I'm terrible with tracking emails sometimes.

> Cc: Michael Hennerich <michael.hennerich@analog.com>
> Cc: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/iio/industrialio-core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 261d3b17edc9..7943d0545b61 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1466,11 +1466,14 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>                 goto error_clear_attrs;
>         }
>         /* Copy across original attributes */
> -       if (indio_dev->info->attrs)
> +       if (indio_dev->info->attrs) {
>                 memcpy(iio_dev_opaque->chan_attr_group.attrs,
>                        indio_dev->info->attrs->attrs,
>                        sizeof(iio_dev_opaque->chan_attr_group.attrs[0])
>                        *attrcount_orig);
> +               iio_dev_opaque->chan_attr_group.is_visible =
> +                       indio_dev->info->attrs->is_visible;

So, I think I was pretty silly at the time, and did not fully know how
IIO handles attributes.
But I can see the bug now [in adi-axi-adc].
Thanks for identifying it :)

As an initial handling, this looks good.

I think this also means that we should check and see where else we may
need to do this.
Maybe, we should do a utility that handles this atribute_group copying.

But for this one as-is:

Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> +       }
>         attrn = attrcount_orig;
>         /* Add all elements from the list. */
>         list_for_each_entry(p, &iio_dev_opaque->channel_attr_list, l)
> --
> 2.28.0
>

Re: [PATCH 0/1] iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible

[Hans de Goede]

Hi,

On 11/25/20 9:46 AM, Hans de Goede wrote:
> Hi All,
> 
> I wrote this because I was planning on using is_visible in a driver's
> attribute group myself (1). But in the end it looks like I'm going to
> do things differently.
> 
> This is still useful to have though, both for possible future use of
> is_visible in driver's attribute groups as well as to make the current
> usage of is_visible in adi-axi-adc.c actually work.

Jonathan, any opinion / remarks on this one?

FWIW since I no longer have plans to use is_visible in an iio-driver
myself I'm fine with dropping this one, but:

1. Being able to use is_visible in the attr group of iio-drivers
seems like a nice thing to have.

2. There is an existing use of is_visible in adi-axi-adc.c which currently is broken.

Regards,

Hans




> 1) I was planning to add an (optional) in_accel_location attribute which
> would contain "lid" or "base" on 360 degree hinges (yoga) style 2-in-1s
> with 2 accelerometers and where the kernel knows the location. But given
> the recent discussion to use label-s for this for proximity sensors,
> I believe that using label-s here makes more sense too. I will write
> a patch-set for this when I can find / make some time for this.
> 

Re: [PATCH 0/1] iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible

[Jonathan Cameron]

On Tue, 1 Dec 2020 13:28:47 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 11/25/20 9:46 AM, Hans de Goede wrote:
> > Hi All,
> > 
> > I wrote this because I was planning on using is_visible in a driver's
> > attribute group myself (1). But in the end it looks like I'm going to
> > do things differently.
> > 
> > This is still useful to have though, both for possible future use of
> > is_visible in driver's attribute groups as well as to make the current
> > usage of is_visible in adi-axi-adc.c actually work.  
> 
> Jonathan, any opinion / remarks on this one?
> 
> FWIW since I no longer have plans to use is_visible in an iio-driver
> myself I'm fine with dropping this one, but:
> 
> 1. Being able to use is_visible in the attr group of iio-drivers
> seems like a nice thing to have.
> 
> 2. There is an existing use of is_visible in adi-axi-adc.c which currently is broken.

I was giving time for Alex, or others to sanity check the need for a fix
(well more specifically wether this one was the right one as clearly
a need!).

Anyhow, all sounds good.  Giving timing I'll mark it as one I'll pick up
to go in after rc1 + stable. 

I'm not totally sure on the fixes tag.  Current patch will have to go on top
of 207c2d27a010 ("iio: core: move channel list & group to private iio device object")
but I don't think it worked before that either as we were still copying attributes
around, just to a different location.

Jonathan



> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > 1) I was planning to add an (optional) in_accel_location attribute which
> > would contain "lid" or "base" on 360 degree hinges (yoga) style 2-in-1s
> > with 2 accelerometers and where the kernel knows the location. But given
> > the recent discussion to use label-s for this for proximity sensors,
> > I believe that using label-s here makes more sense too. I will write
> > a patch-set for this when I can find / make some time for this.
> >   
> 

Re: [PATCH 0/1] iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible

[Hans de Goede]

Hi,

n 12/5/20 4:23 PM, Jonathan Cameron wrote:
> On Tue, 1 Dec 2020 13:28:47 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 11/25/20 9:46 AM, Hans de Goede wrote:
>>> Hi All,
>>>
>>> I wrote this because I was planning on using is_visible in a driver's
>>> attribute group myself (1). But in the end it looks like I'm going to
>>> do things differently.
>>>
>>> This is still useful to have though, both for possible future use of
>>> is_visible in driver's attribute groups as well as to make the current
>>> usage of is_visible in adi-axi-adc.c actually work.  
>>
>> Jonathan, any opinion / remarks on this one?
>>
>> FWIW since I no longer have plans to use is_visible in an iio-driver
>> myself I'm fine with dropping this one, but:
>>
>> 1. Being able to use is_visible in the attr group of iio-drivers
>> seems like a nice thing to have.
>>
>> 2. There is an existing use of is_visible in adi-axi-adc.c which currently is broken.
> 
> I was giving time for Alex, or others to sanity check the need for a fix
> (well more specifically wether this one was the right one as clearly
> a need!).
> 
> Anyhow, all sounds good.  Giving timing I'll mark it as one I'll pick up
> to go in after rc1 + stable. 
> 
> I'm not totally sure on the fixes tag.  Current patch will have to go on top
> of 207c2d27a010 ("iio: core: move channel list & group to private iio device object")
> but I don't think it worked before that either as we were still copying attributes
> around, just to a different location.

I don't think it is important for this one to go to the stables series,
so if you think it may cause problems feel free to drop the Fixes tag
(or point it to a different commit).

Regards,

Hans

Re: [PATCH 0/1] iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible

[Alexandru Ardelean]

On Sat, Dec 5, 2020 at 5:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> n 12/5/20 4:23 PM, Jonathan Cameron wrote:
> > On Tue, 1 Dec 2020 13:28:47 +0100
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >
> >> Hi,
> >>
> >> On 11/25/20 9:46 AM, Hans de Goede wrote:
> >>> Hi All,
> >>>
> >>> I wrote this because I was planning on using is_visible in a driver's
> >>> attribute group myself (1). But in the end it looks like I'm going to
> >>> do things differently.
> >>>
> >>> This is still useful to have though, both for possible future use of
> >>> is_visible in driver's attribute groups as well as to make the current
> >>> usage of is_visible in adi-axi-adc.c actually work.
> >>
> >> Jonathan, any opinion / remarks on this one?
> >>
> >> FWIW since I no longer have plans to use is_visible in an iio-driver
> >> myself I'm fine with dropping this one, but:
> >>
> >> 1. Being able to use is_visible in the attr group of iio-drivers
> >> seems like a nice thing to have.
> >>
> >> 2. There is an existing use of is_visible in adi-axi-adc.c which currently is broken.
> >
> > I was giving time for Alex, or others to sanity check the need for a fix
> > (well more specifically wether this one was the right one as clearly
> > a need!).
> >
> > Anyhow, all sounds good.  Giving timing I'll mark it as one I'll pick up
> > to go in after rc1 + stable.
> >
> > I'm not totally sure on the fixes tag.  Current patch will have to go on top
> > of 207c2d27a010 ("iio: core: move channel list & group to private iio device object")
> > but I don't think it worked before that either as we were still copying attributes
> > around, just to a different location.
>
> I don't think it is important for this one to go to the stables series,
> so if you think it may cause problems feel free to drop the Fixes tag
> (or point it to a different commit).

I am also fine to not port this into the stable series.
The AXI ADC driver that is usually used, is from the Analog Devices Linux tree.
The current upstream version will be the one that reworks/cleans-up
the old ones in the ADI tree.
For some reason, there are like 4 AXI ADC variants in the ADI tree :p
All these 4 should be unified into this one at some point.

>
> Regards,
>
> Hans
>

Re: [PATCH 0/1] iio: core: Copy iio_info.attrs->is_visible into iio_dev_opaque.chan_attr_group.is_visible

[Jonathan Cameron]

On Sat, 5 Dec 2020 17:50:02 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, Dec 5, 2020 at 5:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > n 12/5/20 4:23 PM, Jonathan Cameron wrote:  
> > > On Tue, 1 Dec 2020 13:28:47 +0100
> > > Hans de Goede <hdegoede@redhat.com> wrote:
> > >  
> > >> Hi,
> > >>
> > >> On 11/25/20 9:46 AM, Hans de Goede wrote:  
> > >>> Hi All,
> > >>>
> > >>> I wrote this because I was planning on using is_visible in a driver's
> > >>> attribute group myself (1). But in the end it looks like I'm going to
> > >>> do things differently.
> > >>>
> > >>> This is still useful to have though, both for possible future use of
> > >>> is_visible in driver's attribute groups as well as to make the current
> > >>> usage of is_visible in adi-axi-adc.c actually work.  
> > >>
> > >> Jonathan, any opinion / remarks on this one?
> > >>
> > >> FWIW since I no longer have plans to use is_visible in an iio-driver
> > >> myself I'm fine with dropping this one, but:
> > >>
> > >> 1. Being able to use is_visible in the attr group of iio-drivers
> > >> seems like a nice thing to have.
> > >>
> > >> 2. There is an existing use of is_visible in adi-axi-adc.c which currently is broken.  
> > >
> > > I was giving time for Alex, or others to sanity check the need for a fix
> > > (well more specifically wether this one was the right one as clearly
> > > a need!).
> > >
> > > Anyhow, all sounds good.  Giving timing I'll mark it as one I'll pick up
> > > to go in after rc1 + stable.
> > >
> > > I'm not totally sure on the fixes tag.  Current patch will have to go on top
> > > of 207c2d27a010 ("iio: core: move channel list & group to private iio device object")
> > > but I don't think it worked before that either as we were still copying attributes
> > > around, just to a different location.  
> >
> > I don't think it is important for this one to go to the stables series,
> > so if you think it may cause problems feel free to drop the Fixes tag
> > (or point it to a different commit).  
> 
> I am also fine to not port this into the stable series.
> The AXI ADC driver that is usually used, is from the Analog Devices Linux tree.
> The current upstream version will be the one that reworks/cleans-up
> the old ones in the ADI tree.
> For some reason, there are like 4 AXI ADC variants in the ADI tree :p
> All these 4 should be unified into this one at some point.
Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> 
> >
> > Regards,
> >
> > Hans
> >