Re: [PATCH v2] iio: hid-sensor-prox: Split difference from multiple channels

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 11 Jan 2025 10:17:27 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Hi Jonathan
> 
> Happy new year!
> 
> Friendly ping about this patch so we can change the ABI before the
> kernel release happens

We might not quite make that :(  Srinivas, Jiri I'm looking for your
input on this.

I'm fine with us taking this as a fix that goes into an early point
release on basis only crazy people base products on a version that hasn't
gotten the fixes that inevitably only go in a few weeks later.

Jonathan

> 
> On Thu, 19 Dec 2024 at 18:17, Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 16 Dec 2024 10:05:53 +0000
> > Ricardo Ribalda <ribalda@chromium.org> wrote:
> >  
> > > When the driver was originally created, it was decided that
> > > sampling_frequency and hysteresis would be shared_per_type instead
> > > of shared_by_all (even though it is internally shared by all). Eg:
> > > in_proximity_raw
> > > in_proximity_sampling_frequency
> > >
> > > When we introduced support for more channels, we continued with
> > > shared_by_type which. Eg:
> > > in_proximity0_raw
> > > in_proximity1_raw
> > > in_proximity_sampling_frequency
> > > in_attention_raw
> > > in_attention_sampling_frequency
> > >
> > > Ideally we should change to shared_by_all, but it is not an option,
> > > because the current naming has been a stablished ABI by now. Luckily we
> > > can use separate instead. That will be more consistent:
> > > in_proximity0_raw
> > > in_proximity0_sampling_frequency
> > > in_proximity1_raw
> > > in_proximity1_sampling_frequency
> > > in_attention_raw
> > > in_attention_sampling_frequency
> > >
> > > Fixes: 596ef5cf654b ("iio: hid-sensor-prox: Add support for more channels")
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>  
> >
> > I got lost somewhere in the discussion.  This is still an ABI change compared
> > to original interface at the top (which is the one that has been there
> > quite some time).
> >
> > However we already had to make one of those to add the index that wasn't there
> > for _raw. (I'd missed that in earlier discussion - thanks for laying out the
> > steps here!)  Srinivas, Jiri, do you think we are better off just assuming users
> > of this will be using a library that correctly deals with sharing and just
> > jump to
> > in_proximity0_raw
> > in_proximity1_raw
> > in_attention_raw
> > (should have indexed that but it may never matter) and
> > sampling_frequency
> >
> > Which is what I think Ricardo originally asked.
> >
> > Do we have any guarantee the sampling_frequency will be shared across the
> > sensor channels?  It may be the most common situation but I don't want to
> > wall us into a corner if it turns out someone runs separate sensors at
> > different rates (no particularly reason they should be one type of sensor
> > so this might make sense).  If we don't have that guarantee
> > then this patch is fine as far as I'm concerned.
> >
> > Jonathan
> >
> >
> >  
> > > ---
> > > Changes in v2:
> > > - Use separate
> > > - Link to v1: https://lore.kernel.org/r/20241205-fix-hid-sensor-v1-1-9b789f39c220@chromium.org
> > > ---
> > >  drivers/iio/light/hid-sensor-prox.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
> > > index c83acbd78275..71dcef3fbe57 100644
> > > --- a/drivers/iio/light/hid-sensor-prox.c
> > > +++ b/drivers/iio/light/hid-sensor-prox.c
> > > @@ -49,9 +49,10 @@ static const u32 prox_sensitivity_addresses[] = {
> > >  #define PROX_CHANNEL(_is_proximity, _channel) \
> > >       {\
> > >               .type = _is_proximity ? IIO_PROXIMITY : IIO_ATTENTION,\
> > > -             .info_mask_separate = _is_proximity ? BIT(IIO_CHAN_INFO_RAW) :\
> > > -                                   BIT(IIO_CHAN_INFO_PROCESSED),\
> > > -             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |\
> > > +             .info_mask_separate = \
> > > +             (_is_proximity ? BIT(IIO_CHAN_INFO_RAW) :\
> > > +                             BIT(IIO_CHAN_INFO_PROCESSED)) |\
> > > +             BIT(IIO_CHAN_INFO_OFFSET) |\
> > >               BIT(IIO_CHAN_INFO_SCALE) |\
> > >               BIT(IIO_CHAN_INFO_SAMP_FREQ) |\
> > >               BIT(IIO_CHAN_INFO_HYSTERESIS),\
> > >
> > > ---
> > > base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
> > > change-id: 20241203-fix-hid-sensor-62e1979ecd03
> > >
> > > Best regards,  
> >  
> 
>