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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The IIO CALIBBIAS attribute incorrectly handles negative offsets, breaking two's complement offset calibration and violating the IIO ABI.
--

commit c96b4e9aa2c8a4f4230f53b26b0c87ecec23865f
Author: Janani Sunil <janani.sunil@analog.com>

iio: adc: ad7768: Add calibration controls

This commit exposes the per-channel offset and gain calibration registers
through the IIO calibbias and calibscale attributes.

> diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> index f2c762688fb39..802666f7abd45 100644
> --- a/drivers/iio/adc/ad7768.c
> +++ b/drivers/iio/adc/ad7768.c

[ ... ]

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

[ ... ]

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

[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?

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

[ ... ]

> @@ -816,24 +899,45 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
>  static int ad7768_write_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int val, int val2, long info)
>  {

[ ... ]

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

[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?

> 
> -	return -EINVAL;
> +		base_reg = ad7768_get_calib_reg_base(st, chan, false);
> +		return ad7768_write_calib_value(st, base_reg, val);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-ad7768-driver-v6-0-e4378f946bfb@analog.com?part=13