Re: [PATCH v2 2/7] iio: accel: HID: Replace method accel_3d_adjust_channel_bit_mask()

Re: [PATCH v2 2/7] iio: accel: HID: Replace method accel_3d_adjust_channel_bit_mask()

[Andy Shevchenko]

On Fri, Apr 24, 2026 at 08:08:47PM +0100, Jonathan Cameron wrote:
> On Wed, 22 Apr 2026 12:07:07 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Apr 21, 2026 at 07:20:34PM -0300, Natália Salvino André wrote:

...

> > > -		accel_3d_adjust_channel_bit_mask(channels,
> > > -				CHANNEL_SCAN_INDEX_X + i,
> > > -				st->accel[CHANNEL_SCAN_INDEX_X + i].size);
> > > +		hid_sensor_adjust_channel_bit_mask(channels,
> > > +			CHANNEL_SCAN_INDEX_X + i,
> > > +			st->accel[CHANNEL_SCAN_INDEX_X + i].size);  
> > 
> > Indentation is broken. Taking into account that the last line is too long when
> > properly indented, perhaps
> > 
> > 		hid_sensor_adjust_channel_bit_mask(channels,
> > 				CHANNEL_SCAN_INDEX_X + i,
> > 				st->accel[CHANNEL_SCAN_INDEX_X + i].size);
> > 
> > Which makes it most right and under 80 limit.

> Why the double tab? Maybe just go long on this one and align after the (

I never know when you are strict about 80 limit :-)


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/7] iio: accel: HID: Replace method accel_3d_adjust_channel_bit_mask()

[Jonathan Cameron]

On Mon, 27 Apr 2026 12:38:44 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Fri, Apr 24, 2026 at 08:08:47PM +0100, Jonathan Cameron wrote:
> > On Wed, 22 Apr 2026 12:07:07 +0300
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> > > On Tue, Apr 21, 2026 at 07:20:34PM -0300, Natália Salvino André wrote:  
> 
> ...
> 
> > > > -		accel_3d_adjust_channel_bit_mask(channels,
> > > > -				CHANNEL_SCAN_INDEX_X + i,
> > > > -				st->accel[CHANNEL_SCAN_INDEX_X + i].size);
> > > > +		hid_sensor_adjust_channel_bit_mask(channels,
> > > > +			CHANNEL_SCAN_INDEX_X + i,
> > > > +			st->accel[CHANNEL_SCAN_INDEX_X + i].size);    
> > > 
> > > Indentation is broken. Taking into account that the last line is too long when
> > > properly indented, perhaps
> > > 
> > > 		hid_sensor_adjust_channel_bit_mask(channels,
> > > 				CHANNEL_SCAN_INDEX_X + i,
> > > 				st->accel[CHANNEL_SCAN_INDEX_X + i].size);
> > > 
> > > Which makes it most right and under 80 limit.  
> 
> > Why the double tab? Maybe just go long on this one and align after the (  
> 
> I never know when you are strict about 80 limit :-)
> 
I was meaning vs single tab really.

You are slowly wearing me down on the 80 limit by pointing out lots of
cases where going beyond it really helps! ;)

Re: [PATCH v2 2/7] iio: accel: HID: Replace method accel_3d_adjust_channel_bit_mask()

[Andy Shevchenko]

On Tue, Apr 28, 2026 at 07:12:46PM +0100, Jonathan Cameron wrote:
> On Mon, 27 Apr 2026 12:38:44 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Fri, Apr 24, 2026 at 08:08:47PM +0100, Jonathan Cameron wrote:
> > > On Wed, 22 Apr 2026 12:07:07 +0300
> > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> > > > On Tue, Apr 21, 2026 at 07:20:34PM -0300, Natália Salvino André wrote:  

...

> > > > > -		accel_3d_adjust_channel_bit_mask(channels,
> > > > > -				CHANNEL_SCAN_INDEX_X + i,
> > > > > -				st->accel[CHANNEL_SCAN_INDEX_X + i].size);
> > > > > +		hid_sensor_adjust_channel_bit_mask(channels,
> > > > > +			CHANNEL_SCAN_INDEX_X + i,
> > > > > +			st->accel[CHANNEL_SCAN_INDEX_X + i].size);    
> > > > 
> > > > Indentation is broken. Taking into account that the last line is too long when
> > > > properly indented, perhaps
> > > > 
> > > > 		hid_sensor_adjust_channel_bit_mask(channels,
> > > > 				CHANNEL_SCAN_INDEX_X + i,
> > > > 				st->accel[CHANNEL_SCAN_INDEX_X + i].size);
> > > > 
> > > > Which makes it most right and under 80 limit.  
> > 
> > > Why the double tab? Maybe just go long on this one and align after the (  
> > 
> > I never know when you are strict about 80 limit :-)
> > 
> I was meaning vs single tab really.

Ah, my point if the line is too long and maintainer is too strict about
the limit the indentation I would like to use is to occupy as much room as
possible. I.o.w. to indent right as much as possible to fit the limit.

> You are slowly wearing me down on the 80 limit by pointing out lots of
> cases where going beyond it really helps! ;)

Ha-ha :-)

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Apr 24, 2026 at 08:01:55PM +0100, Jonathan Cameron wrote:
> On Wed, 22 Apr 2026 12:03:12 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Apr 21, 2026 at 07:20:33PM -0300, Natália Salvino André wrote:
> > > Add helper method to deduplicate code in HID sensors.  

...

> > > +	channels[channel].scan_type.realbits = size * BITS_PER_BYTE;  
> > 
> > BITS_TO_BYTES(size)
> 
> BYTES_TO_BITS(size)

Indeed.

-- 
With Best Regards,
Andy Shevchenko

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

[srinivas pandruvada]

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