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

[David Lechner]

On 6/3/25 9:36 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>
> Reviewed-by: Nuno Sá <nuno.sa@xxxxxxxxxx>

The patch has changed significantly since these tags were given, so
these tags should have been dropped.

> Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> ---
>  drivers/iio/adc/ad7606.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/iio/adc/ad7606.h |  6 +++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index e0a666cc0e14255754e74daa9e1e88bc4ad1665c..22dcb52ced57d4305db6401605c064fc438d5be4 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,
>  };
> @@ -355,6 +363,36 @@ 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;
> +
> +	device_for_each_child_node_scoped(dev, child) {

Now that I had a deep dive into this in v7, I'm wondering what is the
benefit of having a separate function and iterating over child nodes
a second time compared to just having a bool flag in chip_info and
doing this in the existing iterator in ad7606_probe_channels().

It seems like we could do the same thing in much fewer lines of code
if we avoid adding a callback.

> +		u32 reg, r_gain;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			return ret;
> +
> +		/* Chan reg is a 1-based index. */
> +		if (reg < 1 || reg > num_channels)
> +			return -EINVAL;
> +
> +		r_gain = 0;
> +		ret = fwnode_property_read_u32(child, "adi,rfilter-ohms",
> +					       &r_gain);

ret is set but never read here.

> +		if (r_gain > AD7606_CALIB_GAIN_MAX)
> +			return -EINVAL;
> +
> +		st->r_gain[reg - 1] = r_gain;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ad7606c_18bit_chan_scale_setup(struct iio_dev *indio_dev,
>  					  struct iio_chan_spec *chan)
>  {
> @@ -1352,6 +1390,21 @@ static int ad7606b_sw_mode_setup(struct iio_dev *indio_dev)
>  	return st->bops->sw_mode_config(indio_dev);
>  }
>  
> +static int ad7606_set_gain_calib(struct ad7606_state *st)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < st->chip_info->num_adc_channels; i++) {
> +		ret = st->bops->reg_write(st, AD7606_CALIB_GAIN(i),
> +					  DIV_ROUND_CLOSEST(st->r_gain[i],
> +					  AD7606_CALIB_GAIN_STEP));

I think typical kernel style would be to have at least one more tab
on this line (assuming aligning to "(" makes the line too long).

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ad7606_probe_channels(struct iio_dev *indio_dev)
>  {
>  	struct ad7606_state *st = iio_priv(indio_dev);
> @@ -1444,6 +1497,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);
>  
> +	/* Getting gain calibration values for all channels. */
> +	if (st->sw_mode_en && st->chip_info->calib_gain_setup_cb) {
> +		ret = st->chip_info->calib_gain_setup_cb(indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	indio_dev->channels = channels;
>  
>  	return 0;
> @@ -1620,6 +1680,12 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  		st->chip_info->sw_setup_cb(indio_dev);
>  	}
>  
> +	if (st->sw_mode_en && st->chip_info->calib_gain_setup_cb) {
> +		ret = ad7606_set_gain_calib(st);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(ad7606_probe, "IIO_AD7606");
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index f613583a7fa4095115b0b28e3f8e51cd32b93524..a5b0d318e2f4d73d3708288536e807957c5de68c 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];
>  };
> @@ -131,6 +134,7 @@ struct ad7606_chan_scale {
>   * @data:		buffer for reading data from the device
>   * @offload_en:		SPI offload enabled
>   * @bus_data:		bus-specific variables
> + * @r_gain:		array to store gain calibration resistor value in ohm
>   * @d16:		be16 buffer for reading data from the device
>   */
>  struct ad7606_state {
> @@ -161,6 +165,8 @@ struct ad7606_state {
>  	bool				offload_en;
>  	void				*bus_data;
>  
> +	int				r_gain[AD760X_MAX_CHANNELS];

This isn't used outside of probe, so we could possibly get away with
putting it somewhere else.

> +
>  	/*
>  	 * DMA (thus cache coherency maintenance) may require the
>  	 * transfer buffers to live in their own cache lines.
>