Re: [PATCH v2 1/7] iio: HID: Add helper method hid_sensor_adjust_channel_bit_mask()

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

On Fri, 2026-04-24 at 20:23 +0100, Jonathan Cameron wrote:
> On Tue, 21 Apr 2026 19:20:33 -0300
> Natália Salvino André <natalia.andre@ime.usp.br> wrote:
> 
> > Add helper method to deduplicate code in HID sensors.
> > 
> > Signed-off-by: Natália Salvino André <natalia.andre@ime.usp.br>
> > Co-developed-by: Pietro Di Consolo Gregorio
> > <pietro.gregorio@usp.br>
> > Signed-off-by: Pietro Di Consolo Gregorio <pietro.gregorio@usp.br>
> > ---
> >  .../iio/common/hid-sensors/hid-sensor-attributes.c   | 12
> > ++++++++++++
> >  include/linux/hid-sensor-hub.h                       |  3 +++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > index c115a72832b2..3ee6e83c6cac 100644
> > --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > @@ -3,6 +3,7 @@
> >   * HID Sensors Driver
> >   * Copyright (c) 2012, Intel Corporation.
> >   */
> > +#include <linux/bits.h>
> >  #include <linux/module.h>
> >  #include <linux/kernel.h>
> >  #include <linux/time.h>
> > @@ -589,6 +590,17 @@ int hid_sensor_parse_common_attributes(struct
> > hid_sensor_hub_device *hsdev,
> >  }
> >  EXPORT_SYMBOL_NS(hid_sensor_parse_common_attributes, "IIO_HID");
> >  
> > +void hid_sensor_adjust_channel_bit_mask(struct iio_chan_spec
> > *channels,
> > +					int channel, int size)
> > +{
> > +	channels[channel].scan_type.format = 's';
> > +	/* Real storage bits will change based on the report desc.
> > */
> > +	channels[channel].scan_type.realbits = size *
> > BITS_PER_BYTE;
> > +	/* Maximum size of a sample to capture is u32 */
> > +	channels[channel].scan_type.storagebits = sizeof(u32) *
> > BITS_PER_BYTE;
> > +}
> > +EXPORT_SYMBOL_NS(hid_sensor_adjust_channel_bit_mask, "IIO_HID");
> > +
> Looking at this again:
> 
> This feels like a helper that doesn't necessarily add much value.
I think so.

> 
> 		channels[CHANNEL_SCAN_INDEX_X + i].scan_type =
> (struct iio_scan_type) {
> 			.format = 's',
> 			.real_bits = BYTES_TO_BITS(st-
> >accel[CHANNEL_SCAN_INDEX_X + i].size),
> 			.storage_bits = BITS_PER_TYPE(u32),
> 		};
> 
> Maybe with local variables to help a touch. such as
> 		unsigned int ch = CHANNEL_SCAN_INDEX_X + i];
> 
> 		channels[ch].scan_type = (struct iio_scan_type) {
> 			.format = 's',
> 			.real_bits = BYTES_TO_BITS(st-
> >accel[ch].size),
> 			.storage_bits = BITS_PER_TYPE(u32),
> 		};
> Whilst it technically sets other parts of scan_type to 0, they are 0
> anyway
> so that's harmless given readability improvements.
> 
> There is another two uses for ch just above as well so makes this
> even easier to
> argue in favour of as a change as it'll be (this is effectively
> replacing patch 2)
> 
> 	for (unsigned int i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
> 		unsigned int ch = CHANNEL_SCAN_INDEX_X + i;
> 
> 		ret = sensor_hub_input_get_attribute_info(hsdev,
> 				HID_INPUT_REPORT,
> 				usage_id, ch,
> 				&st->accel[ch]);
> 
> 		if (ret < 0)
> 			break;
> 		channels[ch].scan_type = (struct iio_scan_type) {
> 			.format = 's',
> 			.real_bits = BYTES_TO_BITS(st-
> >accel[ch].size),
> 			.storage_bits = BITS_PER_TYPE(u32),
> 		};
> 	}
> 
> Hmm. That's also an odd loop as the use assumes CHANNEL_SCAN_INDEX_X
> = 0
> (which is true) but then uses an offset that implies it isn't.
> Clearer to
> just loop over the actual enum values.
> 
> So probably wants to be something like.
> 
> 	for (unsigned int ch = CHANNEL_SCAN_INDEX_X;
> 	     i <= CHANNEL_SCAN_INDEX_Z; ch++) { //maybe on a long
> single line.
> 		ret = sensor_hub_input_get_attribute_info(hsdev,
> 							 
> HID_INPUT_REPORT,
> 							  usage_id,
> ch,
> 							  &st-
> >accel[ch]);
> 		if (ret < 0)
> 			break;
> 
> 		channels[ch].scan_type = (struct iio_scan_type) {
> 			.format = 's',
> 			.real_bits = BYTES_TO_BITS(st-
> >accel[ch].size),
> 			.storage_bits = BITS_PER_TYPE(u32),
> 		};
> 	}
> 	
> 
This change is better than adding another export function.

Thanks,
Srinivas


> 
> >  MODULE_AUTHOR("Srinivas Pandruvada
> > <srinivas.pandruvada@intel.com>");
> >  MODULE_DESCRIPTION("HID Sensor common attribute processing");
> >  MODULE_LICENSE("GPL");
> > diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-
> > sensor-hub.h
> > index e71056553108..6523d46c63e0 100644
> > --- a/include/linux/hid-sensor-hub.h
> > +++ b/include/linux/hid-sensor-hub.h
> > @@ -281,4 +281,7 @@ bool hid_sensor_batch_mode_supported(struct
> > hid_sensor_common *st);
> >  int hid_sensor_set_report_latency(struct hid_sensor_common *st,
> > int latency);
> >  int hid_sensor_get_report_latency(struct hid_sensor_common *st);
> >  
> > +void hid_sensor_adjust_channel_bit_mask(struct iio_chan_spec
> > *channels,
> > +					int channel, int size);
> > +
> >  #endif