Re: [PATCH v2 2/6] iio: adc: ad4030: add driver for ad4030-24

[Jonathan Cameron <jic23@kernel.org>]

Just commenting on one particular bit feedback. Makes sure to address the
rest!

> > +
> > +static int ad4030_get_chan_calibscale(struct iio_dev *indio_dev,
> > +				      struct iio_chan_spec const *chan,
> > +				      int *val,
> > +				      int *val2)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	u16 gain;
> > +	int ret;
> > +
> > +	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(chan->address),
> > +			       st->rx_data.raw, AD4030_REG_GAIN_BYTES_NB);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gain = get_unaligned_be16(st->rx_data.raw);  
> My impression is that it is a bit odd to handle endianness after/before
> calling regmap_read/write(). Can you try set
> .val_format_endian_default = REGMAP_ENDIAN_BIG, in ad4030_regmap_bus?
> If that doesn't help, what about doing the get/set_unaligned stuff within
> the bus read/write calls?

Unless these are all 16 bit registers (in which case push it into the
regmap side of things), then a bulk read is not implying the registers
read are one value. They could just be a load of registers next to each other.
As such the regmap core won't touch them and endian conversion here, at the
layer where we know they are related is the right thing to do.

It's  not worth dual regmap stuff just because we have a few pairs of
registers. 


> 
> > +
> > +	/* From datasheet: multiplied output = input × gain word/0x8000 */
> > +	*val = gain / 0x8000;  
> Use a define to give a name to the gain constant?
> 
> > +	*val2 = mul_u64_u32_div(gain % 0x8000, NANO, 0x8000);
> > +
> > +	return IIO_VAL_INT_PLUS_NANO;
> > +}
> > +  
> [...]