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@xxxxxxxxxx> 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á
