[v4] iio: adc: add ad7606 calibration support

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

Posted by Angelo Dureghello 9 months, 1 week ago

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.

Tested-by: David Lechner <dlechner@baylibre.com>
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/iio/adc/ad7606.h |  3 +++
 2 files changed, 64 insertions(+)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index a986eb1284106da4980ac36cb0b5990e4e3bd948..be86e14ba85d07398e870ad680764958aa6ef471 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	(63 * AD7606_CALIB_GAIN_STEP)
+
 /*
  * 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,7 @@ 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);
 
 const struct ad7606_chip_info ad7605_4_info = {
 	.max_samplerate = 300 * KILO,
@@ -180,6 +185,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 +201,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 +253,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 +365,52 @@ 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 ad7606_state *st = iio_priv(indio_dev);
+	unsigned int num_channels = st->chip_info->num_adc_channels;
+	struct device *dev = st->dev;
+	int ret;
+
+	/*
+	 * This function is called once, and parses all the channel nodes,
+	 * so continuing on next channel node on errors, informing of them.
+	 */
+	device_for_each_child_node_scoped(dev, child) {
+		u32 reg, r_gain;
+
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret)
+			continue;
+
+		/* Chan reg is a 1-based index. */
+		if (reg < 1 || reg > num_channels) {
+			dev_warn(dev, "wrong ch number (ignoring): %d\n", reg);
+			continue;
+		}
+
+		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
+					       &r_gain);
+		if (ret)
+			/* Keep the default register value. */
+			continue;
+
+		if (r_gain > AD7606_CALIB_GAIN_MAX) {
+			dev_warn(dev, "wrong gain calibration value");
+			continue;
+		}
+
+		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
+			DIV_ROUND_CLOSEST(r_gain, AD7606_CALIB_GAIN_STEP));
+		if (ret) {
+			dev_warn(dev, "error writing r_gain");
+			continue;
+		}
+	}
+
+	return 0;
+}
+
 static int ad7606c_18bit_chan_scale_setup(struct iio_dev *indio_dev,
 					  struct iio_chan_spec *chan)
 {
@@ -1448,6 +1502,13 @@ static int ad7606_probe_channels(struct iio_dev *indio_dev)
 	if (slow_bus)
 		channels[i] = (struct iio_chan_spec)IIO_CHAN_SOFT_TIMESTAMP(i);
 
+	/* Setting up gain calibration for all channels. */
+	if (st->sw_mode_en && st->chip_info->calib_offset_avail) {
+		ret = st->chip_info->calib_gain_setup_cb(indio_dev);
+		if (ret)
+			return ret;
+	}
+
 	indio_dev->channels = channels;
 
 	return 0;
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index f613583a7fa4095115b0b28e3f8e51cd32b93524..6313eea2bd0ccf97222a50dc26d8ec65042d0db7 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -50,6 +50,7 @@ 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 ad7606_chip_info - chip specific information
@@ -66,6 +67,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
  * @calib_offset_avail: pointer to offset calibration range/limits array
  * @calib_phase_avail:  pointer to phase calibration range/limits array
  */
@@ -81,6 +83,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 v4 5/5] iio: adc: ad7606: add gain calibration support

Posted by Andy Shevchenko 9 months, 1 week ago

On Thu, May 08, 2025 at 12:06:09PM +0200, Angelo Dureghello 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.

...

> +static int ad7606_chan_calib_gain_setup(struct iio_dev *indio_dev)
> +{
> +	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;
> +
> +	/*
> +	 * This function is called once, and parses all the channel nodes,
> +	 * so continuing on next channel node on errors, informing of them.
> +	 */
> +	device_for_each_child_node_scoped(dev, child) {
> +		u32 reg, r_gain;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			continue;

> +		/* Chan reg is a 1-based index. */
> +		if (reg < 1 || reg > num_channels) {
> +			dev_warn(dev, "wrong ch number (ignoring): %d\n", reg);
> +			continue;
> +		}

But this will allow to have a broken DT. This check basically diminishes the
effort of the DT schema validation. If there are limits one still would be able
to create a DT that passes the driver but doesn't pass the validation.

> +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> +					       &r_gain);
> +		if (ret)
> +			/* Keep the default register value. */
> +			continue;
> +
> +		if (r_gain > AD7606_CALIB_GAIN_MAX) {
> +			dev_warn(dev, "wrong gain calibration value");
> +			continue;
> +		}
> +
> +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
> +			DIV_ROUND_CLOSEST(r_gain, AD7606_CALIB_GAIN_STEP));
> +		if (ret) {
> +			dev_warn(dev, "error writing r_gain");
> +			continue;
> +		}
> +	}
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Angelo Dureghello 8 months, 3 weeks ago

Hi Andy,

On 08.05.2025 22:00, Andy Shevchenko wrote:
> On Thu, May 08, 2025 at 12:06:09PM +0200, Angelo Dureghello 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.
> 
> ...
> 
> > +static int ad7606_chan_calib_gain_setup(struct iio_dev *indio_dev)
> > +{
> > +	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;
> > +
> > +	/*
> > +	 * This function is called once, and parses all the channel nodes,
> > +	 * so continuing on next channel node on errors, informing of them.
> > +	 */
> > +	device_for_each_child_node_scoped(dev, child) {
> > +		u32 reg, r_gain;
> > +
> > +		ret = fwnode_property_read_u32(child, "reg", &reg);
> > +		if (ret)
> > +			continue;
> 
> > +		/* Chan reg is a 1-based index. */
> > +		if (reg < 1 || reg > num_channels) {
> > +			dev_warn(dev, "wrong ch number (ignoring): %d\n", reg);
> > +			continue;
> > +		}
> 
> But this will allow to have a broken DT. This check basically diminishes the
> effort of the DT schema validation. If there are limits one still would be able
> to create a DT that passes the driver but doesn't pass the validation.
> 

fixed all your points on other patches of this patch-set. Still your
emails are going to google spam, just could catch them on friday. 
Really not clear why.

About the above, i understand, but the check is actually the same as
in ad7606_get_chan_config(), a warning that fdt is not correct, 
i dont see a blocking issue here now, so not going to change it
in this next patchset.

Regards,
angelo

> > +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> > +					       &r_gain);
> > +		if (ret)
> > +			/* Keep the default register value. */
> > +			continue;
> > +
> > +		if (r_gain > AD7606_CALIB_GAIN_MAX) {
> > +			dev_warn(dev, "wrong gain calibration value");
> > +			continue;
> > +		}
> > +
> > +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
> > +			DIV_ROUND_CLOSEST(r_gain, AD7606_CALIB_GAIN_STEP));
> > +		if (ret) {
> > +			dev_warn(dev, "error writing r_gain");
> > +			continue;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

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

Posted by Andy Shevchenko 8 months, 3 weeks ago

On Mon, May 19, 2025 at 11:40:09AM +0200, Angelo Dureghello wrote:
> On 08.05.2025 22:00, Andy Shevchenko wrote:
> > On Thu, May 08, 2025 at 12:06:09PM +0200, Angelo Dureghello wrote:

...

> > > +	device_for_each_child_node_scoped(dev, child) {
> > > +		u32 reg, r_gain;
> > > +
> > > +		ret = fwnode_property_read_u32(child, "reg", &reg);
> > > +		if (ret)
> > > +			continue;
> > 
> > > +		/* Chan reg is a 1-based index. */
> > > +		if (reg < 1 || reg > num_channels) {
> > > +			dev_warn(dev, "wrong ch number (ignoring): %d\n", reg);
> > > +			continue;
> > > +		}
> > 
> > But this will allow to have a broken DT. This check basically diminishes the
> > effort of the DT schema validation. If there are limits one still would be able
> > to create a DT that passes the driver but doesn't pass the validation.
> 
> fixed all your points on other patches of this patch-set. Still your
> emails are going to google spam, just could catch them on friday. 
> Really not clear why.

DKIM which I still need to configure...

> About the above, i understand, but the check is actually the same as
> in ad7606_get_chan_config(), a warning that fdt is not correct, 
> i dont see a blocking issue here now, so not going to change it
> in this next patchset.

I think the 'continue' above is simply wrong. We should not allow to have
broken tables. And I think it's kinda blocking issue.

> > > +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> > > +					       &r_gain);
> > > +		if (ret)
> > > +			/* Keep the default register value. */
> > > +			continue;
> > > +
> > > +		if (r_gain > AD7606_CALIB_GAIN_MAX) {
> > > +			dev_warn(dev, "wrong gain calibration value");
> > > +			continue;
> > > +		}
> > > +
> > > +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
> > > +			DIV_ROUND_CLOSEST(r_gain, AD7606_CALIB_GAIN_STEP));
> > > +		if (ret) {
> > > +			dev_warn(dev, "error writing r_gain");
> > > +			continue;
> > > +		}
> > > +	}

-- 
With Best Regards,
Andy Shevchenko

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

Posted by Angelo Dureghello 8 months, 3 weeks ago

Hi Andy,

On 19.05.2025 13:14, Andy Shevchenko wrote:
> On Mon, May 19, 2025 at 11:40:09AM +0200, Angelo Dureghello wrote:
> > On 08.05.2025 22:00, Andy Shevchenko wrote:
> > > On Thu, May 08, 2025 at 12:06:09PM +0200, Angelo Dureghello wrote:
> 
> ...
> 
> > > > +	device_for_each_child_node_scoped(dev, child) {
> > > > +		u32 reg, r_gain;
> > > > +
> > > > +		ret = fwnode_property_read_u32(child, "reg", &reg);
> > > > +		if (ret)
> > > > +			continue;
> > > 
> > > > +		/* Chan reg is a 1-based index. */
> > > > +		if (reg < 1 || reg > num_channels) {
> > > > +			dev_warn(dev, "wrong ch number (ignoring): %d\n", reg);
> > > > +			continue;
> > > > +		}
> > > 
> > > But this will allow to have a broken DT. This check basically diminishes the
> > > effort of the DT schema validation. If there are limits one still would be able
> > > to create a DT that passes the driver but doesn't pass the validation.
> > 
> > fixed all your points on other patches of this patch-set. Still your
> > emails are going to google spam, just could catch them on friday. 
> > Really not clear why.
> 
> DKIM which I still need to configure...
> 
> > About the above, i understand, but the check is actually the same as
> > in ad7606_get_chan_config(), a warning that fdt is not correct, 
> > i dont see a blocking issue here now, so not going to change it
> > in this next patchset.
> 
> I think the 'continue' above is simply wrong. We should not allow to have
> broken tables. And I think it's kinda blocking issue.
>

Actually the driver is informing of an incorrect channel node and passes
to the next channel, instead of a probe-fail. It is not introducing any 
non-functionality, just skipping that channel.

Not a big issue for me to fix it and issue a v6. 

If it's really wrong and needed, then i should fix this same issue that
is there in other previously accepted parts.
 
> > > > +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> > > > +					       &r_gain);
> > > > +		if (ret)
> > > > +			/* Keep the default register value. */
> > > > +			continue;
> > > > +
> > > > +		if (r_gain > AD7606_CALIB_GAIN_MAX) {
> > > > +			dev_warn(dev, "wrong gain calibration value");
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
> > > > +			DIV_ROUND_CLOSEST(r_gain, AD7606_CALIB_GAIN_STEP));
> > > > +		if (ret) {
> > > > +			dev_warn(dev, "error writing r_gain");
> > > > +			continue;
> > > > +		}
> > > > +	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Regards,
angelo

[PATCH v4 1/5] Documentation: ABI: IIO: add calibconv_delay documentation
[PATCH v4 2/5] iio: core: add ADC delay calibration definition
[PATCH v4 3/5] iio: adc: ad7606: add offset and phase calibration support
[PATCH v4 4/5] dt-bindings: iio: adc: adi,ad7606: add gain calibration support
[PATCH v4 5/5] iio: adc: ad7606: add gain calibration support