Re: [PATCH v2 6/6] iio: adc: ad7124: Implement system calibration

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2025-02-18 at 19:31 +0100, Uwe Kleine-König wrote:
> Allow triggering both zero-scale and full-scale calibration via sysfs in
> the same way as it's done for ad7173.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/iio/adc/ad7124.c | 141 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 124 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 5c2e5a518af3..ad14503e9797 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -4,6 +4,9 @@
>   *
>   * Copyright 2018 Analog Devices Inc.
>   */
> +
> +#define DEBUG

Leftover :)

- Nuno Sá

> +
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> @@ -181,6 +184,7 @@ struct ad7124_channel {
>  	struct ad7124_channel_config cfg;
>  	unsigned int ain;
>  	unsigned int slot;
> +	u8 syscalib_mode;
>  };
>  
>  struct ad7124_state {
> @@ -202,23 +206,6 @@ struct ad7124_state {
>  	DECLARE_KFIFO(live_cfgs_fifo, struct ad7124_channel_config *,
> AD7124_MAX_CONFIGS);
>  };
>  
> -static const struct iio_chan_spec ad7124_channel_template = {
> -	.type = IIO_VOLTAGE,
> -	.indexed = 1,
> -	.differential = 1,
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -		BIT(IIO_CHAN_INFO_SCALE) |
> -		BIT(IIO_CHAN_INFO_OFFSET) |
> -		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> -		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
> -	.scan_type = {
> -		.sign = 'u',
> -		.realbits = 24,
> -		.storagebits = 32,
> -		.endianness = IIO_BE,
> -	},
> -};
> -
>  static struct ad7124_chip_info ad7124_chip_info_tbl[] = {
>  	[ID_AD7124_4] = {
>  		.name = "ad7124-4",
> @@ -903,6 +890,126 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
>  	return 0;
>  }
>  
> +enum {
> +	AD7124_SYSCALIB_ZERO_SCALE,
> +	AD7124_SYSCALIB_FULL_SCALE,
> +};
> +
> +static ssize_t ad7124_write_syscalib(struct iio_dev *indio_dev,
> +				     uintptr_t private,
> +				     const struct iio_chan_spec *chan,
> +				     const char *buf, size_t len)
> +{
> +	struct ad7124_state *st = iio_priv(indio_dev);
> +	struct ad7124_channel *ch = &st->channels[chan->channel];
> +	struct device *dev = &st->sd.spi->dev;
> +	bool sys_calib;
> +	int ret, mode;
> +
> +	ret = kstrtobool(buf, &sys_calib);
> +	if (ret)
> +		return ret;
> +
> +	mode = ch->syscalib_mode;
> +	if (sys_calib) {

FWIW, I agree with both David's comments. Moreover, if we do not claim the IIO
lock in other drivers/devices during calibration I think we should be doing
that. 


- Nuno Sá