| 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: <aSQxiSoZcI_ol3S5@smile.fi.intel.com>
Date: Mon, 24 Nov 2025 12:20:57 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jorge Marques <jorge.marques@...log.com>
Cc: 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 Mon, Nov 24, 2025 at 10:18:02AM +0100, Jorge Marques wrote:
> The AD4060/AD4062 are versatile, 16-bit/12-bit, successive approximation
> register (SAR) analog-to-digital converter (ADC) with low-power and
> threshold monitoring modes.
...
> +#define AD4062_SOFT_RESET 0x81
The grouping seems a bit strange. Haven't you forgotten a blank line here?
Ditto for other similar cases.
> +#define AD4060_MAX_AVG 0x7
> +#define AD4062_MAX_AVG 0xB
> +#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.
> +#define AD4062_GP_DRDY 0x2
> +#define AD4062_INTR_EN_NEITHER 0x0
> +#define AD4062_TCONV_NS 270
...
> +struct ad4062_state {
> + const struct ad4062_chip_info *chip;
> + const struct ad4062_bus_ops *ops;
> + enum ad4062_operation_mode mode;
> + struct completion completion;
> + struct iio_trigger *trigger;
> + struct iio_dev *indio_dev;
> + struct i3c_device *i3cdev;
> + struct regmap *regmap;
> + u16 sampling_frequency;
> + int vref_uv;
> + int samp_freqs[ARRAY_SIZE(ad4062_conversion_freqs)];
> + u8 oversamp_ratio;
> + union {
> + __be32 be32;
> + __be16 be16;
> + u8 bytes[4];
> + } buf __aligned(IIO_DMA_MINALIGN);
> + u8 reg_addr_conv;
Can't we group u8:s to save a few bytes of memory?
> +};
...
> +static int ad4062_set_oversampling_ratio(struct ad4062_state *st, unsigned int val)
> +{
> + int ret;
> +
> + 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.
> + return -EINVAL;
> +
> + /* 1 disables oversampling */
> + val = ilog2(val);
> + if (val == 0) {
> + st->mode = AD4062_SAMPLE_MODE;
> + } else {
> + st->mode = AD4062_BURST_AVERAGING_MODE;
> + ret = regmap_write(st->regmap, AD4062_REG_AVG_CONFIG, val - 1);
> + if (ret)
> + return ret;
> + }
> + st->oversamp_ratio = BIT(val);
> +
> + return 0;
> +}
...
> +static int ad4062_get_oversampling_ratio(struct ad4062_state *st,
> + unsigned int *val)
> +{
> + int ret, buf;
> +
> + if (st->mode == AD4062_SAMPLE_MODE) {
> + *val = 1;
> + return 0;
> + }
> + ret = regmap_read(st->regmap, AD4062_REG_AVG_CONFIG, &buf);
> + return 0;
This is strange piece of code. Why do we have ret at all?
Please, try to compile kernel also with `make LLVM=1 W=1 ...`
assuming you have clang installed. It catches such issues quite
well.
> +}
...
> +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?
> +}
> +
> +static int ad4062_populate_sampling_frequency(struct ad4062_state *st)
> +{
> + for (int i = 0; i < ARRAY_SIZE(ad4062_conversion_freqs); i++)
Why signed iterator?
> + st->samp_freqs[i] = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
> + st->oversamp_ratio);
Perhaps
st->samp_freqs[i] =
ad4062_calc_sampling_frequency(ad4062_conversion_freqs[i],
st->oversamp_ratio);
But I am not insisting on this case and similar.
> + return 0;
> +}
> +static int ad4062_get_sampling_frequency(struct ad4062_state *st, int *val)
> +{
> + *val = ad4062_calc_sampling_frequency(ad4062_conversion_freqs[st->sampling_frequency],
> + st->oversamp_ratio);
Oh, temporary variable makes this better for readability.
> + return 0;
> +}
...
> +static int ad4062_check_ids(struct ad4062_state *st)
> +{
struct device *dev = &st->i3cdev->dev;
> + int ret;
> + u16 val;
> +
> + ret = regmap_bulk_read(st->regmap, AD4062_REG_PROD_ID_1,
> + &st->buf.be16, sizeof(st->buf.be16));
> + if (ret)
> + return ret;
> +
> + val = get_unaligned_be16(st->buf.bytes);
> + if (val != st->chip->prod_id)
> + dev_warn(&st->i3cdev->dev,
> + "Production ID x%x does not match known values", val);
dev_warn(dev, "Production ID x%x does not match known values", val);
> + ret = regmap_bulk_read(st->regmap, AD4062_REG_VENDOR_H,
> + &st->buf.be16, sizeof(st->buf.be16));
> + if (ret)
> + return ret;
> +
> + val = get_unaligned_be16(st->buf.bytes);
> + if (val != AD4062_I3C_VENDOR) {
> + dev_err(&st->i3cdev->dev,
> + "Vendor ID x%x does not match expected value\n", val);
dev_err(dev, "Vendor ID x%x does not match expected value\n", val);
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
...
> +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.
> + return 0;
> +}
...
> +static int ad4062_request_irq(struct iio_dev *indio_dev)
> +{
> + struct ad4062_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->i3cdev->dev;
> + int ret;
> +
> + ret = fwnode_irq_get_byname(dev_fwnode(&st->i3cdev->dev), "gp1");
> + if (ret == -EPROBE_DEFER) {
> + return ret;
> + } else if (ret < 0) {
Redundant 'else'
> + ret = regmap_update_bits(st->regmap, AD4062_REG_ADC_IBI_EN,
> + AD4062_REG_ADC_IBI_EN_CONV_TRIGGER,
> + AD4062_REG_ADC_IBI_EN_CONV_TRIGGER);
> + } else {
> + ret = devm_request_threaded_irq(dev, ret,
> + ad4062_irq_handler_drdy,
> + NULL, IRQF_ONESHOT, indio_dev->name,
> + indio_dev);
> + }
> +
> + return ret;
> +}
...
> +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.
> +};
...
> +static int ad4062_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, const int **vals,
> + int *type, int *len, long mask)
> +{
> + struct ad4062_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + *vals = ad4062_oversampling_avail;
> + *len = ARRAY_SIZE(ad4062_oversampling_avail);
> + *type = IIO_VAL_INT;
> +
> + return IIO_AVAIL_LIST;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = ad4062_populate_sampling_frequency(st);
> + if (ret)
> + return ret;
> + *vals = st->samp_freqs;
> + *len = st->oversamp_ratio != 1 ? ARRAY_SIZE(ad4062_conversion_freqs) : 1;
Why not using positive conditional?
Funny trick that Elvis operator can be used in this case, but please don't,
it will make code harder to follow.
> + *type = IIO_VAL_INT;
> +
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int ad4062_get_chan_scale(struct iio_dev *indio_dev, int *val, int *val2)
> +{
> + struct ad4062_state *st = iio_priv(indio_dev);
> + const struct iio_scan_type *scan_type;
> +
> + scan_type = iio_get_current_scan_type(indio_dev, st->chip->channels);
> + if (IS_ERR(scan_type))
> + return PTR_ERR(scan_type);
> +
> + *val = (st->vref_uv * 2) / MILLI;
It's most likely (MICRO / MILLI) instead of MILLI. Am I right?
> + *val2 = scan_type->realbits - 1; /* signed */
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> +}
...
> +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;
> +}
...
> +static int ad4062_set_chan_calibscale(struct ad4062_state *st, int gain_int, int gain_frac)
Forgot to wrap this line.
> +{
> + 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.
> + 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.
> + 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 one. */
"...is not equal to one."
Also be consistent with the style for one-line comments. Choose one and
use it everywhere. Usual cases:
- my one-line comment
- My one-line comment
- My one-line comment.
> + 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)));
> +}
...
> +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).
> +{
> + struct i3c_device *i3cdev = st->i3cdev;
> + 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,
> + }
> + };
> + int ret;
> +
> + reinit_completion(&st->completion);
> + /* 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.
> + if (ret)
> + return ret;
> + /*
> + * Single sample read should be used only for oversampling and
> + * sampling frequency pairs that take less than 1 sec.
> + */
> + ret = wait_for_completion_timeout(&st->completion,
> + msecs_to_jiffies(1000));
> + if (!ret)
> + return -ETIMEDOUT;
> +
> + ret = i3c_device_do_priv_xfers(i3cdev, &t[1], 1);
> + if (ret)
> + return ret;
> + *val = get_unaligned_be32(st->buf.bytes);
> + return 0;
> +}
...
> +static int ad4062_read_raw_dispatch(struct ad4062_state *st, int *val, int *val2,
> + long info)
The parameters are split in a logical way here...
(however preference is
static int ad4062_read_raw_dispatch(struct ad4062_state *st,
int *val, int *val2, long info)
to fit 80 characters)
...
> +static int ad4062_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long info)
...but here. Why not
static int ad4062_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long info)
?
> + struct ad4062_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (info == IIO_CHAN_INFO_SCALE)
> + return ad4062_get_chan_scale(indio_dev, val, val2);
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> +
> + ret = ad4062_read_raw_dispatch(st, val, val2, info);
> +
> + iio_device_release_direct(indio_dev);
> + return ret ? ret : IIO_VAL_INT;
return ret ?: IIO_VAL_INT;
> +}
...
> +static int ad4062_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + struct ad4062_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (readval)
> + ret = regmap_read(st->regmap, reg, readval);
> + else
> + ret = regmap_write(st->regmap, reg, writeval);
> +
> + return ret;
Do you expand this in the following patches? If not, ret is not needed.
Just return directly.
> +}
...
> +static int ad4062_regulators_get(struct ad4062_state *st, bool *ref_sel)
> +{
> + struct device *dev = &st->i3cdev->dev;
> + int ret;
> +
> + ret = devm_regulator_get_enable(dev, "vio");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable vio voltage\n");
> +
> + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> + *ref_sel = st->vref_uv == -ENODEV;
_uV ?
> + if (st->vref_uv < 0 && st->vref_uv != -ENODEV) {
You already has the second part
if (st->vref_uV < 0 && !*ref_sel) {
I believe this is better to understand as we check that ref_sel is not chosen.
> + return dev_err_probe(dev, st->vref_uv,
> + "Failed to enable and read ref voltage\n");
> + } else if (st->vref_uv == -ENODEV) {
Redundant 'else'
if (*ref_sel) {
(also in similar way as above)
I don't know if the above was asked specifically, but if so, I ask
the requestor(s) to reconsider.
> + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "vdd");
> + if (st->vref_uv < 0)
> + return dev_err_probe(dev, st->vref_uv,
> + "Failed to enable and read vdd voltage\n");
> + } else {
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable vdd regulator\n");
> + }
> +
> + return 0;
> +}
...
> +static int ad4062_runtime_resume(struct device *dev)
> +{
> + struct ad4062_state *st = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = regmap_clear_bits(st->regmap, AD4062_REG_DEVICE_CONFIG,
> + AD4062_REG_DEVICE_CONFIG_POWER_MODE_MSK);
> + if (ret)
> + return ret;
> +
> + fsleep(4000);
4 * USEC_PER_MSEC, also would be good to add a comment for this long delay.
> + return 0;
> +}
...
> +static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops, ad4062_runtime_suspend,
> + ad4062_runtime_resume, NULL);
I think the logical split is slightly better:
static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
ad4062_runtime_suspend, ad4062_runtime_resume, NULL);
OR
static DEFINE_RUNTIME_DEV_PM_OPS(ad4062_pm_ops,
ad4062_runtime_suspend,
ad4062_runtime_resume,
NULL);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists