Re: [PATCH RFC 0/3] iio: add helpers and accessors for active channels and masklength

["Nuno Sá" <noname.nuno@gmail.com>]

On Sat, 2024-06-15 at 14:18 +0100, Jonathan Cameron wrote:
> On Wed, 12 Jun 2024 16:20:47 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > Hi Jonathan,
> > 
> > In [1], you suggested for an iterator for the active channels (so driver
> > don't directly access masklength). This RFC showcases that iterator and
> > goes one step further by giving an accessors for masklength so that
> > drivers can read that variable (we have drivers doing that). The
> > accessors uses ACCESS_PRIVATE() so it will warn us if some driver
> > directly access the variable making it more difficult to mess with it
> > (like changing it's value) without being noticed during review (or the
> > auto builders).
> > 
> > Anyways, before jumping in changing all the drivers using this, I guess
> > the questions are:
> > 
> > 1) Is the iterator useful enough to add one (kind of like it and save a
> > line of code :))?
> > 2) Do we care about going with the work of marking masklength private? 
> > 
> > If we go ahead the plan would be:
> > 
> > 1) Add the helpers macros;
> > 2) Convert all drivers that directly access 'masklength';
> > 3) Annotate it as private.
> > 
> > [1]: https://lore.kernel.org/linux-iio/20240428142343.5067c898@jic23-huawei/
> 
> Cute. I'd not seen the __private bit before.

Yeah, I first noticed it in <linux/irq.h>
> 
> Looks good to me.  I think we should spin it a little differently.
> 1. Add macro and a dummy 
> 
> #define iio_dev_mask_length(indio_dev) (indio_dev)->mask_length
> 
> 2. Convert drivers
> 
> 3. What you have + the ACCESS_PRIVATE change.
> 

Agreed. Looks better

> that accessor still lets people change it rather than making
> it strictly private.   I wonder if we need a little more complicated
> 

I do prefer your inline function as it's a stronger guarantee... Though changing it
through the macro is also odd enough that should clearly pick the reviewers
attention.

- Nuno Sá