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

[Jonathan Cameron <jic23@kernel.org>]

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.

		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),
		};
	}
	


>  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