Message-ID: <20250825154450.75eedc9b@jic23-huawei>
Date: Mon, 25 Aug 2025 15:44:50 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Jonathan Santos <Jonathan.Santos@...log.com>
Cc: <linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <Michael.Hennerich@...log.com>,
 <lars@...afoo.de>, <dlechner@...libre.com>, <nuno.sa@...log.com>,
 <andy@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
 <conor+dt@...nel.org>, <marcelo.schmitt1@...il.com>
Subject: Re: [PATCH v2 4/4] iio: adc: ad7768-1: add support for ADAQ776x-1
 ADC Family

On Sun, 24 Aug 2025 01:10:13 -0300
Jonathan Santos <Jonathan.Santos@...log.com> wrote:

> Add support for ADAQ7767/68/69-1 series, which includes PGIA and
> Anti-aliasing filter (AAF) gains. Unlike the AD7768-1, they do not
> provide a VCM regulator interface.
> 
> The PGA gain is configured in run-time through the scale attribute,
> if supported by the device. PGA is enabled and controlled by the GPIO
> controller (GPIOs 0, 1 and 2) provided by the device with the SPI
> interface.
> 
> The AAF gain is defined by hardware connections and should be specified
> in device tree.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@...log.com>
Hi Jonathan

Various minor comments inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 51562367a9d4..61d93dbfc4c6 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c

>  
>  struct ad7768_state {
> @@ -235,6 +298,9 @@ struct ad7768_state {
>  	unsigned int samp_freq;
>  	unsigned int samp_freq_avail[ARRAY_SIZE(ad7768_mclk_div_rates)];
>  	unsigned int samp_freq_avail_len;
> +	int pga_gain_mode;
> +	int aaf_gain;
> +	int scale_tbl[ADAQ776X_MAX_GAIN_MODES][2];
>  	struct completion completion;
>  	struct iio_trigger *trig;
>  	struct gpio_desc *gpio_sync_in;
> @@ -243,6 +309,7 @@ struct ad7768_state {
>  	struct gpio_chip gpiochip;
>  	const struct ad7768_chip_info *chip;
>  	bool en_spi_sync;
> +	struct mutex pga_lock; /* protect PGA value access */

why?  What is special about that value? Is it only state in device?
If so I'd go with 
/* protect device internal state (PGA) */


> +static int ad7768_calc_pga_gain(struct ad7768_state *st, int gain_int,
> +				int gain_fract, int precision)
> +{
> +	u64 gain_nano;
> +	int gain_idx;
> +	u32 tmp;
> +
> +	gain_nano = gain_int * NANO + gain_fract;
> +	gain_nano = clamp(gain_nano, 0, ADAQ776X_GAIN_MAX_NANO);
> +	tmp = DIV_ROUND_CLOSEST_ULL(gain_nano << precision, NANO);
> +	gain_nano = DIV_ROUND_CLOSEST(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);
put tmp inline.  It's not adding anything by having it in the local variable
where it's not so obvious it is the number of the previous parameter.
> +
> +	return gain_idx;
> +}
> +
> +static int ad7768_set_pga_gain(struct ad7768_state *st,
> +			       int gain_mode)
> +{
> +	int pgia_pins_value = abs(gain_mode - st->chip->pgia_mode2pin_offset);
> +	int check_val;
> +	int ret;
> +
> +	guard(mutex)(&st->pga_lock);
> +	/* Check GPIO control register */

The code is pretty much saying what that comment does. Drop the comment.

> +	ret = regmap_read(st->regmap, AD7768_REG_GPIO_CONTROL, &check_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((check_val & AD7768_GPIO_PGIA_EN) != AD7768_GPIO_PGIA_EN) {
> +		/* Enable PGIA GPIOs and set them as output */
> +		ret = regmap_write(st->regmap, AD7768_REG_GPIO_CONTROL, AD7768_GPIO_PGIA_EN);

Why check it? Just write it and rely on regcache making that only happen once
if at all.  If you need to be careful about double writing - add a comment.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* 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)
> +		return ret;
> +
> +	st->pga_gain_mode = gain_mode;
> +
> +	return 0;
> +}

>  static int __ad7768_write_raw(struct iio_dev *indio_dev,
>  			      struct iio_chan_spec const *chan,
>  			      int val, int val2, long info)
>  {
>  	struct ad7768_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +	int gain_mode;
>  	int ret;

Might as well combine ret and gain_mode on one line.
or pus gain_mode down into a more local scope so it's not defined here
at all.


>  
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
>  	switch (info) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		return ad7768_set_freq(st, val);
> @@ -887,6 +1083,18 @@ 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;
> +
> +		if (scan_type->sign == 's')
> +			gain_mode = ad7768_calc_pga_gain(st, val, val2,
> +							 scan_type->realbits - 1);
> +		else
> +			gain_mode = ad7768_calc_pga_gain(st, val, val2,
> +							 scan_type->realbits);
> +
> +		return ad7768_set_pga_gain(st, gain_mode);

>  static int ad7768_probe(struct spi_device *spi)
>  {
>  	struct ad7768_state *st;
>  	struct iio_dev *indio_dev;
> +	u32 val;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> @@ -1409,9 +1653,37 @@ static int ad7768_probe(struct spi_device *spi)
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	/* Register VCM output regulator */
> -	ret = ad7768_register_regulators(&spi->dev, st, indio_dev);
> -	if (ret)
> -		return ret;
> +	if (st->chip->has_vcm_regulator) {
> +		ret = ad7768_register_regulators(&spi->dev, st, indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	st->aaf_gain = AD7768_AAF_IN1;
> +	ret = device_property_read_u32(&spi->dev, "adi,gain-milli", &val);
> +	if (ret && st->chip->has_variable_aaf)
> +		dev_warn(&spi->dev, "AAF gain not specified, using default\n");
> +
> +	if (!ret && !st->chip->has_variable_aaf)
> +		dev_warn(&spi->dev, "AAF gain not supported for %s\n", st->chip->name);

I'm confused.  If you hit this warn, you then go ahead and set it anyway. How does that
work?

> +
> +	if (!ret) {
use a local bool for ret here with a suitable name. Carrying on using ret is
fragile against later code changes where someone doesn't realise it is still in use.

> +		/* If provided, validate and set the gain */
> +		switch (val) {
> +		case 1000:
> +			st->aaf_gain = AD7768_AAF_IN1;
> +			break;
> +		case 364:
> +			st->aaf_gain = AD7768_AAF_IN2;
> +			break;
> +		case 143:
> +			st->aaf_gain = AD7768_AAF_IN3;
> +			break;
> +		default:
> +			return dev_err_probe(&spi->dev, -EINVAL,
> +					     "Invalid firmware provided AAF gain\n");
> +		}
> +	}

Powered by blists - more mailing lists