| 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
| ||
|
Message-ID: <aSgSsGSUuBtMOuro@smile.fi.intel.com>
Date: Thu, 27 Nov 2025 10:58:24 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jorge Marques <gastmaier@...il.com>
Cc: Jorge Marques <jorge.marques@...log.com>,
Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>,
David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Jonathan Corbet <corbet@....net>,
Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2 3/9] iio: adc: Add support for ad4062
On Wed, Nov 26, 2025 at 12:40:00PM +0100, Jorge Marques wrote:
> On Mon, Nov 24, 2025 at 12:20:57PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
...
> > > +#define AD4062_MON_VAL_MAX_GAIN 1999970
> >
> > This is decimal...
> >
> > > +#define AD4062_MON_VAL_MIDDLE_POINT 0x8000
> >
> > ...and this is hexadecimal. Can you make these consistent?
> > Also, is there any explanation of the number above? To me
> > it looks like 2000000 - 30. Is it so? Or is this a fraction
> > number multiplied by 1000000 or so? In any case some elaboration
> > would be good to have.
> >
> Since this is not a magic number, I will use directly below.
> It MAX_MON_VAL/MON_VAL_MIDDLE_POINT = 0xFFFF/0x8000
Okay, at least it will explain the value.
...
> > > + if (val < 1 || val > BIT(st->chip->max_avg + 1))
> >
> > in_range() ?
> >
> > in_range(val, 1, GENMASK(st->chip->max_avg, 0))
> >
> > if I am not mistaken. Also note, the GENMASK() approach makes possible
> > to have all 32 bits set, however it's most unlikely to happen here anyway.
> >
> Sure, but requires locals to not trigger suspicious usage of sizeof.
> // ...
> const u32 _max = GENMASK(st->chip->max_avg, 0);
> const u32 _min = 1;
> int ret;
>
> if (in_range(val, _min, _max))
> > > + return -EINVAL;
It's fine.
...
> > > +static int ad4062_calc_sampling_frequency(int fosc, unsigned int n_avg)
> > > +{
> > > + /* See datasheet page 31 */
> > > + u64 duration = div_u64((u64)(n_avg - 1) * NSEC_PER_SEC, fosc) + AD4062_TCONV_NS;
> > > +
> > > + return DIV_ROUND_UP_ULL(NSEC_PER_SEC, duration);
> >
> > Why u64?
> >
> > The DIV_ROUND_UP_ULL() seems an overkill here. Or do you expect duration be
> > more than 4 billions?
> >
> This is necessary since at fosc 111 Hz and avg 4096 it does take longer
> than 4 seconds, even though I do timeout after 1 seconds in the raw
> acquisition.
Values above NSEC_PER_SEC+1 do not make sense (it will return 0),
and that fits u32. Can you refactor to avoid 64-bit arithmetics?
> > > +}
...
> > > +static int ad4062_soft_reset(struct ad4062_state *st)
> > > +{
> > > + u8 val = AD4062_SOFT_RESET;
> > > + int ret;
> > > +
> > > + ret = regmap_write(st->regmap, AD4062_REG_INTERFACE_CONFIG_A, val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Wait AD4062 treset time */
> > > + fsleep(5000);
> >
> > 5 * USEC_PER_MSEC
> >
> > This gives a hint on the units without even a need to comment or look somewhere
> > else.
> >
> // TODO
> Since the device functional blocks are powered when voltage is supplied,
> here we can stick with the treset datasheet value 60ns (ndelay(60)).
Add a comment and it will work for me, thanks!
> > > + return 0;
> > > +}
...
> > > +static const int ad4062_oversampling_avail[] = {
> > > + 1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> >
> > It's not easy to count them at glance, please add a comment with indices.
> >
> Ack, will use
> static const int ad4062_oversampling_avail[] = {
> BIT(0), BIT(1), BIT(2), BIT(3), BIT(4), BIT(5), BIT(6), BIT(7), BIT(8),
> BIT(9), BIT(10), BIT(11), BIT(12),
> };
Of course you can use bit notations, but what I meant is to have
1, 2, 4, 8, 16, 32, 64, 128, /* 0 - 7 */
256, 512, 1024, 2048, 4096, /* 8 - 12 */
(or something alike).
> > > +};
...
> > > +static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> > > +{
> > > + u16 gain;
> > > + int ret;
> > > +
> > > + ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> > > + &st->buf.be16, sizeof(st->buf.be16));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + gain = get_unaligned_be16(st->buf.bytes);
> > > +
> > > + /* From datasheet: code out = code in × mon_val/0x8000 */
> > > + *val = gain / AD4062_MON_VAL_MIDDLE_POINT;
> >
> > > + *val2 = mul_u64_u32_div(gain % AD4062_MON_VAL_MIDDLE_POINT, NANO,
> > > + AD4062_MON_VAL_MIDDLE_POINT);
> >
> > I don't see the need for 64-bit division. Can you elaborate what I miss here?
> >
> > > + return IIO_VAL_INT_PLUS_NANO;
> > > +}
> >
> Can be improved to
>
> static int ad4062_get_chan_calibscale(struct ad4062_state *st, int *val, int *val2)
> {
> int ret;
>
> ret = regmap_bulk_read(st->regmap, AD4062_REG_MON_VAL,
> &st->buf.be16, sizeof(st->buf.be16));
> if (ret)
> return ret;
>
> /* From datasheet: code out = code in × mon_val/0x8000 */
> *val = get_unaligned_be16(st->buf.bytes) * 2;
> *val2 = 16;
>
> return IIO_VAL_FRACTIONAL_LOG2;
> }
Much better, thanks!
...
> > > +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
> >
> > Forgot to wrap this line.
> >
> ack
> > > +{
> > > + u64 gain;
> > > + int ret;
> > > +
> > > + if (gain_int < 0 || gain_frac < 0)
> > > + return -EINVAL;
> > > +
> > > + gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> >
> > > +
> >
> > Redundant blank line.
> >
> Ack.
> > > + if (gain > AD4062_MON_VAL_MAX_GAIN)
> > > + return -EINVAL;
> > > +
> > > + put_unaligned_be16(DIV_ROUND_CLOSEST_ULL(gain * AD4062_MON_VAL_MIDDLE_POINT,
> > > + MICRO),
> > > + st->buf.bytes);
> >
> > Also in doubt here about 64-bit division.
> >
> This can be slightly improved to
>
> static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
> int gain_frac)
> {
> u32 gain;
> int ret;
>
> if (gain_int < 0 || gain_frac < 0)
> return -EINVAL;
>
> gain = gain_int * MICRO + gain_frac;
> if (gain > 1999970)
But this magic should be changed to what you explained to me
(as in 0xffff/0x8000 with the proper precision, and this
can be done in 32-bit space).
Or even better
if (gain_int < 0 || gain_int > 1)
return -EINVAL;
if (gain_int == 1 && gain_frac > 0x7fff) // did I get this right?
return -EINVAL;
> return -EINVAL;
>
> put_unaligned_be16(DIV_ROUND_CLOSEST_ULL((u64)gain * AD4062_MON_VAL_MIDDLE_POINT,
> MICRO),
...with temporary variable at minimum.
But again, I still don't see the need for 64-bit space.
> st->buf.bytes);
>
> ret = regmap_bulk_write(st->regmap, AD4062_REG_MON_VAL,
> &st->buf.be16, sizeof(st->buf.be16));
> if (ret)
> return ret;
>
> /* Enable scale if gain is not equal to one */
> return regmap_update_bits(st->regmap, AD4062_REG_ADC_CONFIG,
> AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> FIELD_PREP(AD4062_REG_ADC_CONFIG_SCALE_EN_MSK,
> !(gain_int == 1 && gain_frac == 0)));
> }
>
> To provide the enough resolution to compute every step (e.g., 0xFFFF and
> 0xFFFE) with the arbitrary user input.
...
> > > +static int __ad4062_read_chan_raw(struct ad4062_state *st, int *val)
> >
> > Can be named without leading double underscore? Preference is to use
> > the suffix, like _no_pm (but you can find better one).
> >
> Since there is one usage of this method, can be merged into ad4062_read_chan_raw.
Good choice!
...
> > > + struct i3c_priv_xfer t[2] = {
> > > + {
> > > + .data.out = &st->reg_addr_conv,
> > > + .len = sizeof(st->reg_addr_conv),
> > > + .rnw = false,
> > > + },
> > > + {
> > > + .data.in = &st->buf.be32,
> > > + .len = sizeof(st->buf.be32),
> > > + .rnw = true,
> > > + }
> > > + };
> > > + /* Change address pointer to trigger conversion */
> > > + ret = i3c_device_do_priv_xfers(i3cdev, &t[0], 1);
> >
> > Why array? Just split them on per transfer and use separately. This gives a bit
> > odd feeling that the two goes together, but no. They are semi-related as we
> > have a special condition after the first one.
> >
> For this commit sure, but in the next a fallback method is introduced
> for when the gp1 gpio line is not connected.
> There are two register to trigger and read samples:
>
> * write CONV_READ -> read dummy value - [conversion] -> read value -> [conv ...
> * write CONV_TRIGGER - [conversion] -> read value -> write ...
>
> The first allows almost twice the sampling frequency, but does not work
> with the fallback because In-Band-Interrupt for CONV_READ are not
> yielded.
Do you mean that the same array is reused differently? If so, then okay.
> > > + if (ret)
> > > + return ret;
...
> > > + fsleep(4000);
> >
> > 4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
> >
> Will add
> /* Wait device functional blocks to power up */
> Based on hardware tests, I can drop to 2 * USEC_PER_MSEC, lower than
> that the device is not ready to switch to acquisition mode for
> conversions.
Good!
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists