Message-ID: <9d1ac78951439bbcdc2f294713f55ee34e30f84e.camel@gmail.com>
Date: Mon, 29 Apr 2024 09:17:18 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, David Lechner
 <dlechner@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Shawn Guo <shawnguo@...nel.org>, Sascha
 Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team
 <kernel@...gutronix.de>,  Fabio Estevam <festevam@...il.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 0/3] iio: cleanup masklength usage

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

Powered by blists - more mailing lists