[PATCH 0/3] iio: cleanup masklength usage

[PATCH 0/3] iio: cleanup masklength usage

[David Lechner]

While working on other patches I noticed that a few drivers are setting
the masklength field of struct iio_dev even though it is marked as
[INTERN]. It looks like maybe this was not always the case, but we can
safely clean it up now without breaking anything.

---
David Lechner (3):
      iio: adc: ad7266: don't set masklength
      iio: adc: mxs-lradc-adc: don't set masklength
      iio: buffer: initialize masklength accumulator to 0

 drivers/iio/adc/ad7266.c          | 1 -
 drivers/iio/adc/mxs-lradc-adc.c   | 1 -
 drivers/iio/industrialio-buffer.c | 2 +-
 3 files changed, 1 insertion(+), 3 deletions(-)
---
base-commit: b80ad8e3cd2712b78b98804d1f59199680d8ed91
change-id: 20240425-b4-iio-masklength-cleanup-86b632b19901

[PATCH 1/3] iio: adc: ad7266: don't set masklength

[David Lechner]

The masklength field is marked as [INTERN] and should not be set by
drivers, so remove the assignment in the ad7266 driver.

__iio_device_register() will populate this field with the correct value.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7266.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index 468c2656d2be..353a97f9c086 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -371,7 +371,6 @@ static void ad7266_init_channels(struct iio_dev *indio_dev)
 	indio_dev->channels = chan_info->channels;
 	indio_dev->num_channels = chan_info->num_channels;
 	indio_dev->available_scan_masks = chan_info->scan_masks;
-	indio_dev->masklength = chan_info->num_channels - 1;
 }
 
 static const char * const ad7266_gpio_labels[] = {

-- 
2.43.2

[PATCH 2/3] iio: adc: mxs-lradc-adc: don't set masklength

[David Lechner]

The masklength field is marked as [INTERN] and should not be set by
drivers, so remove the assignment in the mxs-lradc-adc driver.

__iio_device_register() will populate this field with the correct value.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/mxs-lradc-adc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
index 2e60c10ee4ff..8c7b64e78dbb 100644
--- a/drivers/iio/adc/mxs-lradc-adc.c
+++ b/drivers/iio/adc/mxs-lradc-adc.c
@@ -724,7 +724,6 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
 	iio->dev.of_node = dev->parent->of_node;
 	iio->info = &mxs_lradc_adc_iio_info;
 	iio->modes = INDIO_DIRECT_MODE;
-	iio->masklength = LRADC_MAX_TOTAL_CHANS;
 
 	if (lradc->soc == IMX23_LRADC) {
 		iio->channels = mx23_lradc_chan_spec;

-- 
2.43.2

[PATCH 3/3] iio: buffer: initialize masklength accumulator to 0

[David Lechner]

Since masklength is marked as [INTERN], no drivers should assign it and
the value will always be 0. Therefore, the local ml accumulator variable
in iio_buffers_alloc_sysfs_and_mask() will always start out as 0.

This changes the code to explicitly set ml to 0 to make it clear that
drivers should not be trying to override the masklength field.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/industrialio-buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 1d950a3e153b..cec58a604d73 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1744,7 +1744,7 @@ int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 
 	channels = indio_dev->channels;
 	if (channels) {
-		int ml = indio_dev->masklength;
+		int ml = 0;
 
 		for (i = 0; i < indio_dev->num_channels; i++)
 			ml = max(ml, channels[i].scan_index + 1);

-- 
2.43.2

Re: [PATCH 0/3] iio: cleanup masklength usage

[Nuno Sá]

On Thu, 2024-04-25 at 10:03 -0500, David Lechner wrote:
> While working on other patches I noticed that a few drivers are setting
> the masklength field of struct iio_dev even though it is marked as
> [INTERN]. It looks like maybe this was not always the case, but we can
> safely clean it up now without breaking anything.
> 
> ---
> David Lechner (3):
>       iio: adc: ad7266: don't set masklength
>       iio: adc: mxs-lradc-adc: don't set masklength
>       iio: buffer: initialize masklength accumulator to 0
> 
>  drivers/iio/adc/ad7266.c          | 1 -
>  drivers/iio/adc/mxs-lradc-adc.c   | 1 -
>  drivers/iio/industrialio-buffer.c | 2 +-
>  3 files changed, 1 insertion(+), 3 deletions(-)
> ---
> base-commit: b80ad8e3cd2712b78b98804d1f59199680d8ed91
> change-id: 20240425-b4-iio-masklength-cleanup-86b632b19901
> 

Hi David,

Nice cleanup. The patches look good to me but there's one thing missing :). As you
correctly noted, the field should be internal to the IIO core and drivers should not
touch it. Hence, you need to make sure not driver is using it so we can move it into
struct iio_dev_opaque [1]. That's the place all the intern fields should, eventually,
end up.

Now, quite some drivers in the trigger handler will read the masklength for looping
with for_each_set_bit(). Hence, the straight thing would be an helper to get it.
Maybe there's a clever way...

I know this is more work than what you had in mind but I think it should be fairly
simple (hopefully) and since you started it :), maybe we can get the whole thing done
and remove another [INTERN] member from the iio_dev struct.

[1]: https://elixir.bootlin.com/linux/latest/source/include/linux/iio/iio-opaque.h#L42

- Nuno Sá

Re: [PATCH 0/3] iio: cleanup masklength usage

[David Lechner]

On Fri, Apr 26, 2024 at 2:13 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Thu, 2024-04-25 at 10:03 -0500, David Lechner wrote:
> > While working on other patches I noticed that a few drivers are setting
> > the masklength field of struct iio_dev even though it is marked as
> > [INTERN]. It looks like maybe this was not always the case, but we can
> > safely clean it up now without breaking anything.
> >
> > ---
> > David Lechner (3):
> >       iio: adc: ad7266: don't set masklength
> >       iio: adc: mxs-lradc-adc: don't set masklength
> >       iio: buffer: initialize masklength accumulator to 0
> >
> >  drivers/iio/adc/ad7266.c          | 1 -
> >  drivers/iio/adc/mxs-lradc-adc.c   | 1 -
> >  drivers/iio/industrialio-buffer.c | 2 +-
> >  3 files changed, 1 insertion(+), 3 deletions(-)
> > ---
> > base-commit: b80ad8e3cd2712b78b98804d1f59199680d8ed91
> > change-id: 20240425-b4-iio-masklength-cleanup-86b632b19901
> >
>
> Hi David,
>
> Nice cleanup. The patches look good to me but there's one thing missing :). As you
> correctly noted, the field should be internal to the IIO core and drivers should not
> touch it. Hence, you need to make sure not driver is using it so we can move it into
> struct iio_dev_opaque [1]. That's the place all the intern fields should, eventually,
> end up.
>
> Now, quite some drivers in the trigger handler will read the masklength for looping
> with for_each_set_bit(). Hence, the straight thing would be an helper to get it.
> Maybe there's a clever way...
>
> I know this is more work than what you had in mind but I think it should be fairly
> simple (hopefully) and since you started it :), maybe we can get the whole thing done
> and remove another [INTERN] member from the iio_dev struct.
>
> [1]: https://elixir.bootlin.com/linux/latest/source/include/linux/iio/iio-opaque.h#L42
>
> - Nuno Sá

Sounds like fun. :-p

I will look into it.

Re: [PATCH 0/3] iio: cleanup masklength usage

[Jonathan Cameron]

On Fri, 26 Apr 2024 10:26:31 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Fri, Apr 26, 2024 at 2:13 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > On Thu, 2024-04-25 at 10:03 -0500, David Lechner wrote:  
> > > While working on other patches I noticed that a few drivers are setting
> > > the masklength field of struct iio_dev even though it is marked as
> > > [INTERN]. It looks like maybe this was not always the case, but we can
> > > safely clean it up now without breaking anything.
> > >
> > > ---
> > > David Lechner (3):
> > >       iio: adc: ad7266: don't set masklength
> > >       iio: adc: mxs-lradc-adc: don't set masklength
> > >       iio: buffer: initialize masklength accumulator to 0
> > >
> > >  drivers/iio/adc/ad7266.c          | 1 -
> > >  drivers/iio/adc/mxs-lradc-adc.c   | 1 -
> > >  drivers/iio/industrialio-buffer.c | 2 +-
> > >  3 files changed, 1 insertion(+), 3 deletions(-)
> > > ---
> > > base-commit: b80ad8e3cd2712b78b98804d1f59199680d8ed91
> > > change-id: 20240425-b4-iio-masklength-cleanup-86b632b19901
> > >  
> >
> > Hi David,
> >
> > Nice cleanup. The patches look good to me but there's one thing missing :). As you
> > correctly noted, the field should be internal to the IIO core and drivers should not
> > touch it. Hence, you need to make sure not driver is using it so we can move it into
> > struct iio_dev_opaque [1]. That's the place all the intern fields should, eventually,
> > end up.
> >
> > Now, quite some drivers in the trigger handler will read the masklength for looping
> > with for_each_set_bit(). Hence, the straight thing would be an helper to get it.
> > Maybe there's a clever way...
> >
> > I know this is more work than what you had in mind but I think it should be fairly
> > simple (hopefully) and since you started it :), maybe we can get the whole thing done
> > and remove another [INTERN] member from the iio_dev struct.
> >
> > [1]: https://elixir.bootlin.com/linux/latest/source/include/linux/iio/iio-opaque.h#L42
> >
> > - Nuno Sá  
> 
> Sounds like fun. :-p
> 
> I will look into it.

I think this one might be miss marked as [INTERN]. It should be constant from the driver
point of view, but given active_scan_masks is meant to be visible to the driver,
it's length should probably be as well.

Sure every driver should be able to trivially work this out for themselves, but
do we care about stopping them using this?

It might be worth some nice iterator wrappers with names like
iio_for_each_active_channel() though I'd expect those to still be accessing these
fields directly as this is a high performance path so we don't want to to bounce
though a core function to get them.

Jonathan

Re: [PATCH 0/3] iio: cleanup masklength usage

[Jonathan Cameron]

On Thu, 25 Apr 2024 10:03:26 -0500
David Lechner <dlechner@baylibre.com> wrote:

> While working on other patches I noticed that a few drivers are setting
> the masklength field of struct iio_dev even though it is marked as
> [INTERN]. It looks like maybe this was not always the case, but we can
> safely clean it up now without breaking anything.
> 
> ---
> David Lechner (3):
>       iio: adc: ad7266: don't set masklength
>       iio: adc: mxs-lradc-adc: don't set masklength
>       iio: buffer: initialize masklength accumulator to 0
> 
>  drivers/iio/adc/ad7266.c          | 1 -
>  drivers/iio/adc/mxs-lradc-adc.c   | 1 -
>  drivers/iio/industrialio-buffer.c | 2 +-
>  3 files changed, 1 insertion(+), 3 deletions(-)
Applied to the togreg branch of iio.git and pushed out as testing for 0-day
to poke at it.

I can't remember that ever being set by drivers so this is rather weird.
Mind you our docs used to be less clear on what drivers should set so
maybe that was the issue.

Jonathan

> ---
> base-commit: b80ad8e3cd2712b78b98804d1f59199680d8ed91
> change-id: 20240425-b4-iio-masklength-cleanup-86b632b19901
> 

Re: [PATCH 0/3] iio: cleanup masklength usage

[Nuno Sá]

On Sun, 2024-04-28 at 14:23 +0100, Jonathan Cameron wrote:
> On Fri, 26 Apr 2024 10:26:31 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On Fri, Apr 26, 2024 at 2:13 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > 
> > > On Thu, 2024-04-25 at 10:03 -0500, David Lechner wrote:  
> > > > While working on other patches I noticed that a few drivers are setting
> > > > the masklength field of struct iio_dev even though it is marked as
> > > > [INTERN]. It looks like maybe this was not always the case, but we can
> > > > safely clean it up now without breaking anything.
> > > > 
> > > > ---
> > > > David Lechner (3):
> > > >       iio: adc: ad7266: don't set masklength
> > > >       iio: adc: mxs-lradc-adc: don't set masklength
> > > >       iio: buffer: initialize masklength accumulator to 0
> > > > 
> > > >  drivers/iio/adc/ad7266.c          | 1 -
> > > >  drivers/iio/adc/mxs-lradc-adc.c   | 1 -
> > > >  drivers/iio/industrialio-buffer.c | 2 +-
> > > >  3 files changed, 1 insertion(+), 3 deletions(-)
> > > > ---
> > > > base-commit: b80ad8e3cd2712b78b98804d1f59199680d8ed91
> > > > change-id: 20240425-b4-iio-masklength-cleanup-86b632b19901
> > > >  
> > > 
> > > Hi David,
> > > 
> > > Nice cleanup. The patches look good to me but there's one thing missing
> > > :). As you
> > > correctly noted, the field should be internal to the IIO core and drivers
> > > should not
> > > touch it. Hence, you need to make sure not driver is using it so we can
> > > move it into
> > > struct iio_dev_opaque [1]. That's the place all the intern fields should,
> > > eventually,
> > > end up.
> > > 
> > > Now, quite some drivers in the trigger handler will read the masklength
> > > for looping
> > > with for_each_set_bit(). Hence, the straight thing would be an helper to
> > > get it.
> > > Maybe there's a clever way...
> > > 
> > > I know this is more work than what you had in mind but I think it should
> > > be fairly
> > > simple (hopefully) and since you started it :), maybe we can get the whole
> > > thing done
> > > and remove another [INTERN] member from the iio_dev struct.
> > > 
> > > [1]:
> > > https://elixir.bootlin.com/linux/latest/source/include/linux/iio/iio-opaque.h#L42
> > > 
> > > - Nuno Sá  
> > 
> > Sounds like fun. :-p
> > 
> > I will look into it.
> 
> I think this one might be miss marked as [INTERN]. It should be constant from
> the driver
> point of view, but given active_scan_masks is meant to be visible to the
> driver,
> it's length should probably be as well.
> 

Yeah, that did crossed my mind. I guess we should just make it [DRIVER] then
(likely with RO statement).

> Sure every driver should be able to trivially work this out for themselves,
> but
> do we care about stopping them using this?
> 
> It might be worth some nice iterator wrappers with names like
> iio_for_each_active_channel() though I'd expect those to still be accessing
> these

That looks like a good idea. It would make it more clear that member is not to
be directly accessed.

- Nuno Sßa