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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VdeSqwVecJHx+jXn9++zgN+CBH56OGUYX5kBbdc0AFKSA@mail.gmail.com>
Date: Sat, 30 Aug 2025 10:57:06 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-spi@...r.kernel.org, jic23@...nel.org, Michael.Hennerich@...log.com, 
	nuno.sa@...log.com, eblanc@...libre.com, dlechner@...libre.com, 
	andy@...nel.org, corbet@....net, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, broonie@...nel.org, Jonathan.Cameron@...wei.com, 
	andriy.shevchenko@...ux.intel.com, ahaslam@...libre.com, 
	marcelo.schmitt1@...il.com
Subject: Re: [PATCH 15/15] iio: adc: ad4030: Add support for ADAQ4216 and ADAQ4224

On Sat, Aug 30, 2025 at 3:46 AM Marcelo Schmitt
<marcelo.schmitt@...log.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