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..ec063dd4a67eb94610c41c473e2d8040c91974cf 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,
+};
+
+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;
+	val /= step_val;
+
+	return st->bops->reg_write(st, AD7606_CALIB_OFFSET(ch), val);
+}
+
+static int ad7606_set_calib_phase(struct ad7606_state *st, int ch, int val,
+				  int val2)
+{
+	int wreg, start_ns, step_ns, stop_ns;
+
+	if (val != 0)
+		return -EINVAL;
+
+	start_ns = st->chip_info->calib_phase_avail[0][1];
+	step_ns = st->chip_info->calib_phase_avail[1][1];
+	stop_ns = st->chip_info->calib_phase_avail[2][1];
+
+	/*
+	 * ad7606b: phase dielay 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.
+	 */
+	if (val2 < start_ns || val2 > stop_ns)
+			return -EINVAL;
+
+	wreg = val2 / step_ns;
+
+	return st->bops->reg_write(st, AD7606_CALIB_PHASE(ch), wreg);
+}
+
+static int ad7606_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan, long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_CALIBPHASE_DELAY:
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int ad7606_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int val,
@@ -820,6 +950,18 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 		if (val < 0 && val2 != 0)
 			return -EINVAL;
 		return ad7606_set_sampling_freq(st, val);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
+		ret = ad7606_set_calib_offset(st, chan->scan_index, val);
+		iio_device_release_direct(indio_dev);
+		return ret;
+	case IIO_CHAN_INFO_CALIBPHASE_DELAY:
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
+		ret = ad7606_set_calib_phase(st, chan->scan_index, val, val2);
+		iio_device_release_direct(indio_dev);
+		return ret;
 	default:
 		return -EINVAL;
 	}
@@ -998,6 +1140,14 @@ static int ad7606_read_avail(struct iio_dev *indio_dev,
 		*type = IIO_VAL_INT_PLUS_MICRO;
 
 		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_CALIBBIAS:
+		*vals = st->chip_info->calib_offset_avail;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_RANGE;
+	case IIO_CHAN_INFO_CALIBPHASE_DELAY:
+		*vals = (const int *)st->chip_info->calib_phase_avail;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		return IIO_AVAIL_RANGE;
 	}
 	return -EINVAL;
 }
@@ -1060,6 +1210,7 @@ static const struct iio_info ad7606_info_sw_mode = {
 	.read_raw = &ad7606_read_raw,
 	.write_raw = &ad7606_write_raw,
 	.read_avail = &ad7606_read_avail,
+	.write_raw_get_fmt = ad7606_write_raw_get_fmt,
 	.debugfs_reg_access = &ad7606_reg_access,
 	.validate_trigger = &ad7606_validate_trigger,
 	.update_scan_mode = &ad7606_update_scan_mode,
@@ -1252,6 +1403,15 @@ static int ad7606_probe_channels(struct iio_dev *indio_dev)
 			chan->info_mask_separate_available |=
 				BIT(IIO_CHAN_INFO_SCALE);
 
+			if (st->chip_info->calib_offset_avail) {
+				chan->info_mask_separate |=
+					BIT(IIO_CHAN_INFO_CALIBBIAS) |
+					BIT(IIO_CHAN_INFO_CALIBPHASE_DELAY);
+				chan->info_mask_separate_available |=
+					BIT(IIO_CHAN_INFO_CALIBBIAS) |
+					BIT(IIO_CHAN_INFO_CALIBPHASE_DELAY);
+			}
+
 			/*
 			 * All chips with software mode support oversampling,
 			 * so we skip the oversampling_available check. And the
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 89d49551eaf515bab9706c12bff056dfcb707b67..4c39de36154ffb80dfbb01bb4f812826bdc55967 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -40,6 +40,11 @@
 #define AD7606_RANGE_CH_ADDR(ch)	(0x03 + ((ch) >> 1))
 #define AD7606_OS_MODE			0x08
 
+#define AD7606_CALIB_GAIN(ch)		(0x09 + (ch))
+#define AD7606_CALIB_GAIN_MASK		GENMASK(5, 0)
+#define AD7606_CALIB_OFFSET(ch)		(0x11 + (ch))
+#define AD7606_CALIB_PHASE(ch)		(0x19 + (ch))
+
 struct ad7606_state;
 
 typedef int (*ad7606_scale_setup_cb_t)(struct iio_dev *indio_dev,
@@ -61,6 +66,8 @@ typedef int (*ad7606_sw_setup_cb_t)(struct iio_dev *indio_dev);
  * @init_delay_ms:	required delay in milliseconds for initialization
  *			after a restart
  * @offload_storagebits: storage bits used by the offload hw implementation
+ * @calib_offset_avail: pointer to offset calibration range/limits array
+ * @calib_phase_avail:  pointer to phase calibration range/limits array
  */
 struct ad7606_chip_info {
 	unsigned int			max_samplerate;
@@ -74,6 +81,8 @@ struct ad7606_chip_info {
 	bool				os_req_reset;
 	unsigned long			init_delay_ms;
 	u8				offload_storagebits;
+	const int			*calib_offset_avail;
+	const int			(*calib_phase_avail)[2];
 };
 
 /**

-- 
2.49.0

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á

On 4/30/25 10:36 AM, Nuno Sá wrote:
> 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á
> 

From testing, it seems to be working as expected for me, so I think this is
correct. The register value is not signed. 0x80 is no offset.

Also, I like having the scaling so that the units are the same LSB as the raw
value like it is now. It makes calibration easy since I can generate a constant
voltage and do a buffered read. Then I can take the average of all samples and
see how it compares to the expected value. Then take the difference and that is
the exact value to enter into the attribute. Millivolts would work to but that
requires applying the scale to the average of the raw values to get the number
that you would need to enter into the calibration attribute.

On 4/30/25 11:14 AM, David Lechner wrote:
> On 4/30/25 10:36 AM, Nuno Sá wrote:
>> On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>
>>>

...

>>> +
>>> +	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á
>>
> 
> From testing, it seems to be working as expected for me, so I think this is
> correct. The register value is not signed. 0x80 is no offset.
> 

Heh, you are actually quite right. Even though it working correctly, it is
because the value that gets written to the register is val & 0xFF, so adding
or subtracting here basically has the same effect. But subtracting is the more
logical way to do it. (I tested it that way too just to be 100% sure.)

On Wed, 2025-04-30 at 13:33 -0500, David Lechner wrote:
> On 4/30/25 11:14 AM, David Lechner wrote:
> > On 4/30/25 10:36 AM, Nuno Sá wrote:
> > > On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
> > > > From: Angelo Dureghello <adureghello@baylibre.com>
> > > > 
> > > > 
> 
> ...
> 
> > > > +
> > > > +	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á
> > > 
> > 
> > From testing, it seems to be working as expected for me, so I think this is
> > correct. The register value is not signed. 0x80 is no offset.
> > 
> 
> Heh, you are actually quite right. Even though it working correctly, it is
> because the value that gets written to the register is val & 0xFF, so adding
> or subtracting here basically has the same effect. But subtracting is the more
> logical way to do it. (I tested it that way too just to be 100% sure.)

Yeps, when testing it i realized that the current form just gives the correct
value in the 2 LSB so I assumed we were doing something to cast way the invalid
bits.

To be more pedantic, I think subtracting is the *correct* way :)

- Nuno Sá

On Tue, Apr 29, 2025 at 03:06:47PM +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

...

> +	if (val2 < start_ns || val2 > stop_ns)
> +			return -EINVAL;

Wrong indentation.

-- 
With Best Regards,
Andy Shevchenko

On 30.04.2025 15:50, Andy Shevchenko wrote:
> On Tue, Apr 29, 2025 at 03:06:47PM +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
> 
> ...
> 
> > +	if (val2 < start_ns || val2 > stop_ns)
> > +			return -EINVAL;
> 
> Wrong indentation.
>

code is correct, i checked it since also checkpatch claims for bad
indentation, but i just have 1 tab after the "if".

Quite strange, where could be the issue here ?
 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>