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

["Pandruvada, Srinivas" <srinivas.pandruvada@intel.com>]

On Sat, 2025-01-11 at 10:17 +0100, Ricardo Ribalda wrote:
> Hi Jonathan
> 
> Happy new year!
> 
> Friendly ping about this patch so we can change the ABI before the
> kernel release happens
> 
> 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!) 

Didn't realize this. I don't see proximity sensor use in the mainline
Linux distro user space, so it will affect only some private user space
programs. 
Adding Mark to see if it affects Lenovo Sensing solution as there was
specific custom sensor added to this driver for Lenovo.

> >  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.

From HID sensor spec, these are three different sensor usage IDs. So
you can have three different sensor properties like sampling frequency.

#define HID_USAGE_SENSOR_TYPE_BIOMETRIC_PRESENCE	0x11
#define HID_USAGE_SENSOR_TYPE_BIOMETRIC_PROXIMITY	0x12
#define HID_USAGE_SENSOR_TYPE_BIOMETRIC_TOUCH		0x13

This driver either loads on HID_USAGE_SENSOR_TYPE_BIOMETRIC_PRESENCE or
Lenovo custom sensor, which merge all channels, in that case they will
share one sampling frequency for all.

So there is no guarantee.

Thanks,
Srinivas


> > 
> > 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,
> > 
> 
>