On Tue, 18 Feb 2025 19:31:12 +0100
Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
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");
