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

[Jonathan Cameron <jic23@kernel.org>]

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.

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.

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

static inline int iio_dev_mask_length(struct iio_dev *indio_dev)
{
	return ACCESS_PRIVATE()...
}

or can just review for anyone doing iio_dev_mask_length(indio_dev) = 4;




> 
> ---
> Nuno Sa (3):
>       iio: core: add new helper to iterate active channels
>       iio: imu: adis16475: make use of iio_for_each_active_channel()
>       iio: core annotate masklength as private
> 
>  drivers/iio/imu/adis16475.c | 3 +--
>  include/linux/iio/iio.h     | 8 +++++++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> ---
> base-commit: cc1ce839526a65620778617da0b022bd88e8a139
> change-id: 20240612-dev-iio-scan-private-86f4a0fd288f
> --
> 
> Thanks!
> - Nuno Sá
>