Re: [PATCH 4/4] iio: adc: ad7768-1: add support for ADAQ776x-1 ADC Family

[Jonathan Santos]

On 08/14, Nuno Sá wrote:
> On Tue, 2025-08-12 at 23:49 -0300, 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.
> > 
> > The AAF gain is configured in the devicetree and it should correspond to
> > the AAF channel selected in hardware.
> > 
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@xxxxxxxxxx>
> > ---
> 
> Hi Jonathan,
> 
> Some comments from me...
> 

...

> > +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;
> > +
> > +	/* Check GPIO control register */
> > +	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);
> > +		if (ret < 0)
> > +			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 < 0)
> > +		return ret;
> > +
> > +	st->pga_gain_mode = gain_mode;
> > +
> 
> It looks the above function could use some locking.
> 
> > +	return 0;
> > +}
> > +
> >  static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int
> > offset)
> >  {
> >  	struct iio_dev *indio_dev = gpiochip_get_data(chip);
> > @@ -782,13 +937,17 @@ static const struct iio_chan_spec ad7768_channels[] = {
> >  	AD7768_CHAN(0, AD7768_CHAN_INFO_NONE),
> >  };
> >  
> > +static const struct iio_chan_spec adaq776x_channels[] = {
> > +	AD7768_CHAN(0, BIT(IIO_CHAN_INFO_SCALE)),
> > +};
> > +
> >  static int ad7768_read_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 scale_uv, ret, temp;
> > +	int ret, temp;
> >  
> >  	scan_type = iio_get_current_scan_type(indio_dev, chan);
> >  	if (IS_ERR(scan_type))
> > @@ -809,12 +968,19 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
> >  		return IIO_VAL_INT;
> >  
> >  	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];
> > +		/*
> > +		 * ADC output code is two's complement so only (realbits - 1)
> > +		 * bits express voltage magnitude.
> > +		 */
> > +		*val2 = scan_type->realbits - 1;
> >  
> 
> I'm a bit confused. Is there something wrong with the original code that needs to be
> fixed? The above change seems to effectively change behavior of the code with had
> before.
> 
> - Nuno Sá

I think it is more of a semantic clarification than a behavioral change
per se. Previously, the calculation gave the impression of a bipolar 
unsigned input, since it was using the full scale. With the update, 
we’re explicitly considering the sign bit, which makes it clear 
that the input is a two’s complement signal.

Altough pehaps this should not be mixed into the patch.

> >  		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;
> >  	}
> >  }
...

- Jonathan Santos