Re: [PATCH v8 05/17] iio: adc: Add AD7768 and AD7768-4 core support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] pm: missing pm_runtime_mark_last_busy() defeats autosuspend delay
- [Medium] spi: DMA API violation by reusing buffer across spi_transfer in same message
- [Medium] iio: adc: ad7768: CH_STANDBY uses logical instead of physical channel mask
--

commit 7e51bcc24a25f10a36ea6f88decaae9ab960a856
Author: Janani Sunil <janani.sunil@analog.com>

    iio: adc: Add AD7768 and AD7768-4 core support
    
    Add core support for the AD7768 and AD7768-4 simultaneous sampling ADCs.
    Configure supplies, clock and reset, use a custom regmap bus for the SPI
    protocol, and parse the enabled channels and input buffer settings from
    devicetree.

> diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> new file mode 100644
> index 0000000000000..2c5d0657edf95
> --- /dev/null
> +++ b/drivers/iio/adc/ad7768.c
[ ... ]
> +static u8 ad7768_all_standby_mask(const struct ad7768_state *st)
> +{
> +	return GENMASK(st->chip_info->num_channels - 1, 0);
> +}

[Severity: Medium]
Does this correctly handle the physical channels for the AD7768-4?

The AD7768-4 uses physical channels 0, 1, 4, and 5 (as defined in
ad7768_4_chan_map). Generating a logical mask of GENMASK(3, 0) yields
0x0F, which would write 0 to bits 4 and 5 in the CH_STANDBY register. 
Since 0 means active, won't this force physical channels 4 and 5 to remain 
permanently active and cause a continuous power leak?

[ ... ]
> +static int ad7768_regmap_read(void *context, const void *reg_buf,
> +			      size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct ad7768_state *st = spi_get_drvdata(context);
> +	struct spi_device *spi = context;
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = &st->d16,
> +			.len = sizeof(st->d16),
> +			.cs_change = 1,
> +		}, {
> +			/*
> +			 * Register responses are delayed by one CS frame. While
> +			 * receiving the response to this read, the device also
> +			 * decodes another command on SDI. Repeat the read
> +			 * command to avoid sending an unspecified dummy
> +			 * command.
> +			 */
> +			.tx_buf = &st->d16,
> +			.rx_buf = &st->d16,
> +			.len = sizeof(st->d16),
> +		},
> +	};

[Severity: Medium]
Can reusing the same virtual address (&st->d16) across multiple spi_transfer
structures in the same message lead to DMA mapping conflicts?

When spi_sync_transfer() processes this message, it maps the buffers for each
transfer. Won't this cause &st->d16 to be mapped as DMA_TO_DEVICE for the
first transfer, and then immediately mapped as DMA_BIDIRECTIONAL for the 
second transfer without an unmap in between, potentially violating the
DMA API and causing cache coherence issues?

[ ... ]
> +static int ad7768_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +
> +	pm_runtime_put_autosuspend(regmap_get_device(st->regmap));
> +
> +	return 0;
> +}

[Severity: Medium]
Will this suspend the device synchronously and immediately rather than
honoring the autosuspend delay?

Since pm_runtime_mark_last_busy() is not called before
pm_runtime_put_autosuspend(), the last_busy timestamp will not be refreshed.
If the device was active for longer than the 2000ms autosuspend delay, won't
this result in unnecessary power cycling? This pattern also appears to be
missing in other disable paths.

[ ... ]
> +static int ad7768_parse_config(struct iio_dev *indio_dev,
> +			       struct device *dev)
> +{
[ ... ]
> +		if (channel >= st->chip_info->num_channels)
> +			return dev_err_probe(dev, -ECHRNG,
> +					     "Invalid channel %u in firmware\n",
> +					     channel);
> +
> +		ret = regmap_clear_bits(st->regmap, AD7768_REG_CH_STANDBY,
> +					BIT(channel));

[Severity: Medium]
Is it intentional to use the logical channel index here instead of the
mapped physical channel?

If this is the AD7768-4, the parsed channel index might be 2 or 3, but the
hardware channels that should be taken out of standby are 4 and 5. Should
this use ad7768_channel_mask(st, channel) to derive the correct physical
bits instead of BIT(channel)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-ad7768-driver-v8-0-27aa24db5225@analog.com?part=5