| 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: <zryqws2h2i4duejczo2rptwhlzhile7fa7brriqh2hmtarwjxn@cr2cyzymwpav>
Date: Fri, 28 Nov 2025 19:50:02 +0100
From: Jorge Marques <gastmaier@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.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 Thu, Nov 27, 2025 at 10:58:24AM +0200, Andy Shevchenko wrote:
> 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?
>
Ok, any frequency lower than 1 Hz does not make sense.
static int ad4062_calc_sampling_frequency(int fosc, unsigned int oversamp_ratio)
{
/* See datasheet page 31 */
u32 period = NSEC_PER_SEC / fosc;
u32 n_avg = BIT(oversamp_ratio) - 1;
/* Result is less than 1 Hz */
if (n_avg >= fosc)
return 1;
return NSEC_PER_SEC / (n_avg * period + AD4062_TCONV_NS);
}
> > > > +}
>
> ...
>
> > > > +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).
>
Ack
> > > > +};
>
> ...
>
> > > > +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;
>
gain_frac would be 999999 max, or 999970 for the limit that fits in the
register after the math. I think > 1.999.970 is self explanatory.
> > 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.
>
Well, by dividing mon_val and micro values by a common divisor the
operation fit in 32-bits:
static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int,
int gain_frac)
{
const u32 mon_val = AD4062_MON_VAL_MIDDLE_POINT / 64;
const u32 micro = MICRO / 64;
const u32 gain = gain_int * MICRO + gain_frac;
int ret;
if (gain_int < 0 || gain_frac < 0)
return -EINVAL;
if (gain > 1999970)
return -EINVAL;
put_unaligned_be16(DIV_ROUND_CLOSEST(gain * mon_val, micro), 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)));
}
> > 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.
>
Yes
> > > > + 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
>
>
Best regards,
Jorge
Powered by blists - more mailing lists