On Sun, 2024-04-28 at 14:23 +0100, Jonathan Cameron wrote: > On Fri, 26 Apr 2024 10:26:31 -0500 > David Lechner <dlechner@xxxxxxxxxxxx> wrote: > > > On Fri, Apr 26, 2024 at 2:13 AM Nuno Sá <noname.nuno@xxxxxxxxx> 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
