Re: [PATCH v5 6/6] iio: imu: inv_icm42607: Add accelerometer calibbias support

[Kanak Shilledar]

Hi Andy,

Thanks for going through the patches.

On Fri, 2026-10-02 at 16:18 +0300, Andy Shevchenko wrote:
> On Fri, Oct 02, 2026 at 01:54:30PM +0200, Kanak Shilledar wrote:
> > Expose IIO_CHAN_INFO_CALIBBIAS on the accelerometer channels. The
> > registers are stored in MREG1. The calibration bias is written to
> > OFFSET_USER4 to OFFSET_USER8 registers in MREG1. Reject the out of
> > limit calibbias values instead of clamping it.
> > 
> > Note: The accelerometer functionality is tested with Invensense,
> > ICM42370-P development board.
> 
> ...
> 
> > +	case IIO_MOD_X:
> > +	case IIO_MOD_Z:
> > +		offset = sign_extend32(((lo_val & 0xF0) << 4) |
> > hi_val, 11);
> > +		break;
> > +	case IIO_MOD_Y:
> > +		offset = sign_extend32(((hi_val & 0x0F) << 8) |
> > lo_val, 11);
> > +		break;
> 
> Why do we have hi/lo and not proper __le16 or __be16 type for that to
> begin
> with?

The reason for having hi/lo is because the actual offset values are
split between two registers, as described in the section 16.33 to
section 16.37 of the datasheet [1].

MREG1 register     Contents
--------------     --------------------------------
OFFSET_USER4       X[11:8] | other bits
OFFSET_USER5       X[7:0]

OFFSET_USER6       Y[7:0]
OFFSET_USER7       Z[11:8] | Y[11:8]

OFFSET_USER8       Z[7:0]

> ...
> 
> > +	val64 = (s64)offset * 5LL * 9806650LL;
> > +	/* For rounding, add + or - divisor (10000) divided by 2
> > */
> > +	if (val64 >= 0)
> > +		val64 += 10000LL / 2LL;
> > +	else
> > +		val64 -= 10000LL / 2LL;
> > +
> > +	bias = div_s64(val64, 10000L);
> 
> We have DIV_S64_ROUND_CLOSEST().

I will replace it with the suggested one.


> ...
> 
> Overall, the feeling is that this is cumbersome change and may be
> split to
> smaller and more isolated logical updates.

Do you have any advice on how to split this patch series?

Thanks and Regards,
Kanak Shilledar

[1] https://www.lcsc.com/product-detail/C5129967.html

Attachment: signature.asc
Description: This is a digitally signed message part