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 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/iio/adc/ad7606.h |  4 ++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index a986eb1284106da4980ac36cb0b5990e4e3bd948..049fd8616769d32778aa238b348b2fb82fa83745 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,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,49 @@ 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) {
+		u32 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, "wrong ch number (ignoring): %d\n", reg);
+			continue;
+		}
+
+		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
+					       &r_gain);
+		if (ret == -EINVAL)
+			/* Keep the default register value. */
+			continue;
+		if (ret)
+			return ret;
+
+		if (r_gain > AD7606_CALIB_GAIN_MAX)
+			return dev_err_probe(st->dev, -EINVAL,
+					     "wrong gain calibration value.");
+
+		/* Chan reg is 1-based index. */
+		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
+			DIV_ROUND_CLOSEST(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 +1462,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_CONVDELAY);
+				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 f613583a7fa4095115b0b28e3f8e51cd32b93524..94165d217b69d54cbce9109b8c0f9dc0237cf304 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

On Tue, 2025-05-06 at 23:03 +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.
> 
> 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 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/iio/adc/ad7606.h |  4 ++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index
> a986eb1284106da4980ac36cb0b5990e4e3bd948..049fd8616769d32778aa238b348b2fb82fa83745
> 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,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,49 @@ 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) {
> +		u32 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, "wrong ch number (ignoring): %d\n", reg);
> +			continue;
> +		}
> +

Sorry Angelo, just realized this now. Any reason for not treating the above as a real
invalid argument? It's minor and not a big deal but odd enough...

- Nuno Sá

> +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> +					       &r_gain);
> +		if (ret == -EINVAL)
> +			/* Keep the default register value. */
> +			continue;
> +		if (ret)
> +			return ret;
> +
> +		if (r_gain > AD7606_CALIB_GAIN_MAX)
> +			return dev_err_probe(st->dev, -EINVAL,
> +					     "wrong gain calibration value.");
> +
> +		/* Chan reg is 1-based index. */
> +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
> +			DIV_ROUND_CLOSEST(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 +1462,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_CONVDELAY);
> +				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
> f613583a7fa4095115b0b28e3f8e51cd32b93524..94165d217b69d54cbce9109b8c0f9dc0237cf304
> 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];
>  };
> 

Hi all,
On 07.05.2025 07:14, Nuno Sá wrote:
> On Tue, 2025-05-06 at 23:03 +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.
> > 
> > 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 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/iio/adc/ad7606.h |  4 ++++
> >  2 files changed, 60 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > index
> > a986eb1284106da4980ac36cb0b5990e4e3bd948..049fd8616769d32778aa238b348b2fb82fa83745
> > 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,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,49 @@ 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) {
> > +		u32 reg, r_gain;
> > +

working on further features, i noticed this function is called
for each channel, that is not correct, so need to fix this,
will send a v4.

Regards,
angelo

> > +		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, "wrong ch number (ignoring): %d\n", reg);
> > +			continue;
> > +		}
> > +
> 
> Sorry Angelo, just realized this now. Any reason for not treating the above as a real
> invalid argument? It's minor and not a big deal but odd enough...
> 
> - Nuno Sá
> 
> > +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> > +					       &r_gain);
> > +		if (ret == -EINVAL)
> > +			/* Keep the default register value. */
> > +			continue;
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (r_gain > AD7606_CALIB_GAIN_MAX)
> > +			return dev_err_probe(st->dev, -EINVAL,
> > +					     "wrong gain calibration value.");
> > +
> > +		/* Chan reg is 1-based index. */
> > +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
> > +			DIV_ROUND_CLOSEST(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 +1462,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_CONVDELAY);
> > +				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
> > f613583a7fa4095115b0b28e3f8e51cd32b93524..94165d217b69d54cbce9109b8c0f9dc0237cf304
> > 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];
> >  };
> > 
> 

On 5/8/25 4:16 AM, Angelo Dureghello wrote:
> Hi all,
> On 07.05.2025 07:14, Nuno Sá wrote:
>> On Tue, 2025-05-06 at 23:03 +0200, 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) {
>>> +		u32 reg, r_gain;
>>> +
> 
> working on further features, i noticed this function is called
> for each channel, that is not correct, so need to fix this,
> will send a v4.

Why is this not correct? Each input could have an amplifier with different
series resistor value so this seems correct to me.

> 
> Regards,
> angelo
> 
>>> +		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, "wrong ch number (ignoring): %d\n", reg);
>>> +			continue;
>>> +		}
>>> +
>>
>> Sorry Angelo, just realized this now. Any reason for not treating the above as a real
>> invalid argument? It's minor and not a big deal but odd enough...
>>
>> - Nuno Sá
>>
>>> +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
>>> +					       &r_gain);
>>> +		if (ret == -EINVAL)
>>> +			/* Keep the default register value. */
>>> +			continue;
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		if (r_gain > AD7606_CALIB_GAIN_MAX)
>>> +			return dev_err_probe(st->dev, -EINVAL,
>>> +					     "wrong gain calibration value.");
>>> +
>>> +		/* Chan reg is 1-based index. */
>>> +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
>>> +			DIV_ROUND_CLOSEST(r_gain, AD7606_CALIB_GAIN_STEP));
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +

On 5/8/25 8:50 AM, David Lechner wrote:
> On 5/8/25 4:16 AM, Angelo Dureghello wrote:
>> Hi all,
>> On 07.05.2025 07:14, Nuno Sá wrote:
>>> On Tue, 2025-05-06 at 23:03 +0200, Angelo Dureghello wrote:
>>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>>

...

>>>> +		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, "wrong ch number (ignoring): %d\n", reg);
>>>> +			continue;
>>>> +		}
>>>> +
>>>
>>> Sorry Angelo, just realized this now. Any reason for not treating the above as a real
>>> invalid argument? It's minor and not a big deal but odd enough...
>>>
Ah, I see what you fixed now in v4. All is OK.

On 5/8/25 11:27 AM, David Lechner wrote:
> On 5/8/25 8:50 AM, David Lechner wrote:
>> On 5/8/25 4:16 AM, Angelo Dureghello wrote:
>>> Hi all,
>>> On 07.05.2025 07:14, Nuno Sá wrote:
>>>> On Tue, 2025-05-06 at 23:03 +0200, Angelo Dureghello wrote:
>>>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>>>
> 
> ...
> 
>>>>> +		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, "wrong ch number (ignoring): %d\n", reg);
>>>>> +			continue;
>>>>> +		}
>>>>> +
>>>>
>>>> Sorry Angelo, just realized this now. Any reason for not treating the above as a real
>>>> invalid argument? It's minor and not a big deal but odd enough...
>>>>
> Ah, I see what you fixed now in v4. All is OK.
> 


Oops, trimmed too much, that was in reply to my own comment not Nuno's.

>> Why is this not correct? Each input could have an amplifier with different
>> series resistor value so this seems correct to me.

On 07.05.2025 07:14, Nuno Sá wrote:
> On Tue, 2025-05-06 at 23:03 +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.
> > 
> > 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 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/iio/adc/ad7606.h |  4 ++++
> >  2 files changed, 60 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > index
> > a986eb1284106da4980ac36cb0b5990e4e3bd948..049fd8616769d32778aa238b348b2fb82fa83745
> > 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,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,49 @@ 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) {
> > +		u32 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, "wrong ch number (ignoring): %d\n", reg);
> > +			continue;
> > +		}
> > +
> 
> Sorry Angelo, just realized this now. Any reason for not treating the above as a real
> invalid argument? It's minor and not a big deal but odd enough...
> 
> - Nuno Sá

Hi Nuno,

no problem, feel free to comment,
 
Main reason is that on a wrong fdt channel node, continue does not stop
the parsing of other maybe correct channel nodes.

Regards,
angelo

> 
> > +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> > +					       &r_gain);
> > +		if (ret == -EINVAL)
> > +			/* Keep the default register value. */
> > +			continue;
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (r_gain > AD7606_CALIB_GAIN_MAX)
> > +			return dev_err_probe(st->dev, -EINVAL,
> > +					     "wrong gain calibration value.");
> > +
> > +		/* Chan reg is 1-based index. */
> > +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(reg - 1),
> > +			DIV_ROUND_CLOSEST(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 +1462,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_CONVDELAY);
> > +				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
> > f613583a7fa4095115b0b28e3f8e51cd32b93524..94165d217b69d54cbce9109b8c0f9dc0237cf304
> > 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];
> >  };
> > 
>