Re: [PATCH] iio: hid-sensor-prox: Merge information from different channels

[Jonathan Cameron]

On Sun, 8 Dec 2024 21:09:16 +0100
Ricardo Ribalda <ribalda@xxxxxxxxxxxx> wrote:

> Hi Jonathan
> 
> 
> On Sun, 8 Dec 2024 at 17:39, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Thu, 05 Dec 2024 12:59:20 +0000
> > Ricardo Ribalda <ribalda@xxxxxxxxxxxx> wrote:
> >  
> > > The device only provides a single scale, frequency and hysteresis for
> > > all the channels. Fix the info_mask_* to match the reality of the
> > > device.
> > >
> > > Without this patch:
> > > in_attention_scale
> > > in_attention_hysteresis
> > > in_attention_input
> > > in_attention_offset
> > > in_attention_sampling_frequency
> > > in_proximity_scale
> > > in_proximity_sampling_frequency
> > > in_proximity_offset
> > > in_proximity0_raw
> > > in_proximity_hysteresis
> > >
> > > With this patch:
> > > hysteresis
> > > scale
> > > sampling_frequency
> > > in_attention_input
> > > in_attention_offset
> > > in_proximity0_offset
> > > in_proximity0_raw
> > >
> > > Fixes: 596ef5cf654b ("iio: hid-sensor-prox: Add support for more channels")
> > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>  
> >
> > whilst perhaps not ideal use of the ABI, what is there today is not wrong
> > as such.  If the ABI above was all introduce in the recent patch I might
> > be fine adjusting it as you suggestion. However it wasn't, in_proximity_scale
> > has been there a long time so this would be an ABI change.
> > Those are generally only ok if there is a bug.
> >
> > Drivers are always allowed to provide finer granularity than necessary
> > so in this case I don't see this as a bug.  
> 
> Is it ok that changing the attention_sampling frequency the
> proximity_sampling frequency changes as well?
> (Just asking for my own education, not complaining :) )

Yes.  In general the ABI has always had to allow for interactions because
there are lots of non obvious ones between attributes for different channels
as well as those for the same channels.

> 
> Also, what about ?:
> in_attention_scale
> in_attention_hysteresis
> in_attention_input
> in_attention_offset
> in_attention_sampling_frequency
> in_proximity0_scale
> in_proximity0_sampling_frequency
> in_proximity0_offset
> in_proximity0_raw
> in_proximity0_hysteresis
> 
> Would that be acceptable? I think that if we are giving the false
> impression that every sampling frequency is independent we should go
> all the way in. WDYT?

It's indeed far from ideal, but so is changing an ABI we've exposed to
userspace. We definitely can't touch anything in a release kernel but if
there are clear improvements to be made on stuff that we can sort of term
a fix we can maybe get away with it.


> 
> Thanks!
> 
> ps: this patch is in the queue in case you missed it
> https://lore.kernel.org/linux-iio/20241122-fix-processed-v2-1-b9f606d3b519@xxxxxxxxxxxx/
It's in patchwork so i'll get to it. Not sure why I haven't applied it, maybe a tree
management thing and lack of time last weekend to check for what was unblocked by
the rebase.  I'll catch up soon.

Jonathan

> 
> That one is a real fix for the driver :)
> 
> >
> > Jonathan
> >
> >  
> > > ---
> > >  drivers/iio/light/hid-sensor-prox.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
> > > index e8e7b2999b4c..f21d2da4c7f9 100644
> > > --- a/drivers/iio/light/hid-sensor-prox.c
> > > +++ b/drivers/iio/light/hid-sensor-prox.c
> > > @@ -49,9 +49,11 @@ 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),\
> > > +             .info_mask_shared_by_all = \
> > >               BIT(IIO_CHAN_INFO_SCALE) |\
> > >               BIT(IIO_CHAN_INFO_SAMP_FREQ) |\
> > >               BIT(IIO_CHAN_INFO_HYSTERESIS),\
> > >
> > > ---
> > > base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
> > > change-id: 20241203-fix-hid-sensor-62e1979ecd03
> > >
> > > Best regards,  
> >  
> 
>