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

[andriy.shevchenko@xxxxxxxxx]

On Fri, Oct 02, 2026 at 02:03:56PM +0000, Kanak Shilledar wrote:
> 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:

...

> > > +		offset = sign_extend32(((lo_val & 0xF0) << 4) |
> > > hi_val, 11);

> > > +		offset = sign_extend32(((hi_val & 0x0F) << 8) |
> > > lo_val, 11);

> > 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]

Oh, this is unfortunate.

...

> > 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?

I have just generic recommendation: try to find all pieces that can be done
before the main work, like renaming something, replacing, moving in the code
and so on. I haven't deeply checked the code, my feelings purely based on my
review experience and contribution that I made in the past decade or so.
I truly believe it's possible to split.

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


-- 
With Best Regards,
Andy Shevchenko