Re: [PATCH v4 5/8] iio: imu: Add driver for the ADI ADIS16607

[Andy Shevchenko]

On Mon, Oct 26, 2026 at 10:22:22AM +0000, Sabau, Radu bogdan wrote:
> > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> > Sent: Friday, October 23, 2026 1:27 PM

...

> > > +static int adis16607_firmware_revision_show(struct seq_file *s, void *arg)
> > > +{
> > > +	struct adis16607 *st = s->private;
> > > +	u16 rev;
> > 
> > Can this be
> > 
> > 	struct revision {
> > 		u8 minor;
> > 		u8 major;
> > 	} rev;
> > 
> > ?
> 
> Although I like the idea, I have a question here.
> If u16 rev would be replaced by the struct and the read was done
> directly on the struct pointer, could it mess things up on big-endian
> targets? A native 16-bit store would place the value's low byte at
> the lower address on little-endian, but the high byte at the lower
> address on big-endian - so minor (at the lower offset) would only
> get the right byte on little-endian, and would actually get major's
> byte on big-endian, right?
> 
> What about we declare this struct and also have a u16 tmp declared,
> do the read on tmp, and then rev.minor/rev.major get assigned via
> FIELD_GET() - keeping the readability improvement without that risk?
> 
> struct {
>         u8 minor;
>         u8 major;
> } rev;
> u16 tmp;
> int ret;
> 
> ret = adis_read_reg_16(&st->adis, ADIS16607_REG_REV_ID, &tmp);
> if (ret)
>         return ret;
> 
> rev.major = FIELD_GET(GENMASK(15, 8), tmp);
> rev.minor = FIELD_GET(GENMASK(7, 0), tmp);
> 
> seq_printf(s, "%u.%u\n", rev.major, rev.minor);

Yes, this will work for me.

-- 
With Best Regards,
Andy Shevchenko