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

[Sabau, Radu bogdan]

> -----Original Message-----
> 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;
> 
> ?
> 

Hi Andy! Thank you also for looking at this series.

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