[PATCH 0/5] iio: adc: add ad7606 calibration support

[PATCH 0/5] iio: adc: add ad7606 calibration support

[Angelo Dureghello]

Add gain, offset and phase (as a delay) calibration support, for
ad7606b, ad7606c16 and ad7606c18.

Calibration is available for devices with software mode capability. 

Offset and phase calibration is configurable by sysfs attributes, while
gain calibration value in ohms must match the external RFilter value,
when an external RFilter is available, so implemented through a specific
devicetree "adi,rfilter-ohms" property.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
Angelo Dureghello (5):
      Documentation: ABI: IIO: add calibphase_delay documentation
      iio: core: add ADC phase calibration definition
      iio: adc: ad7606: add offset and phase calibration support
      dt-bindings: iio: adc: adi,ad7606: add gain calibration support
      iio: adc: ad7606: add gain calibration support

 Documentation/ABI/testing/sysfs-bus-iio            |  20 ++
 .../devicetree/bindings/iio/adc/adi,ad7606.yaml    |  14 ++
 drivers/iio/adc/ad7606.c                           | 213 +++++++++++++++++++++
 drivers/iio/adc/ad7606.h                           |  13 ++
 drivers/iio/industrialio-core.c                    |   1 +
 include/linux/iio/types.h                          |   1 +
 6 files changed, 262 insertions(+)
---
base-commit: e22e3d5089987cb4250801623026992b2ba4645d
change-id: 20250429-wip-bl-ad7606-calibration-20a396a60352

Best regards,
-- 
Angelo Dureghello <adureghello@baylibre.com>

[PATCH 1/5] Documentation: ABI: IIO: add calibphase_delay documentation

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add new IIO calibphase_delay documentation.

The delay suffix is added to specify that the phase, generally in
radiants, is for this case (needed from ad7606) in nanoseconds.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 33c09c4ac60a4feec82308461643134f5ba84b66..f233190d48a34882b7fed2d961141cc6bec3ddb2 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -559,6 +559,26 @@ Description:
 		- a small discrete set of values like "0 2 4 6 8"
 		- a range specified as "[min step max]"
 
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay
+KernelVersion:  6.16
+Contact:        linux-iio@vger.kernel.org
+Description:
+		Hardware applied calbiration phase (assumed to fix errors that are
+		introduced from external filters).
+		For the ad7606 ADC series, this value is intended as a time delay,
+		as an integer plus nanoseconds.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay_available
+KernelVersion:	6.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Available values of calibphase_delay. Maybe expressed as:
+
+		- a range specified as "[min step max]"
+
+		If shared across all channels, <type>_calibphase_delay_available
+		is used.
+
 What:		/sys/bus/iio/devices/iio:deviceX/in_accel_x_calibscale
 What:		/sys/bus/iio/devices/iio:deviceX/in_accel_y_calibscale
 What:		/sys/bus/iio/devices/iio:deviceX/in_accel_z_calibscale

-- 
2.49.0

[PATCH 2/5] iio: core: add ADC phase calibration definition

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

ADCs as ad7606 implement phase calibration.
Add such definition, needed for ad7606.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/industrialio-core.c | 1 +
 include/linux/iio/types.h       | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 178e99b111debc59a247fcc3a6037e429db3bebf..73ef9347d96b39c49e8a831576482071a95b5a41 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -188,6 +188,7 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_CALIBAMBIENT] = "calibambient",
 	[IIO_CHAN_INFO_ZEROPOINT] = "zeropoint",
 	[IIO_CHAN_INFO_TROUGH] = "trough_raw",
+	[IIO_CHAN_INFO_CALIBPHASE_DELAY] = "calibphase_delay",
 };
 /**
  * iio_device_id() - query the unique ID for the device
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index d89982c98368cf72c0fc30fa66ab001e48af4e8b..f341233613633830745e1899870377439e50d7f6 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -69,6 +69,7 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_CALIBAMBIENT,
 	IIO_CHAN_INFO_ZEROPOINT,
 	IIO_CHAN_INFO_TROUGH,
+	IIO_CHAN_INFO_CALIBPHASE_DELAY,
 };
 
 #endif /* _IIO_TYPES_H_ */

-- 
2.49.0

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

[Angelo Dureghello]

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

[PATCH 4/5] dt-bindings: iio: adc: adi,ad7606: add gain calibration support

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add gain calibration support by a per-channel resistor value.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index 29f12d650442b8ff2eb455306ce59a0e87867ddd..df30545fb52c89a814257443183303775a06a7f2 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -204,6 +204,15 @@ patternProperties:
           considered a bipolar differential channel. Otherwise it is bipolar
           single-ended.
 
+      adi,rfilter-ohms:
+        description:
+          For ADCs that supports gain calibration, this property must be set to
+          the value of the external RFilter resistor. Proper gain error
+          correction is applied based on this value.
+        default: 0
+        minimum: 0
+        maximum: 65536
+
     required:
       - reg
       - bipolar
@@ -271,6 +280,10 @@ allOf:
     then:
       properties:
         adi,sw-mode: false
+      patternProperties:
+        "^channel@[0-9a-f]+$":
+          properties:
+            adi,rfilter-ohms: false
     else:
       properties:
         pwms:
@@ -398,6 +411,7 @@ examples:
                 reg = <8>;
                 diff-channels = <8 8>;
                 bipolar;
+                adi,rfilter-ohms = <2048>;
             };
 
         };

-- 
2.49.0

[PATCH 5/5] iio: adc: ad7606: add gain calibration support

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add gain calibration support, using resistor values set on devicetree,
values to be set accordingly with ADC external RFilter, as explained in
the ad7606c-16 datasheet, rev0, page 37.

Usage example in the fdt yaml documentation.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/iio/adc/ad7606.h |  4 ++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index ec063dd4a67eb94610c41c473e2d8040c91974cf..1ebc7080d3d153a2ba02bc5c126ef9957dc782ab 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -33,6 +33,10 @@
 
 #include "ad7606.h"
 
+#define AD7606_CALIB_GAIN_MIN	0
+#define AD7606_CALIB_GAIN_STEP	1024
+#define AD7606_CALIB_GAIN_MAX	65536
+
 /*
  * Scales are computed as 5000/32768 and 10000/32768 respectively,
  * so that when applied to the raw values they provide mV values.
@@ -125,6 +129,8 @@ static int ad7609_chan_scale_setup(struct iio_dev *indio_dev,
 				   struct iio_chan_spec *chan);
 static int ad7616_sw_mode_setup(struct iio_dev *indio_dev);
 static int ad7606b_sw_mode_setup(struct iio_dev *indio_dev);
+static int ad7606_chan_calib_gain_setup(struct iio_dev *indio_dev,
+					struct iio_chan_spec *chan);
 
 const struct ad7606_chip_info ad7605_4_info = {
 	.max_samplerate = 300 * KILO,
@@ -180,6 +186,7 @@ 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_gain_setup_cb = ad7606_chan_calib_gain_setup,
 	.calib_offset_avail = ad7606_calib_offset_avail,
 	.calib_phase_avail = ad7606b_calib_phase_avail,
 };
@@ -195,6 +202,7 @@ 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_gain_setup_cb = ad7606_chan_calib_gain_setup,
 	.calib_offset_avail = ad7606_calib_offset_avail,
 	.calib_phase_avail = ad7606c_calib_phase_avail,
 };
@@ -246,6 +254,7 @@ 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_gain_setup_cb = ad7606_chan_calib_gain_setup,
 	.calib_offset_avail = ad7606c_18bit_calib_offset_avail,
 	.calib_phase_avail = ad7606c_calib_phase_avail,
 };
@@ -357,6 +366,46 @@ static int ad7606_get_chan_config(struct iio_dev *indio_dev, int ch,
 	return 0;
 }
 
+static int ad7606_chan_calib_gain_setup(struct iio_dev *indio_dev,
+					struct iio_chan_spec *chan)
+{
+	struct ad7606_state *st = iio_priv(indio_dev);
+	unsigned int num_channels = st->chip_info->num_adc_channels;
+	struct device *dev = st->dev;
+	int ret;
+
+	device_for_each_child_node_scoped(dev, child) {
+		int reg, r_gain;
+
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret)
+			return ret;
+
+		/* channel number (here) is from 1 to num_channels */
+		if (reg < 1 || reg > num_channels) {
+			dev_warn(dev, "invalid ch number (ignoring): %d\n", reg);
+			continue;
+		}
+
+		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
+					       &r_gain);
+		if (ret)
+			return ret;
+
+		if (r_gain < AD7606_CALIB_GAIN_MIN ||
+		    r_gain > AD7606_CALIB_GAIN_MAX)
+			return -EINVAL;
+
+		/* Chan reg is 1-based index. */
+		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
+					  r_gain / AD7606_CALIB_GAIN_STEP);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int ad7606c_18bit_chan_scale_setup(struct iio_dev *indio_dev,
 					  struct iio_chan_spec *chan)
 {
@@ -1410,6 +1459,10 @@ static int ad7606_probe_channels(struct iio_dev *indio_dev)
 				chan->info_mask_separate_available |=
 					BIT(IIO_CHAN_INFO_CALIBBIAS) |
 					BIT(IIO_CHAN_INFO_CALIBPHASE_DELAY);
+				ret = st->chip_info->calib_gain_setup_cb(
+					indio_dev, chan);
+				if (ret)
+					return ret;
 			}
 
 			/*
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 4c39de36154ffb80dfbb01bb4f812826bdc55967..e9a59d2afafd43e66137599dbd8220cd6b641e61 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -50,6 +50,8 @@ struct ad7606_state;
 typedef int (*ad7606_scale_setup_cb_t)(struct iio_dev *indio_dev,
 				       struct iio_chan_spec *chan);
 typedef int (*ad7606_sw_setup_cb_t)(struct iio_dev *indio_dev);
+typedef int (*ad7606_calib_gain_setup_cb_t)(struct iio_dev *indio_dev,
+					    struct iio_chan_spec *chan);
 
 /**
  * struct ad7606_chip_info - chip specific information
@@ -66,6 +68,7 @@ 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_gain_setup_cb: callback to setup of gain calibration for each channel
  * @calib_offset_avail: pointer to offset calibration range/limits array
  * @calib_phase_avail:  pointer to phase calibration range/limits array
  */
@@ -81,6 +84,7 @@ struct ad7606_chip_info {
 	bool				os_req_reset;
 	unsigned long			init_delay_ms;
 	u8				offload_storagebits;
+	ad7606_calib_gain_setup_cb_t	calib_gain_setup_cb;
 	const int			*calib_offset_avail;
 	const int			(*calib_phase_avail)[2];
 };

-- 
2.49.0

Re: [PATCH 4/5] dt-bindings: iio: adc: adi,ad7606: add gain calibration support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

On Tue, Apr 29, 2025 at 03:06:48PM +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Add gain calibration support by a per-channel resistor value.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 4/5] dt-bindings: iio: adc: adi,ad7606: add gain calibration support

[David Lechner]

On 4/29/25 8:06 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Add gain calibration support by a per-channel resistor value.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index 29f12d650442b8ff2eb455306ce59a0e87867ddd..df30545fb52c89a814257443183303775a06a7f2 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -204,6 +204,15 @@ patternProperties:
>            considered a bipolar differential channel. Otherwise it is bipolar
>            single-ended.
>  
> +      adi,rfilter-ohms:
> +        description:
> +          For ADCs that supports gain calibration, this property must be set to
> +          the value of the external RFilter resistor. Proper gain error
> +          correction is applied based on this value.
> +        default: 0
> +        minimum: 0
> +        maximum: 65536
> +
>      required:
>        - reg
>        - bipolar
> @@ -271,6 +280,10 @@ allOf:
>      then:
>        properties:
>          adi,sw-mode: false
> +      patternProperties:
> +        "^channel@[0-9a-f]+$":
> +          properties:
> +            adi,rfilter-ohms: false

I think this is in the wrong place. It would allow this property on ad7616, but
ad7616 does not have this feature.


>      else:
>        properties:
>          pwms:
> @@ -398,6 +411,7 @@ examples:
>                  reg = <8>;
>                  diff-channels = <8 8>;
>                  bipolar;
> +                adi,rfilter-ohms = <2048>;
>              };
>  
>          };
> 

Re: [PATCH 4/5] dt-bindings: iio: adc: adi,ad7606: add gain calibration support

[Angelo Dureghello]

On 29.04.2025 10:26, David Lechner wrote:
> On 4/29/25 8:06 AM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > Add gain calibration support by a per-channel resistor value.
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> >  Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > index 29f12d650442b8ff2eb455306ce59a0e87867ddd..df30545fb52c89a814257443183303775a06a7f2 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > @@ -204,6 +204,15 @@ patternProperties:
> >            considered a bipolar differential channel. Otherwise it is bipolar
> >            single-ended.
> >  
> > +      adi,rfilter-ohms:
> > +        description:
> > +          For ADCs that supports gain calibration, this property must be set to
> > +          the value of the external RFilter resistor. Proper gain error
> > +          correction is applied based on this value.
> > +        default: 0
> > +        minimum: 0
> > +        maximum: 65536
> > +
> >      required:
> >        - reg
> >        - bipolar
> > @@ -271,6 +280,10 @@ allOf:
> >      then:
> >        properties:
> >          adi,sw-mode: false
> > +      patternProperties:
> > +        "^channel@[0-9a-f]+$":
> > +          properties:
> > +            adi,rfilter-ohms: false
> 
> I think this is in the wrong place. It would allow this property on ad7616, but
> ad7616 does not have this feature.
>

Looks like it cannot work on ad7616 due to

  - if:
      not:
        properties:
          compatible:
            enum:
              - adi,ad7606c-16
              - adi,ad7606c-18
    then:
      patternProperties:
        "^channel@[1-8]$": false


But maybe an additional patch should be added to add also adi,ad7606b
to the above enum.

Regards,
angelo


 
> 
> >      else:
> >        properties:
> >          pwms:
> > @@ -398,6 +411,7 @@ examples:
> >                  reg = <8>;
> >                  diff-channels = <8 8>;
> >                  bipolar;
> > +                adi,rfilter-ohms = <2048>;
> >              };
> >  
> >          };
> > 
> 

Re: [PATCH 5/5] iio: adc: ad7606: add gain calibration support

[Andy Shevchenko]

On Tue, Apr 29, 2025 at 4:08 PM Angelo Dureghello
<adureghello@baylibre.com> wrote:
>
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> Add gain calibration support, using resistor values set on devicetree,
> values to be set accordingly with ADC external RFilter, as explained in
> the ad7606c-16 datasheet, rev0, page 37.
>
> Usage example in the fdt yaml documentation.

...

> +#define AD7606_CALIB_GAIN_MIN  0
> +#define AD7606_CALIB_GAIN_STEP 1024
> +#define AD7606_CALIB_GAIN_MAX  65536

Are those values in decimal in the datasheet?
It looks to me something like

_MAX = (64 * _STEP)

but is it for real? Usually values are limited by the amount of bits
in the HW, here it spans over 65 steps, i.e. 7 bits would be needed...
Confusing.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] iio: adc: ad7606: add gain calibration support

[David Lechner]

On 4/29/25 8:06 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 

...

> +static int ad7606_chan_calib_gain_setup(struct iio_dev *indio_dev,
> +					struct iio_chan_spec *chan)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +	unsigned int num_channels = st->chip_info->num_adc_channels;
> +	struct device *dev = st->dev;
> +	int ret;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		int reg, r_gain;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			return ret;
> +
> +		/* channel number (here) is from 1 to num_channels */
> +		if (reg < 1 || reg > num_channels) {
> +			dev_warn(dev, "invalid ch number (ignoring): %d\n", reg);
> +			continue;
> +		}
> +
> +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> +					       &r_gain);

Instead of...

> +		if (ret)
> +			return ret;

... we need:

		if (ret == -EINVAL)
			r_gain = 0;
		else if (ret)
			return ret;

Otherwise driver fails to probe if adi,rfilter-ohms is missing.

> +
> +		if (r_gain < AD7606_CALIB_GAIN_MIN ||
> +		    r_gain > AD7606_CALIB_GAIN_MAX)
> +			return -EINVAL;
> +

Also, return dev_err_probe() on the returns above would have made debugging
easier.

> +		/* Chan reg is 1-based index. */
> +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
> +					  r_gain / AD7606_CALIB_GAIN_STEP);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +

Re: [PATCH 1/5] Documentation: ABI: IIO: add calibphase_delay documentation

[Nuno Sá]

On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Add new IIO calibphase_delay documentation.
> 
> The delay suffix is added to specify that the phase, generally in
> radiants, is for this case (needed from ad7606) in nanoseconds.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
> b/Documentation/ABI/testing/sysfs-bus-iio
> index
> 33c09c4ac60a4feec82308461643134f5ba84b66..f233190d48a34882b7fed2d961141cc6bec3ddb2
> 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -559,6 +559,26 @@ Description:
>  		- a small discrete set of values like "0 2 4 6 8"
>  		- a range specified as "[min step max]"
>  
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay

Not sure if I'm too convinced on the _delay suffix

> +KernelVersion:  6.16
> +Contact:        linux-iio@vger.kernel.org
> +Description:
> +		Hardware applied calbiration phase (assumed to fix errors that are
> +		introduced from external filters).
> +		For the ad7606 ADC series, this value is intended as a time delay,
> +		as an integer plus nanoseconds.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay_avai
> lable
> +KernelVersion:	6.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Available values of calibphase_delay. Maybe expressed as:
> +
> +		- a range specified as "[min step max]"
> +
> +		If shared across all channels, <type>_calibphase_delay_available
> +		is used.
> +
>  What:		/sys/bus/iio/devices/iio:deviceX/in_accel_x_calibscale
>  What:		/sys/bus/iio/devices/iio:deviceX/in_accel_y_calibscale
>  What:		/sys/bus/iio/devices/iio:deviceX/in_accel_z_calibscale
> 

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

[Andy Shevchenko]

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

Re: [PATCH 1/5] Documentation: ABI: IIO: add calibphase_delay documentation

[David Lechner]

On 4/30/25 12:40 AM, Nuno Sá wrote:
> On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
>> From: Angelo Dureghello <adureghello@baylibre.com>
>>
>> Add new IIO calibphase_delay documentation.
>>
>> The delay suffix is added to specify that the phase, generally in
>> radiants, is for this case (needed from ad7606) in nanoseconds.
>>
>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
>> ---
>>  Documentation/ABI/testing/sysfs-bus-iio | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>> b/Documentation/ABI/testing/sysfs-bus-iio
>> index
>> 33c09c4ac60a4feec82308461643134f5ba84b66..f233190d48a34882b7fed2d961141cc6bec3ddb2
>> 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -559,6 +559,26 @@ Description:
>>  		- a small discrete set of values like "0 2 4 6 8"
>>  		- a range specified as "[min step max]"
>>  
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay
> 
> Not sure if I'm too convinced on the _delay suffix
> 
Phase is measured in radians, not seconds, so it seems wrong to use it here.

https://en.wikipedia.org/wiki/Phase_(waves)

And the delay here is with respect to individual samples in a simultaneous
conversion without regard for a sampling frequency, so I don't see how we could
convert the time to radians in any meaningful way.

Re: [PATCH 1/5] Documentation: ABI: IIO: add calibphase_delay documentation

[Andy Shevchenko]

On Wed, Apr 30, 2025 at 09:21:28AM -0500, David Lechner wrote:
> On 4/30/25 12:40 AM, Nuno Sá wrote:
> > On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
> >> From: Angelo Dureghello <adureghello@baylibre.com>
> >>
> >> Add new IIO calibphase_delay documentation.
> >>
> >> The delay suffix is added to specify that the phase, generally in
> >> radiants, is for this case (needed from ad7606) in nanoseconds.

...

> >> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay
> > 
> > Not sure if I'm too convinced on the _delay suffix
> > 
> Phase is measured in radians, not seconds, so it seems wrong to use it here.
> 
> https://en.wikipedia.org/wiki/Phase_(waves)
> 
> And the delay here is with respect to individual samples in a simultaneous
> conversion without regard for a sampling frequency, so I don't see how we could
> convert the time to radians in any meaningful way.

And how this delay is aplicable to the phase in the hardware? Sounds to me that
HW has some meaningful way of such a conversion?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] Documentation: ABI: IIO: add calibphase_delay documentation

[David Lechner]

On 4/30/25 9:45 AM, Andy Shevchenko wrote:
> On Wed, Apr 30, 2025 at 09:21:28AM -0500, David Lechner wrote:
>> On 4/30/25 12:40 AM, Nuno Sá wrote:
>>> On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
>>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>>
>>>> Add new IIO calibphase_delay documentation.
>>>>
>>>> The delay suffix is added to specify that the phase, generally in
>>>> radiants, is for this case (needed from ad7606) in nanoseconds.
> 
> ...
> 
>>>> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay
>>>
>>> Not sure if I'm too convinced on the _delay suffix
>>>
>> Phase is measured in radians, not seconds, so it seems wrong to use it here.
>>
>> https://en.wikipedia.org/wiki/Phase_(waves)
>>
>> And the delay here is with respect to individual samples in a simultaneous
>> conversion without regard for a sampling frequency, so I don't see how we could
>> convert the time to radians in any meaningful way.
> 
> And how this delay is aplicable to the phase in the hardware? Sounds to me that
> HW has some meaningful way of such a conversion?
> 

It is a calibration to account for a phase difference between two input signals.
This is a simultaneous sampling ADC, so all channels normally sample at exactly
the same time. This phase delay calibration factor can introduce a small delay
on an individual channel so that it starts it's conversion some microseconds
after the others.

There is a nice diagram here:

https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf#%5B%7B%22num%22%3A113%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C34%2C594%2C0%5D

To convert the phase delay to a phase angle and back would require also knowing
the frequency of the input voltage signals.

Re: [PATCH 1/5] Documentation: ABI: IIO: add calibphase_delay documentation

[David Lechner]

On 4/30/25 9:56 AM, David Lechner wrote:
> On 4/30/25 9:45 AM, Andy Shevchenko wrote:
>> On Wed, Apr 30, 2025 at 09:21:28AM -0500, David Lechner wrote:
>>> On 4/30/25 12:40 AM, Nuno Sá wrote:
>>>> On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
>>>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>>>
>>>>> Add new IIO calibphase_delay documentation.
>>>>>
>>>>> The delay suffix is added to specify that the phase, generally in
>>>>> radiants, is for this case (needed from ad7606) in nanoseconds.
>>
>> ...
>>
>>>>> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay
>>>>
>>>> Not sure if I'm too convinced on the _delay suffix
>>>>
>>> Phase is measured in radians, not seconds, so it seems wrong to use it here.
>>>
>>> https://en.wikipedia.org/wiki/Phase_(waves)
>>>
>>> And the delay here is with respect to individual samples in a simultaneous
>>> conversion without regard for a sampling frequency, so I don't see how we could
>>> convert the time to radians in any meaningful way.
>>
>> And how this delay is aplicable to the phase in the hardware? Sounds to me that
>> HW has some meaningful way of such a conversion?
>>
> 
> It is a calibration to account for a phase difference between two input signals.
> This is a simultaneous sampling ADC, so all channels normally sample at exactly
> the same time. This phase delay calibration factor can introduce a small delay
> on an individual channel so that it starts it's conversion some microseconds
> after the others.
> 
> There is a nice diagram here:
> 
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf#%5B%7B%22num%22%3A113%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C34%2C594%2C0%5D
> 
> To convert the phase delay to a phase angle and back would require also knowing
> the frequency of the input voltage signals.

Maybe calling it "conversion delay" would make more sense? Since the phase part
of it is really referring to the application rather than to what we are actually
adjusting.

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

[Nuno Sá]

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á

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

[David Lechner]

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.

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

[David Lechner]

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.)

Re: [PATCH 1/5] Documentation: ABI: IIO: add calibphase_delay documentation

[Angelo Dureghello]

On 30.04.2025 10:04, David Lechner wrote:
> On 4/30/25 9:56 AM, David Lechner wrote:
> > On 4/30/25 9:45 AM, Andy Shevchenko wrote:
> >> On Wed, Apr 30, 2025 at 09:21:28AM -0500, David Lechner wrote:
> >>> On 4/30/25 12:40 AM, Nuno Sá wrote:
> >>>> On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
> >>>>> From: Angelo Dureghello <adureghello@baylibre.com>
> >>>>>
> >>>>> Add new IIO calibphase_delay documentation.
> >>>>>
> >>>>> The delay suffix is added to specify that the phase, generally in
> >>>>> radiants, is for this case (needed from ad7606) in nanoseconds.
> >>
> >> ...
> >>
> >>>>> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay
> >>>>
> >>>> Not sure if I'm too convinced on the _delay suffix
> >>>>
> >>> Phase is measured in radians, not seconds, so it seems wrong to use it here.
> >>>
> >>> https://en.wikipedia.org/wiki/Phase_(waves)
> >>>
> >>> And the delay here is with respect to individual samples in a simultaneous
> >>> conversion without regard for a sampling frequency, so I don't see how we could
> >>> convert the time to radians in any meaningful way.
> >>
> >> And how this delay is aplicable to the phase in the hardware? Sounds to me that
> >> HW has some meaningful way of such a conversion?
> >>
> > 
> > It is a calibration to account for a phase difference between two input signals.
> > This is a simultaneous sampling ADC, so all channels normally sample at exactly
> > the same time. This phase delay calibration factor can introduce a small delay
> > on an individual channel so that it starts it's conversion some microseconds
> > after the others.
> > 
> > There is a nice diagram here:
> > 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf#%5B%7B%22num%22%3A113%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C34%2C594%2C0%5D
> > 
> > To convert the phase delay to a phase angle and back would require also knowing
> > the frequency of the input voltage signals.
> 
> Maybe calling it "conversion delay" would make more sense? Since the phase part
> of it is really referring to the application rather than to what we are actually
> adjusting.

Are there examples of a phase calibration in iio ? Becouse apply a radians 
calibration seems complicated and maybe non approrpiate for non-periodic 
signals as often used in real world applications.

So another viable idea could be to use a IIO_CHAN_INFO_CALIBDELAY instead.

Regards,
angelo

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

[Angelo Dureghello]

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

Re: [PATCH 5/5] iio: adc: ad7606: add gain calibration support

[Angelo Dureghello]

On 29.04.2025 17:46, David Lechner wrote:
> On 4/29/25 8:06 AM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> 
> ...
> 
> > +static int ad7606_chan_calib_gain_setup(struct iio_dev *indio_dev,
> > +					struct iio_chan_spec *chan)
> > +{
> > +	struct ad7606_state *st = iio_priv(indio_dev);
> > +	unsigned int num_channels = st->chip_info->num_adc_channels;
> > +	struct device *dev = st->dev;
> > +	int ret;
> > +
> > +	device_for_each_child_node_scoped(dev, child) {
> > +		int reg, r_gain;
> > +
> > +		ret = fwnode_property_read_u32(child, "reg", &reg);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* channel number (here) is from 1 to num_channels */
> > +		if (reg < 1 || reg > num_channels) {
> > +			dev_warn(dev, "invalid ch number (ignoring): %d\n", reg);
> > +			continue;
> > +		}
> > +
> > +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> > +					       &r_gain);
> 
> Instead of...
> 
> > +		if (ret)
> > +			return ret;
> 
> ... we need:
> 
> 		if (ret == -EINVAL)
> 			r_gain = 0;
> 		else if (ret)
> 			return ret;
> 
> Otherwise driver fails to probe if adi,rfilter-ohms is missing.
>

Correct, i changed this before sending and could not catch it.
But not totally sure of applying a 0.
We are here after chip reset. So conceptually, would not apply any default,
ince it is already set after reset. What about:

		if (ret == -EINVAL)
			contnue;
		else if (ret)
			return ret;
 
> > +
> > +		if (r_gain < AD7606_CALIB_GAIN_MIN ||
> > +		    r_gain > AD7606_CALIB_GAIN_MAX)
> > +			return -EINVAL;
> > +
> 
> Also, return dev_err_probe() on the returns above would have made debugging
> easier.
> 
ack

> > +		/* Chan reg is 1-based index. */
> > +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
> > +					  r_gain / AD7606_CALIB_GAIN_STEP);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +

Re: [PATCH 5/5] iio: adc: ad7606: add gain calibration support

[Angelo Dureghello]

On 30.04.2025 01:34, Andy Shevchenko wrote:
> On Tue, Apr 29, 2025 at 4:08 PM Angelo Dureghello
> <adureghello@baylibre.com> wrote:
> >
> > From: Angelo Dureghello <adureghello@baylibre.com>
> >
> > Add gain calibration support, using resistor values set on devicetree,
> > values to be set accordingly with ADC external RFilter, as explained in
> > the ad7606c-16 datasheet, rev0, page 37.
> >
> > Usage example in the fdt yaml documentation.
> 
> ...
> 
> > +#define AD7606_CALIB_GAIN_MIN  0
> > +#define AD7606_CALIB_GAIN_STEP 1024
> > +#define AD7606_CALIB_GAIN_MAX  65536
>
Hi Andy,
 
> Are those values in decimal in the datasheet?
> It looks to me something like
> 
> _MAX = (64 * _STEP)
> 
> but is it for real? Usually values are limited by the amount of bits
> in the HW, here it spans over 65 steps, i.e. 7 bits would be needed...
> Confusing.
>

true, thanks,
there must be a typo in the datasheet that says 0 to 65536 with a
step of 1024 with 6 bits. Only 0 to 63 are possbile here.

step 0 = 0
step 63 = 64512
 
Will fix that.

Regards,
angelo

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 5/5] iio: adc: ad7606: add gain calibration support

[David Lechner]

On 5/1/25 8:35 AM, Angelo Dureghello wrote:
> On 29.04.2025 17:46, David Lechner wrote:
>> On 4/29/25 8:06 AM, Angelo Dureghello wrote:
>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>
>>
>> ...
>>
>>> +static int ad7606_chan_calib_gain_setup(struct iio_dev *indio_dev,
>>> +					struct iio_chan_spec *chan)
>>> +{
>>> +	struct ad7606_state *st = iio_priv(indio_dev);
>>> +	unsigned int num_channels = st->chip_info->num_adc_channels;
>>> +	struct device *dev = st->dev;
>>> +	int ret;
>>> +
>>> +	device_for_each_child_node_scoped(dev, child) {
>>> +		int reg, r_gain;
>>> +
>>> +		ret = fwnode_property_read_u32(child, "reg", &reg);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		/* channel number (here) is from 1 to num_channels */
>>> +		if (reg < 1 || reg > num_channels) {
>>> +			dev_warn(dev, "invalid ch number (ignoring): %d\n", reg);
>>> +			continue;
>>> +		}
>>> +
>>> +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
>>> +					       &r_gain);
>>
>> Instead of...
>>
>>> +		if (ret)
>>> +			return ret;
>>
>> ... we need:
>>
>> 		if (ret == -EINVAL)
>> 			r_gain = 0;
>> 		else if (ret)
>> 			return ret;
>>
>> Otherwise driver fails to probe if adi,rfilter-ohms is missing.
>>
> 
> Correct, i changed this before sending and could not catch it.
> But not totally sure of applying a 0.
> We are here after chip reset. So conceptually, would not apply any default,
> ince it is already set after reset. What about:
> 
> 		if (ret == -EINVAL)
> 			contnue;
> 		else if (ret)
> 			return ret;

Sure. A comment could help here and the continue makes else not needed.



 		if (ret == -EINVAL)
			/* Keep the default register value. */
 			contnue;
 		if (ret)
 			return ret;

>  
>>> +
>>> +		if (r_gain < AD7606_CALIB_GAIN_MIN ||
>>> +		    r_gain > AD7606_CALIB_GAIN_MAX)
>>> +			return -EINVAL;
>>> +
>>
>> Also, return dev_err_probe() on the returns above would have made debugging
>> easier.
>>
> ack
> 
>>> +		/* Chan reg is 1-based index. */
>>> +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
>>> +					  r_gain / AD7606_CALIB_GAIN_STEP);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +

Re: [PATCH 1/5] Documentation: ABI: IIO: add calibphase_delay documentation

[David Lechner]

On 5/1/25 7:33 AM, Angelo Dureghello wrote:
> On 30.04.2025 10:04, David Lechner wrote:
>> On 4/30/25 9:56 AM, David Lechner wrote:
>>> On 4/30/25 9:45 AM, Andy Shevchenko wrote:
>>>> On Wed, Apr 30, 2025 at 09:21:28AM -0500, David Lechner wrote:
>>>>> On 4/30/25 12:40 AM, Nuno Sá wrote:
>>>>>> On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:
>>>>>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>>>>>
>>>>>>> Add new IIO calibphase_delay documentation.
>>>>>>>
>>>>>>> The delay suffix is added to specify that the phase, generally in
>>>>>>> radiants, is for this case (needed from ad7606) in nanoseconds.
>>>>
>>>> ...
>>>>
>>>>>>> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay
>>>>>>
>>>>>> Not sure if I'm too convinced on the _delay suffix
>>>>>>
>>>>> Phase is measured in radians, not seconds, so it seems wrong to use it here.
>>>>>
>>>>> https://en.wikipedia.org/wiki/Phase_(waves)
>>>>>
>>>>> And the delay here is with respect to individual samples in a simultaneous
>>>>> conversion without regard for a sampling frequency, so I don't see how we could
>>>>> convert the time to radians in any meaningful way.
>>>>
>>>> And how this delay is aplicable to the phase in the hardware? Sounds to me that
>>>> HW has some meaningful way of such a conversion?
>>>>
>>>
>>> It is a calibration to account for a phase difference between two input signals.
>>> This is a simultaneous sampling ADC, so all channels normally sample at exactly
>>> the same time. This phase delay calibration factor can introduce a small delay
>>> on an individual channel so that it starts it's conversion some microseconds
>>> after the others.
>>>
>>> There is a nice diagram here:
>>>
>>> https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf#%5B%7B%22num%22%3A113%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C34%2C594%2C0%5D
>>>
>>> To convert the phase delay to a phase angle and back would require also knowing
>>> the frequency of the input voltage signals.
>>
>> Maybe calling it "conversion delay" would make more sense? Since the phase part
>> of it is really referring to the application rather than to what we are actually
>> adjusting.
> 
> Are there examples of a phase calibration in iio ? Becouse apply a radians 
> calibration seems complicated and maybe non approrpiate for non-periodic 
> signals as often used in real world applications.
> 
> So another viable idea could be to use a IIO_CHAN_INFO_CALIBDELAY instead.
> 
> Regards,
> angelo

I was looking at the datasheet on another ADC that popped up on the mailing list
today. https://www.ti.com/product/ADS1262

It has a "conversion delay" register that does basically the same thing. So I'm
liking that name even more now. Just calling it "delay" seems a bit too vague.
We could make it IIO_CHAN_INFO_CALIBCONV_DELAY to try to keep it shorter.

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

[Nuno Sá]

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á

Re: [PATCH 1/5] Documentation: ABI: IIO: add calibphase_delay documentation

[Jonathan Cameron]

On Thu, 1 May 2025 09:44:51 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 5/1/25 7:33 AM, Angelo Dureghello wrote:
> > On 30.04.2025 10:04, David Lechner wrote:  
> >> On 4/30/25 9:56 AM, David Lechner wrote:  
> >>> On 4/30/25 9:45 AM, Andy Shevchenko wrote:  
> >>>> On Wed, Apr 30, 2025 at 09:21:28AM -0500, David Lechner wrote:  
> >>>>> On 4/30/25 12:40 AM, Nuno Sá wrote:  
> >>>>>> On Tue, 2025-04-29 at 15:06 +0200, Angelo Dureghello wrote:  
> >>>>>>> From: Angelo Dureghello <adureghello@baylibre.com>
> >>>>>>>
> >>>>>>> Add new IIO calibphase_delay documentation.
> >>>>>>>
> >>>>>>> The delay suffix is added to specify that the phase, generally in
> >>>>>>> radiants, is for this case (needed from ad7606) in nanoseconds.  
> >>>>
> >>>> ...
> >>>>  
> >>>>>>> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_calibphase_delay  
> >>>>>>
> >>>>>> Not sure if I'm too convinced on the _delay suffix
> >>>>>>  
> >>>>> Phase is measured in radians, not seconds, so it seems wrong to use it here.
> >>>>>
> >>>>> https://en.wikipedia.org/wiki/Phase_(waves)
> >>>>>
> >>>>> And the delay here is with respect to individual samples in a simultaneous
> >>>>> conversion without regard for a sampling frequency, so I don't see how we could
> >>>>> convert the time to radians in any meaningful way.  
> >>>>
> >>>> And how this delay is aplicable to the phase in the hardware? Sounds to me that
> >>>> HW has some meaningful way of such a conversion?
> >>>>  
> >>>
> >>> It is a calibration to account for a phase difference between two input signals.
> >>> This is a simultaneous sampling ADC, so all channels normally sample at exactly
> >>> the same time. This phase delay calibration factor can introduce a small delay
> >>> on an individual channel so that it starts it's conversion some microseconds
> >>> after the others.
> >>>
> >>> There is a nice diagram here:
> >>>
> >>> https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf#%5B%7B%22num%22%3A113%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C34%2C594%2C0%5D
> >>>
> >>> To convert the phase delay to a phase angle and back would require also knowing
> >>> the frequency of the input voltage signals.  
> >>
> >> Maybe calling it "conversion delay" would make more sense? Since the phase part
> >> of it is really referring to the application rather than to what we are actually
> >> adjusting.  
> > 
> > Are there examples of a phase calibration in iio ? Becouse apply a radians 
> > calibration seems complicated and maybe non approrpiate for non-periodic 
> > signals as often used in real world applications.
> > 
> > So another viable idea could be to use a IIO_CHAN_INFO_CALIBDELAY instead.
> > 
> > Regards,
> > angelo  
> 
> I was looking at the datasheet on another ADC that popped up on the mailing list
> today. https://www.ti.com/product/ADS1262
> 
> It has a "conversion delay" register that does basically the same thing. So I'm
> liking that name even more now. Just calling it "delay" seems a bit too vague.
> We could make it IIO_CHAN_INFO_CALIBCONV_DELAY to try to keep it shorter.

This is wondering into a long term gap in IIO ABI.  Even if we ignore this particular
usecase, it could potentially be useful to indicate the timing offsets between the
sampling of specific channels.  In simultaneous sampling case we'd normally assume
0 (subject to tweaks like this one) whereas in a sequencer type situation it can get
complex.  IIRC there are sequencers that allow insertions of extra delays as well
as the simpler ones where it is just dependent on the previous channel sampling times.
What you have here is a relatively simple time delay control but I'd like
to cover the full gamut of things we might see.

To me it's really not got much to do with calibration so I'd drop the calib bit.
Define a baseline for all channels (which should probably be an arbitrary period
after trigger as measuring trigger to first sample gets complicated in some devices.)

Anyhow, I'll comment on v2 and point back at this.