Re: [PATCH v6 13/17] iio: adc: ad7768: Add calibration controls

[Jonathan Cameron]

> Expose the per-channel offset and gain calibration registers through the
> IIO calibbias and calibscale attributes.
> 
> Signed-off-by: Janani Sunil <janani.sunil@xxxxxxxxxx>

Sashiko raises some more good points in here.

Honestly in a large complex driver like this, humans (well me
anyway) are getting burnt out by this point in a series
so this sort of things slips through us.
Good to have a bot help out!  Note this is also why best practice
is to start with a simpler driver and add features in follow up series
that remain more manageable.

>
> diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> index f2c762688fb3..802666f7abd4 100644
> --- a/drivers/iio/adc/ad7768.c
> +++ b/drivers/iio/adc/ad7768.c
> @@ -27,6 +27,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/time.h>
>  #include <linux/types.h>
> +#include <linux/unaligned.h>
>  #include <linux/units.h>
>  
>  #include <linux/iio/backend.h>
> @@ -107,6 +108,7 @@
>  #define AD7768_MAX_FREQ_PER_MODE			6
>  #define AD7768_MAX_CHANNEL				8
>  #define AD7768_NUM_CHANNEL_MODES			2
> +#define AD7768_CALIB_REG_MSK				GENMASK(23, 0)
>  #define AD7768_WIDEBAND_SETTLING_SAMPLES		68
>  #define AD7768_SINC5_SETTLING_SAMPLES			7
>  
> @@ -232,6 +234,18 @@ static u8 ad7768_all_standby_mask(const struct ad7768_state *st)
>  	return GENMASK(st->chip_info->num_channels - 1, 0);
>  }
>  
> +static unsigned int ad7768_offset_reg(const struct ad7768_state *st,
> +				      unsigned int ch)
> +{
> +	return AD7768_REG_OFFSET(st->chip_info->chan_map[ch]);
> +}
> +
> +static unsigned int ad7768_gain_reg(const struct ad7768_state *st,
> +				    unsigned int ch)
> +{
> +	return AD7768_REG_GAIN(st->chip_info->chan_map[ch]);
> +}
> +
>  static u8 ad7768_precharge_buf1_mask(const struct ad7768_state *st, u16 val)
>  {
>  	return val & GENMASK(st->chip_info->prebuf_split - 1, 0);
> @@ -359,6 +373,59 @@ static const struct regmap_config ad7768_4_regmap_config = {
>  	.readable_reg = ad7768_4_readable_reg,
>  };
>  
> +static unsigned int ad7768_get_calib_reg_base(struct ad7768_state *st,
> +					      const struct iio_chan_spec *chan,
> +					      bool is_gain)
> +{
> +	if (is_gain)
> +		return ad7768_gain_reg(st, chan->channel);
> +
> +	return ad7768_offset_reg(st, chan->channel);
> +}
> +
> +static int ad7768_read_calib_value(struct ad7768_state *st,
> +				   unsigned int base_reg, unsigned int *val)
> +{
> +	u8 data[3];
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(regmap_get_device(st->regmap), pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_bulk_read(st->regmap, base_reg, data, sizeof(data));
> +	if (ret)
> +		return ret;
> +
> +	*val = get_unaligned_be24(data);

So you'll need to know whether or not to sign extend.
I'd pass in a bool to provide that info so you can still share
the functions

> +
> +	return 0;
> +}
> +
> +static int ad7768_write_calib_value(struct ad7768_state *st,
> +				    unsigned int base_reg, unsigned int val)
> +{
> +	u8 data[3];
> +	int ret;
> +
> +	if (val > AD7768_CALIB_REG_MSK)
> +		return -EINVAL;

This will need adjusting to deal with 2s comp bias values.

> +
> +	put_unaligned_be24(val, data);
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(regmap_get_device(st->regmap), pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	return regmap_bulk_write(st->regmap, base_reg, data, sizeof(data));
> +}
> +
>  static int ad7768_reg_access(struct iio_dev *indio_dev,
>  			     unsigned int reg,
>  			     unsigned int writeval,
> @@ -798,6 +865,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
>  			   int *val, int *val2, long info)
>  {
>  	struct ad7768_state *st = iio_priv(indio_dev);
> +	int ret;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -808,6 +876,21 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
>  
>  		return IIO_VAL_INT;
>  	}
> +
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +	case IIO_CHAN_INFO_CALIBSCALE: {
> +		bool is_gain = info == IIO_CHAN_INFO_CALIBSCALE;
> +		unsigned int base_reg;
> +		unsigned int calib;
> +
> +		base_reg = ad7768_get_calib_reg_base(st, chan, is_gain);
> +		ret = ad7768_read_calib_value(st, base_reg, &calib);
> +		if (ret)
> +			return ret;
> +
> +		*val = calib;

Sashiko:

[Severity: High]
Does this assignment correctly handle negative two's complement offsets?

Since calib is an unsigned integer populated from a 24-bit register, reading
a negative offset directly assigns it without sign extension. Will this cause
userspace to see massive positive integers (e.g., 0xFFFFFF) instead of the
actual negative offset, violating the IIO ABI?

-
I would indeed expect a sign_extend32() to deal with the 24 bit 2s
comp value of calibbias. Given that doesn't apply to scale you may
have to split the two paths.

> +		return IIO_VAL_INT;
> +	}
>  	default:
>  		return -EINVAL;
>  	}

...

>  static int ad7768_write_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int val, int val2, long info)
>  {
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	unsigned int base_reg;
> +
>  	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
>  	if (IIO_DEV_ACQUIRE_FAILED(claim))
>  		return -EBUSY;
>  
> -	if (info == IIO_CHAN_INFO_SAMP_FREQ)
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
>  		return ad7768_set_sampling_freq(indio_dev, val, chan->channel);
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (val < 0 || val > AD7768_CALIB_REG_MSK)
> +			return -EINVAL;

Sashiko:
[Severity: High]
Is it expected to explicitly block negative values for the calibration bias?

By returning -EINVAL when val < 0, this rejects valid negative integer
offsets that users or auto-calibration scripts might attempt to write. Should
negative values be converted to their 24-bit two's complement representation
instead of being rejected?
-
The register is 2s comp so the comment seems to be correct.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>