Re: [PATCH v2 5/6] iio: adc: ad7124: Implement internal calibration at probe time

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 18 Feb 2025 19:31:12 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Use the calibration function provided by the ad_sigma_delta shim to
> calibrate all channels at probe time.
> 
> For measurements with gain 1 (i.e. if CONFIG_x.PGA = 0) full-scale
> calibrations are not supported and the reset default value of the GAIN
> register is supposed to be used then.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
one passing comment inline. 

> +static int ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio_dev)
> +{
> +	struct device *dev = &st->sd.spi->dev;
> +	int ret, i;
> +	unsigned int adc_control = st->adc_control;
Maybe factor out from here...
> +
> +	/*
> +	 * Calibration isn't supported at full power, so speed down a bit.
> +	 * Setting .adc_control is enough here because the control register is
> +	 * written as part of ad_sd_calibrate() -> ad_sigma_delta_set_mode().
> +	 */
> +	if (FIELD_GET(AD7124_ADC_CTRL_PWR_MSK, adc_control) >= AD7124_FULL_POWER) {
> +		st->adc_control &= ~AD7124_ADC_CTRL_PWR_MSK;
> +		st->adc_control |= AD7124_ADC_CTRL_PWR(AD7124_MID_POWER);
> +	}
> +
> +	for (i = 0; i < st->num_channels; i++) {
> +
> +		if (indio_dev->channels[i].type != IIO_VOLTAGE)
> +			continue;
> +
> +		/*
> +		 * For calibration the OFFSET register should hold its reset default
> +		 * value. For the GAIN register there is no such requirement but
> +		 * for gain 1 it should hold the reset default value, too. So to
> +		 * simplify matters use the reset default value for both.
> +		 */
> +		st->channels[i].cfg.calibration_offset = 0x800000;
> +		st->channels[i].cfg.calibration_gain = st->gain_default;
> +
> +		/*
> +		 * Full-scale calibration isn't supported at gain 1, so skip in
> +		 * that case. Note that untypically full-scale calibration has
> +		 * to happen before zero-scale calibration. This only applies to
> +		 * the internal calibration. For system calibration it's as
> +		 * usual: first zero-scale then full-scale calibration.
> +		 */
> +		if (st->channels[i].cfg.pga_bits > 0) {
> +			ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_INT_FULL, i);
> +			if (ret < 0)
> +				goto out;
> +
> +			/*
> +			 * read out the resulting value of GAIN
> +			 * after full-scale calibration because the next
> +			 * ad_sd_calibrate() call overwrites this via
> +			 * ad_sigma_delta_set_channel() -> ad7124_set_channel()
> +			 * ... -> ad7124_enable_channel().
> +			 */
> +			ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(st->channels[i].cfg.cfg_slot), 3,
> +					     &st->channels[i].cfg.calibration_gain);
> +			if (ret < 0)
> +				goto out;
> +		}
> +
> +		ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_INT_ZERO, i);
> +		if (ret < 0)
> +			goto out;
> +
> +		ret = ad_sd_read_reg(&st->sd, AD7124_OFFSET(st->channels[i].cfg.cfg_slot), 3,
> +				     &st->channels[i].cfg.calibration_offset);
> +		if (ret < 0)
> +			goto out;
> +
> +		dev_dbg(dev, "offset and gain for channel %d = 0x%x + 0x%x\n", i,
> +			st->channels[i].cfg.calibration_offset,
> +			st->channels[i].cfg.calibration_gain);
> +	}

to here as a ad7124_do_calibrate_all() or something like that.
Then you can do direct returns and it becomes really obvious this function
is stashing and restoring the adc_control value.

I don't mind that much as the flow is fairly simple.

> +
> +out:
> +	st->adc_control = adc_control;
> +
> +	return ret;
> +}
> +
>  static void ad7124_reg_disable(void *r)
>  {
>  	regulator_disable(r);
> @@ -1132,6 +1241,10 @@ static int ad7124_probe(struct spi_device *spi)
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to setup triggers\n");
>  
> +	ret = ad7124_calibrate_all(st, indio_dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = devm_iio_device_register(&spi->dev, indio_dev);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to register iio device\n");