| Message ID | 20241205-fix-hid-sensor-v1-1-9b789f39c220@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | iio: hid-sensor-prox: Merge information from different channels | expand |
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, >
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),\
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,