Re: [PATCH 3/5] iio: adc: ad7606: add offset and phase calibration support

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Add support for offset and phase calibration, only for
> devices that support software mode, that are:
> 
> ad7606b
> ad7606c-16
> ad7606c-18
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  drivers/iio/adc/ad7606.c | 160
> +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/iio/adc/ad7606.h |   9 +++
>  2 files changed, 169 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index
> ad5e6b5e1d5d2edc7f8ac7ed9a8a4e6e43827b85..ec063dd4a67eb94610c41c473e2d8040c919
> 74cf 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -95,6 +95,22 @@ static const unsigned int ad7616_oversampling_avail[8] = {
>  	1, 2, 4, 8, 16, 32, 64, 128,
>  };
>  
> +static const int ad7606_calib_offset_avail[3] = {
> +	-128, 1, 127,
> +};
> +
> +static const int ad7606c_18bit_calib_offset_avail[3] = {
> +	-512, 4, 511,
> +};

From the DS, it seems this is 508?

> +
> +static const int ad7606b_calib_phase_avail[][2] = {
> +	{ 0, 0 }, { 0, 1250 }, { 0, 318750 },
> +};
> +
> +static const int ad7606c_calib_phase_avail[][2] = {
> +	{ 0, 0 }, { 0, 1000 }, { 0, 255000 },
> +};
> +
>  static int ad7606c_18bit_chan_scale_setup(struct iio_dev *indio_dev,
>  					  struct iio_chan_spec *chan);
>  static int ad7606c_16bit_chan_scale_setup(struct iio_dev *indio_dev,
> @@ -164,6 +180,8 @@ const struct ad7606_chip_info ad7606b_info = {
>  	.scale_setup_cb = ad7606_16bit_chan_scale_setup,
>  	.sw_setup_cb = ad7606b_sw_mode_setup,
>  	.offload_storagebits = 32,
> +	.calib_offset_avail = ad7606_calib_offset_avail,
> +	.calib_phase_avail = ad7606b_calib_phase_avail,
>  };
>  EXPORT_SYMBOL_NS_GPL(ad7606b_info, "IIO_AD7606");
>  
> @@ -177,6 +195,8 @@ const struct ad7606_chip_info ad7606c_16_info = {
>  	.scale_setup_cb = ad7606c_16bit_chan_scale_setup,
>  	.sw_setup_cb = ad7606b_sw_mode_setup,
>  	.offload_storagebits = 32,
> +	.calib_offset_avail = ad7606_calib_offset_avail,
> +	.calib_phase_avail = ad7606c_calib_phase_avail,
>  };
>  EXPORT_SYMBOL_NS_GPL(ad7606c_16_info, "IIO_AD7606");
>  
> @@ -226,6 +246,8 @@ const struct ad7606_chip_info ad7606c_18_info = {
>  	.scale_setup_cb = ad7606c_18bit_chan_scale_setup,
>  	.sw_setup_cb = ad7606b_sw_mode_setup,
>  	.offload_storagebits = 32,
> +	.calib_offset_avail = ad7606c_18bit_calib_offset_avail,
> +	.calib_phase_avail = ad7606c_calib_phase_avail,
>  };
>  EXPORT_SYMBOL_NS_GPL(ad7606c_18_info, "IIO_AD7606");
>  
> @@ -683,6 +705,40 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev,
> unsigned int ch,
>  	return ret;
>  }
>  
> +static int ad7606_get_calib_offset(struct ad7606_state *st, int ch, int *val)
> +{
> +	int ret;
> +
> +	ret = st->bops->reg_read(st, AD7606_CALIB_OFFSET(ch));
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = st->chip_info->calib_offset_avail[0] +
> +		ret * st->chip_info->calib_offset_avail[1];
> +
> +	return 0;
> +}
> +
> +static int ad7606_get_calib_phase(struct ad7606_state *st, int ch, int *val,
> +				  int *val2)
> +{
> +	int ret;
> +
> +	ret = st->bops->reg_read(st, AD7606_CALIB_PHASE(ch));
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = 0;
> +
> +	/*
> +	 * ad7606b: phase delay from 0 to 318.75 μs in steps of 1.25 μs.
> +	 * ad7606c-16/18: phase delay from 0 µs to 255 µs in steps of 1 µs.
> +	 */
> +	*val2 = ret * st->chip_info->calib_phase_avail[1][1];
> +
> +	return 0;
> +}
> +
>  static int ad7606_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val,
> @@ -717,6 +773,22 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>  		pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
>  		*val = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
> cnvst_pwm_state.period);
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +		ret = ad7606_get_calib_offset(st, chan->scan_index, val);
> +		iio_device_release_direct(indio_dev);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_CALIBPHASE_DELAY:
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +		ret = ad7606_get_calib_phase(st, chan->scan_index, val,
> val2);
> +		iio_device_release_direct(indio_dev);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT_PLUS_NANO;
>  	}
>  	return -EINVAL;
>  }
> @@ -767,6 +839,64 @@ static int ad7606_write_os_hw(struct iio_dev *indio_dev,
> int val)
>  	return 0;
>  }
>  
> +static int ad7606_set_calib_offset(struct ad7606_state *st, int ch, int val)
> +{
> +	int start_val, step_val, stop_val;
> +
> +	start_val = st->chip_info->calib_offset_avail[0];
> +	step_val = st->chip_info->calib_offset_avail[1];
> +	stop_val = st->chip_info->calib_offset_avail[2];
> +
> +	if (val < start_val || val > stop_val)
> +		return -EINVAL;
> +
> +	val += start_val;

Shouldn't this be val -= start_val?

I also don't think we have any strict rules in the ABI for units for these kind
of interfaces so using "raw" values is easier. But FWIW, I think we could have
this in mv (would naturally depend on scale) 

- Nuno Sá