Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

Jonathan Cameron
2025-07-31T13:04:13Z

Please crop to the remaining discussion points.

> > > > > +
> > > > > +#define TLV493D_DATA_X_GET(b) \
> > > > > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> > > > > + (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
> > > >
> > > > These are odd enough I'd make them c functions rather than macros. Burn a few lines
> > > > for better readability.
> > > >
> > > I saw this kind of data retrival and formation from registers as macros so I sticked to
> > > it. Having all these as function will also require a seperate function
> > > for each channel coz the masks and the layout of the bits changes over
> > > the register. Do you still recommend it as c functions?
> >
> > Is it more than 4 short functions? I'd burn the few lines that costs.
> >
> > s32 tlv493d_data_y_get(u8 *buff)
> > {
> > u16 val = FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> > FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
> >
> > return sign_extend32(val, 11);
> > }
> Okay.
> Will a single function with channel as arguments will be better?

IIRC I gave that a go as my first try before falling back to this.
You either need a look up table, or you need to pass
a lot of parameters. In the end it felt simpler to just have 4 small functions.

If you can come up with a clean and readable way of doing so, go for it!

Jonathan