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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 :) )

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?

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/

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


-- 
Ricardo Ribalda




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux