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

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: <aLH7XNuaZ2CkQVWo@JSANTO12-L01.ad.analog.com>
Date: Fri, 29 Aug 2025 16:11:24 -0300
From: Jonathan Santos <jonath4nns@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Jonathan Santos <Jonathan.Santos@...log.com>,
	linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, Michael.Hennerich@...log.com,
	lars@...afoo.de, jic23@...nel.org, 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 08/24, Andy Shevchenko wrote:
> On Sun, Aug 24, 2025 at 7:10 AM 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.
> 
> the device
> 
> ...
> 

...

> >         struct completion completion;
> >         struct iio_trigger *trig;
> >         struct gpio_desc *gpio_sync_in;
> 
> >         struct gpio_chip gpiochip;
> >         const struct ad7768_chip_info *chip;
> >         bool en_spi_sync;
> > +       struct mutex pga_lock; /* protect PGA value access */
> 
> >  }
> 
> Have you run `pahole`? Does it suggest a better layout?
> 
> ...
> 
yes, I have run with the reorganaize option, tried a few combinations, but couldn't found a better arrengement.

> > +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;
> 
> struct u32_fract fract;
> 

Problem is that rational_best_approximation requires a unsigned long. I
could do:

rational_best_approximation(fract.numerator, fract.denominator,
					    INT_MAX, INT_MAX,
					    (unsigned long *)&fract.numerator,
					    (unsigned long *)&fract.denominator);

But I don't know if this is the best practice.

One way around this would be to represent the gains in fractions instead
of 1000x, example:

static const int ad7768_gains_frac[][2] = {
...
	[AD7768_PGA_GAIN_3] = { 13, 5 }, /* 2.600 */
...
};

then we could drop the rational approximation.

> > +       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 = mult_frac(st->vref_uv, denominator, numerator);
> > +               /* Would multiply by NANO here, but value is already in milli */
> > +               tmp2 = shift_right((u64)val * MICRO, val2);
...
> >         init_completion(&st->completion);
> > +       mutex_init(&st->pga_lock);
> 
> Perhaps devm_mutex_init()?
> 

sure!

> -- 
> With Best Regards,
> Andy Shevchenko