Re: [PATCH v13 2/6] iio: adc: ad4691: add initial driver for AD4691 family

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Meaningful code and commit message mismatch regarding reset sequence.
- [Medium] Regmap cache is not updated for the lower bytes of 16-bit registers written via SPI auto-decrement, leading to stale cache reads.
- [Medium] AD4691_STATE_RESET_REG is a self-clearing command register but is not marked as volatile, causing it to be incorrectly cached.
--

commit 5d0c7a2b74dca7a38834216ccda12567a51d63dd
Author: Radu Sabau <radu.sabau@analog.com>

iio: adc: ad4691: add initial driver for AD4691 family

This commit adds initial driver support for the Analog Devices AD4691
family of multichannel SAR ADCs.

>     Three voltage supply domains are managed: avdd (required), vio, and a
>     reference supply on either the REF pin (ref-supply, external buffer)
>     or the REFIN pin (refin-supply, uses the on-chip reference buffer;
>     REFBUF_EN is set accordingly). Hardware reset is performed by asserting
>     the reset-gpios GPIO line for at least 300 µs then deasserting it;

[Severity: Medium]
The commit message states that the hardware reset is performed by asserting
the reset line for at least 300 µs before deasserting it, but the code in
ad4691_reset() asserts, immediately deasserts, and then waits 300 µs. Is
this description accurate?

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> new file mode 100644
> index 0000000000000..cc1e2ef6bfd8b
> --- /dev/null
> +++ b/drivers/iio/adc/ad4691.c

[ ... ]

> +static bool ad4691_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case AD4691_STATUS_REG:
> +	case AD4691_CLAMP_STATUS1_REG:
> +	case AD4691_CLAMP_STATUS2_REG:
> +	case AD4691_GPIO_READ:
> +	case AD4691_ACC_STATUS_FULL1_REG ... AD4691_ACC_STATUS_SAT2_REG:
> +	case AD4691_ACC_SAT_OVR_REG(0) ... AD4691_ACC_SAT_OVR_REG(15):
> +		return true;

[Severity: Medium]
Does AD4691_STATE_RESET_REG need to be marked as volatile here?

It is used as a self-clearing command register in ad4691_single_shot_read(),
so caching it might return a stale '1' on reads and potentially trigger
spurious resets if the cache is ever synced.

[ ... ]

> +static int ad4691_single_shot_read(struct iio_dev *indio_dev,
> +				   struct iio_chan_spec const *chan, int *val)
> +{
[ ... ]
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   BIT(chan->channel));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> +			   ~BIT(chan->channel) & GENMASK(15, 0));

[Severity: Medium]
Since AD4691_ACC_MASK_REG and AD4691_STD_SEQ_CONFIG use an ADDR_DESCENDING
SPI write where the hardware auto-decrements the address for the second byte,
will the regmap cache for the lower address (e.g., 0x184 for the mask) remain
stale because regmap_write() only updates the cache for the base address?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-ad4692-multichannel-sar-adc-driver-v13-0-1b7626d3b35c@analog.com?part=2