Re: [PATCH 15/15] iio: adc: ad4030: Add support for ADAQ4216 and ADAQ4224

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sat, Aug 30, 2025 at 3:46 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> ADAQ4216 and ADAQ4224 are similar to AD4030, but feature a PGA circuitry
> that scales the analog input signal prior to it reaching the ADC. The PGA
> is controlled through a pair of pins (A0 and A1) whose state define the
> gain that is applied to the input signal.
>
> Add support for ADAQ4216 and ADAQ4224. Provide a list of PGA options
> through the IIO device channel scale available interface and enable control
> of the PGA through the channel scale interface.

...

>  /* Datasheet says 9.8ns, so use the closest integer value */
>  #define AD4030_TQUIET_CNV_DELAY_NS     10

You already used that in one of the previous patches, can you move
there this one and use instead of magic += 10?


> +/* HARDWARE_GAIN */
> +#define ADAQ4616_PGA_PINS              2
> +#define ADAQ4616_GAIN_MAX_NANO         6666666667

Can we use calculus instead (people can't count properly after 3 :-)?
Something like this

(NANO * 2 / 3) // whoever in the above it's 20 and this puzzles me how
something with _NANO can be so big :-)

...

> +/*
> + * Gains computed as fractions of 1000 so they can be expressed by integers.
> + */
> +static const int ad4030_hw_gains[] = {
> +       333, 556, 2222, 6667,

Again, instead of comment (or in addition to) this can be written as

1000 / 3, 5000 / 9, 20000 / 9, 20000 / 3,

Let the compiler do its job.

> +};
> +
> +static const int ad4030_hw_gains_frac[4][2] = {

Drop 4

> +       { 1, 3 },  /* 1/3 gain */
> +       { 5, 9 },  /* 5/9 gain */
> +       { 20, 9 }, /* 20/9 gain */
> +       { 20, 3 }, /* 20/3 gain */
> +};

...

> +static int ad4030_write_raw_get_fmt(struct iio_dev *indio_dev,
> +                                   struct iio_chan_spec const *chan, long mask)
> +{
> +       switch (mask) {
> +       case IIO_CHAN_INFO_SCALE:
> +               return IIO_VAL_INT_PLUS_NANO;
> +       default:
> +               return IIO_VAL_INT_PLUS_MICRO;
> +       }

> +       return -EINVAL;

What's the point of this return?

> +}

...

> +static int ad4030_setup_pga(struct device *dev, struct iio_dev *indio_dev,
> +                           struct ad4030_state *st)
> +{
> +       unsigned int i;
> +       int pga_value;
> +       int ret;
> +
> +       ret = device_property_read_u32(dev, "adi,pga-value", &pga_value);
> +       if (ret && ret != -EINVAL)
> +               return dev_err_probe(dev, ret, "Failed to get PGA value.\n"
> +
> +       if (ret == -EINVAL) {

This can be done differently, i.e. check for EINVAL first and in
'else' branch check for other ret != 0. This will deduplicate the
EINVAL check.

> +               /* Setup GPIOs for PGA control */
> +               st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
> +               if (IS_ERR(st->pga_gpios))
> +                       return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
> +                                            "Failed to get PGA gpios.\n");
> +
> +               if (st->pga_gpios->ndescs != 2)
> +                       return dev_err_probe(dev, -EINVAL,
> +                                            "Expected 2 GPIOs for PGA control.\n");
> +
> +               st->scale_avail_size = ARRAY_SIZE(ad4030_hw_gains);
> +               st->pga_index = 0;
> +               return ad4030_set_pga_gain(st);
> +       }

...

> +       .max_sample_rate_hz = 2 * MEGA,

HZ_PER_MHZ

...

> +       .max_sample_rate_hz = 2 * MEGA,

Ditto.

-- 
With Best Regards,
Andy Shevchenko