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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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