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

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Wed, 2024-12-11 at 18:40 +0000, Jonathan Cameron wrote:
> On Sun, 8 Dec 2024 21:09:16 +0100
> Ricardo Ribalda <ribalda@chromium.org> wrote:
> 
> > Hi Jonathan
> > 
> > 
> > On Sun, 8 Dec 2024 at 17:39, Jonathan Cameron <jic23@kernel.org>
> > wrote:
> > > 
> > > On Thu, 05 Dec 2024 12:59:20 +0000
> > > Ricardo Ribalda <ribalda@chromium.org> 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@chromium.org>  
> > > 
> > > 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.

In general if this is by a soft sensor in the hub, then likely all will
change the same sampling frequency internally since they don't have a
real sensor in the back.

Thanks,
Srinivas


> 
> > 
> > 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@chromium.org/
> 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,  
> > >  
> > 
> > 
>