Message-ID: <aKTNEP7pNY9ZbrPe@debian-BULLSEYE-live-builder-AMD64>
Date: Tue, 19 Aug 2025 16:14:24 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Jonathan Santos <Jonathan.Santos@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, Michael.Hennerich@...log.com,
	jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
	andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, jonath4nns@...il.com
Subject: Re: [PATCH 4/4] iio: adc: ad7768-1: add support for ADAQ776x-1 ADC
 Family

Hi Jonathan,

A few thoughts from me.

On 08/12, Jonathan Santos wrote:
> Add support for ADAQ7767/68/69-1 series, which includes PGIA and
> Anti-aliasing filter (AAF) gains.
> 
> The PGA gain is configured in run-time through the scale attribute,
> if supported by the device. The scale options are updated according
> to the output data width.
This could provide more information/context. How the PGA is controlled/configured?

> 
> The AAF gain is configured in the devicetree and it should correspond to
> the AAF channel selected in hardware.
Would this be more clear if stated the other way around? 

The AAF gain is defined by hardware connections and thus is specified in device tree. ?

> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@...log.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 286 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 279 insertions(+), 7 deletions(-)
> 
...
>  
> +static void ad7768_fill_scale_tbl(struct iio_dev *dev)
> +{
> +	struct ad7768_state *st = iio_priv(dev);
> +	const struct iio_scan_type *scan_type;
> +	int val, val2, tmp0, tmp1, i;
> +	unsigned long denominator, numerator;
> +	u64 tmp2;
> +
> +	scan_type = iio_get_current_scan_type(dev, &dev->channels[0]);
> +	if (scan_type->sign == 's')
> +		val2 = scan_type->realbits - 1;
> +	else
> +		val2 = scan_type->realbits;
> +
> +	for (i = 0; i < st->chip->num_pga_modes; i++) {
> +		/* Convert gain to a fraction format */
> +		numerator = st->chip->pga_gains[i];
> +		denominator = MILLI;
> +		if (st->chip->has_variable_aaf) {
> +			numerator *= ad7768_aaf_gains[st->aaf_gain];
> +			denominator *= MILLI;
> +		}
> +
> +		rational_best_approximation(numerator, denominator, __INT_MAX__, __INT_MAX__,
> +					    &numerator, &denominator);
> +
> +		val = st->vref_uv / 1000;
What about keeping the reference in µV (not dividing by MILLI)?

> +		/* Multiply by MILLI here to avoid losing precision */
> +		val = mult_frac(val, denominator * MILLI, numerator);
then this can be just 
		val = mult_frac(val, denominator, numerator);

> +		/* Would multiply by NANO here but we already multiplied by MILLI */
> +		tmp2 = shift_right((u64)val * MICRO, val2);
> +		tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);
val is never negative here so can use div_u64_rem() or maybe do_div().

> +		st->scale_tbl[i][0] = tmp0; /* Integer part */
> +		st->scale_tbl[i][1] = abs(tmp1); /* Fractional part */
> +	}
> +}
> +
...
>  
> +static int ad7768_calc_pga_gain(struct ad7768_state *st, int gain_int,
> +				int gain_fract, int precision)
> +{
> +	u64 gain_nano, tmp;
> +	int gain_idx;
> +
> +	precision--;
This is odd out of context.
Also, it only applies to ADCs that provide output codes in two's complement
format. See comment below.


> +	gain_nano = gain_int * NANO + gain_fract;
> +	if (gain_nano < 0 || gain_nano > ADAQ776X_GAIN_MAX_NANO)
I've seen some build tools complain about comparisons like gain_nano < 0 with
gain_nano being u64. Since that's unsigned, it can never be < 0. And in the
context of gain/attenuation, we know gain_nano shall never be negative.
Would just drop the gain_nano < 0 comparison. Or maybe clamp() the value?

> +		return -EINVAL;
> +
> +	tmp = DIV_ROUND_CLOSEST_ULL(gain_nano << precision, NANO);
> +	gain_nano = DIV_ROUND_CLOSEST_ULL(st->vref_uv, tmp);
> +	if (st->chip->has_variable_aaf)
> +		/* remove the AAF gain from the overall gain */
> +		gain_nano = DIV_ROUND_CLOSEST_ULL(gain_nano *  MILLI,
> +						  ad7768_aaf_gains[st->aaf_gain]);
> +	tmp = st->chip->num_pga_modes;
> +	gain_idx = find_closest(gain_nano, st->chip->pga_gains, tmp);
> +
> +	return gain_idx;
> +}
> +
...
> +
> +	/* Write the respective gain values to GPIOs 0, 1, 2 */
> +	ret = regmap_write(st->regmap, AD7768_REG_GPIO_WRITE,
> +			   AD7768_GPIO_WRITE(pgia_pins_value));
> +	if (ret < 0)
I think regmap_write() doesn't return positive values so we can have 'if (ret)' here.

> +		return ret;
> +
> +	st->pga_gain_mode = gain_mode;
> +
> +	return 0;
> +}
> +
...
>  	case IIO_CHAN_INFO_SCALE:
> -		scale_uv = st->vref_uv;
> -		if (scale_uv < 0)
> -			return scale_uv;
> -
> -		*val = (scale_uv * 2) / 1000;
> -		*val2 = scan_type->realbits;
> +		if (st->chip->has_pga) {
> +			*val = st->scale_tbl[st->pga_gain_mode][0];
> +			*val2 = st->scale_tbl[st->pga_gain_mode][1];
> +			return IIO_VAL_INT_PLUS_NANO;
> +		}
> +		*val = st->vref_uv / 1000;
> +		if (st->chip->has_variable_aaf)
> +			*val = (*val * MILLI) / ad7768_aaf_gains[st->aaf_gain];
Similar thing here. Would it make sense to use st->vref_uv without dividing it
by MILLI?

> +		/*
> +		 * ADC output code is two's complement so only (realbits - 1)
> +		 * bits express voltage magnitude.
> +		 */
> +		*val2 = scan_type->realbits - 1;
I see the rationally for this. Instead of doing 'scale_uv * 2' to account for
the range of negative values, this is now using one less precision bit which
shall lead to the same result after going through IIO_VAL_FRACTIONAL_LOG2
handling. I personally prefer the realbits - 1 logic, but others may prefer
to avoid this change since it was already working with 'scale_uv * 2'.

>  
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  
> @@ -869,18 +1035,42 @@ static int ad7768_read_avail(struct iio_dev *indio_dev,
>  		*length = st->samp_freq_avail_len;
>  		*type = IIO_VAL_INT;
>  		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (int *)st->scale_tbl;
> +		*length = st->chip->num_pga_modes * 2;
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		return IIO_AVAIL_LIST;
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
...
> @@ -892,6 +1082,13 @@ static int __ad7768_write_raw(struct iio_dev *indio_dev,
>  
>  		/* Update sampling frequency */
>  		return ad7768_set_freq(st, st->samp_freq);
> +	case IIO_CHAN_INFO_SCALE:
> +		if (!st->chip->has_pga)
> +			return -EOPNOTSUPP;
> +
> +		gain_mode = ad7768_calc_pga_gain(st, val, val2,
> +						 scan_type->realbits);
Check scan_type.sign and pass scan_type->realbits - 1 to ad7768_calc_pga_gain()
if the ADC output codes are in two's complement format.

> +		return ad7768_set_pga_gain(st, gain_mode);
>  	default:
>  		return -EINVAL;
>  	}
...
> +static const struct ad7768_chip_info adaq7767_chip_info = {
> +	.name = "adaq7767-1",
> +	.channel_spec = ad7768_channels,
> +	.num_channels = ARRAY_SIZE(ad7768_channels),
> +	.available_masks = ad7768_channel_masks,
> +	.has_pga = false,
I think these flag initialization can be omitted when they are false.

> +	.has_variable_aaf = true
> +};
> +
> +static const struct ad7768_chip_info adaq7768_chip_info = {
> +	.name = "adaq7768-1",
> +	.channel_spec = adaq776x_channels,
> +	.num_channels = ARRAY_SIZE(adaq776x_channels),
> +	.available_masks = ad7768_channel_masks,
> +	.pga_gains = adaq7768_gains,
> +	.default_pga_mode = AD7768_PGA_GAIN_2,
> +	.num_pga_modes = ARRAY_SIZE(adaq7768_gains),
> +	.pgia_mode2pin_offset = 6,
> +	.has_pga = true,
> +	.has_variable_aaf = false
Same here.

> +};
> +
...
> @@ -1418,6 +1652,35 @@ static int ad7768_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	st->aaf_gain = AD7768_AAF_IN1;
> +	ret = device_property_read_u32(&spi->dev, "adi,aaf-gain", &val);
> +	if (ret) {
> +		/* AAF gain required, but not specified */
> +		if (st->chip->has_variable_aaf)
> +			return dev_err_probe(&spi->dev, -EINVAL, "AAF gain not specified\n");
> +
> +	} else if (!st->chip->has_variable_aaf) {
> +		/* AAF gain provided, but not supported */
> +		return dev_err_probe(&spi->dev, -EINVAL, "AAF gain not supported for %s\n",
> +				     st->chip->name);
Not really sure what to do in these cases. Can't we just ignore or warn on
properties for unsupported features?


Best regards,
Marcelo

Powered by blists - more mailing lists