diff mbox series

iio: hid-sensor-prox: Merge information from different channels

Message ID 20241205-fix-hid-sensor-v1-1-9b789f39c220@chromium.org (mailing list archive)
State Rejected
Headers show
Series iio: hid-sensor-prox: Merge information from different channels | expand

Commit Message

Ricardo Ribalda Dec. 5, 2024, 12:59 p.m. UTC 
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>
---
 drivers/iio/light/hid-sensor-prox.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)


---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241203-fix-hid-sensor-62e1979ecd03

Best regards,

Comments

Jonathan Cameron Dec. 8, 2024, 4:38 p.m. UTC | #1 
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.

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 Dec. 8, 2024, 8:09 p.m. UTC | #2 
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 :) )

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

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,
>
Jonathan Cameron Dec. 11, 2024, 6:40 p.m. UTC | #3 
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.

> 
> 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,  
> >  
> 
>
Srinivas Pandruvada Dec. 11, 2024, 9:48 p.m. UTC | #4 
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,  
> > >  
> > 
> > 
>
diff mbox series

Patch

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