Re: [PATCH v4 8/8] iio: imu: adis16607: Add calibration bias support for gyro/accel

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 23 Oct 2026 12:30:25 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add IIO_CHAN_INFO_CALIBBIAS support for the gyroscope and
> accelerometer axes, backed by the device's per-axis bias registers
> (XA/YA/ZA_BIAS, XG/YG/ZG_BIAS).
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
I'd missed the use of scan_index in here.  That seems like very
fragile coding to me. Moving over to address is going to require
changes a few patches back.

> +
>  /*
>   * SPI Half-Duplex read operation for ADIS16607.
>   * Protocol:
> @@ -863,6 +878,7 @@ static int adis16607_read_raw(struct iio_dev *indio_dev,
>  			      int *val, int *val2, long info)
>  {
>  	struct adis16607 *st = iio_priv(indio_dev);
> +	const int idx = chan->scan_index;

As below. This to me is hurting readability a little.
Using scan_index for this is a bit fragile anyway as those can
be changed by people expecting them to only about buffered capture.
This should be using address and that may well mean you need that
to become a lookup into an array of multiple register addresses
needed for a given channel rather than directly to one register as
currently.


>  	int ret;
>  	u32 tmp;
>  
> @@ -900,6 +916,16 @@ static int adis16607_read_raw(struct iio_dev *indio_dev,
>  		*val = 5000;
>  		return IIO_VAL_INT;
>  
> +	case IIO_CHAN_INFO_CALIBBIAS: {
> +		u16 tmp16;
> +
> +		ret = adis_read_reg_16(&st->adis, adis16607_calib_bias[idx], &tmp16);
> +		if (ret)
> +			return ret;
> +		*val = sign_extend32(tmp16, 15);
> +		return IIO_VAL_INT;
> +	}
> +
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		ret = adis16607_get_freq(st, &tmp);
>  		if (ret)
> @@ -918,6 +944,7 @@ static int adis16607_write_raw(struct iio_dev *indio_dev,
>  			       int val, int val2, long info)
>  {
>  	struct adis16607 *st = iio_priv(indio_dev);
> +	const int idx = chan->scan_index;

I'd just go long on the line that uses this or at least push declaring it
down to the case statement that uses it.

>  	u32 tmp;
>  
>  	switch (info) {
> @@ -925,6 +952,11 @@ static int adis16607_write_raw(struct iio_dev *indio_dev,
>  		tmp = val * 1000 + val2 / 1000;
>  		return adis16607_set_freq(st, tmp);
>  
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (val < S16_MIN || val > S16_MAX)
> +			return -EINVAL;
> +		return adis_write_reg_16(&st->adis, adis16607_calib_bias[idx], val);
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -934,7 +966,8 @@ static int adis16607_write_raw(struct iio_dev *indio_dev,
>  	.type = (_type), \
>  	.modified = 1, \
>  	.channel2 = (_mod), \
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			      BIT(IIO_CHAN_INFO_CALIBBIAS), \
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
>  	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>  	.address = (_address), \
>