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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Overlapping active DMA mappings for the same memory buffer (`st->d16`) across concurrent SPI transfers violates the DMA API.
- [High] Incorrect channel bit mapping for CH_STANDBY and CH_MODE_SEL on AD7768-4 breaks logical-to-physical translation.
- [Medium] Hardware left powered on after device removal due to cancelled async suspend.
- [Medium] Incorrect polarity for reference buffer configuration (powers down instead of enabling).
- [Medium] Swapped POS and NEG bit assignments for precharge buffers.
- [Medium] Unbalanced IIO backend channel enablement on error path in `ad7768_update_scan_mode`.
- [Medium] Missing PM runtime idle notification at the end of probe leaves the device permanently active.
--

commit cad3d6d13491f167da3c31d541a31ac850d8d3b5
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..c80056a9edd8c
> --- /dev/null
> +++ b/drivers/iio/adc/ad7768.c
> @@ -0,0 +1,1000 @@
> +// SPDX-License-Identifier: GPL-2.0

[ ... ]

> +#define AD7768_REG_PRECHARGE_BUF1			0x11
> +#define AD7768_REG_PRECHARGE_BUF2			0x12
> +#define   AD7768_PREBUF_POS_EN(ch)			BIT((ch) * 2)
> +#define   AD7768_PREBUF_NEG_EN(ch)			BIT(((ch) * 2) + 1)

[Severity: Medium]
Are these bit assignments for the precharge buffers swapped? According to the
AD7768 datasheet, the positive (POS) precharge buffers are assigned to odd bits,
and the negative (NEG) precharge buffers are assigned to even bits.

> +
> +#define AD7768_REG_REFP_BUF				0x13
> +#define AD7768_REG_REFN_BUF				0x14

[ ... ]

> +static u8 ad7768_all_standby_mask(const struct ad7768_state *st)
> +{
> +	return GENMASK(st->chip_info->num_channels - 1, 0);
> +}

[Severity: High]
Does this generate the correct physical mask for the AD7768-4? The AD7768-4
maps its 4 logical channels to physical channels 0, 1, 4, and 5. This returns
GENMASK(3, 0) which is 0x0F, writing 0 to physical bits 4, 5, 6, and 7,
leaving them fully active.

[ ... ]

> +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: High]
Can this violate the DMA API? Both transfers use the same memory address
(&st->d16) for their buffers. When the SPI subsystem maps all transfers for
DMA concurrently, attempting to map the same memory region twice concurrently
(DMA_TO_DEVICE and DMA_BIDIRECTIONAL) can cause cache coherency issues on
non-coherent architectures.

[ ... ]

> +static int ad7768_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	unsigned long channel_mask;
> +	unsigned long standby_mask;
> +	int ret;
> +
> +	channel_mask = ad7768_all_standby_mask(st);
> +	standby_mask = channel_mask & ~*scan_mask;
> +
> +	/*
> +	 * Crystal excitation requires channel 4 on AD7768 or channel 2 on
> +	 * AD7768-4 to remain active.
> +	 */
> +	if (st->clock_source == AD7768_CLOCK_SOURCE_XTAL)
> +		__clear_bit(st->chip_info->num_channels / 2, &standby_mask);
> +
> +	ret = regmap_update_bits(st->regmap, AD7768_REG_CH_STANDBY,
> +				 channel_mask, standby_mask);
> +	if (ret)
> +		return ret;
> +
> +	for (unsigned int c = 0; c < st->chip_info->num_channels; c++) {
> +		if (test_bit(c, scan_mask))
> +			ret = iio_backend_chan_enable(st->back, c);
> +		else
> +			ret = iio_backend_chan_disable(st->back, c);
> +		if (ret)
> +			return ret;
> +	}

[Severity: Medium]
Will this leave the backend hardware out of sync if an error occurs? If one
operation fails, it returns the error immediately without unwinding the
previously modified channels.

> +
> +	return 0;
> +}

[ ... ]

> +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]
Could this leave the hardware powered on after device removal?

If the driver is unbound while the IIO buffer is active,
devm_iio_device_register unwinds first, calling iio_device_unregister(),
which disables the buffer and triggers ad7768_buffer_postdisable().

Immediately after, devm_pm_runtime_set_active_enabled unwinds, calling
pm_runtime_disable() which cancels the pending autosuspend work. The
driver should ensure synchronous suspend if the buffer is active during
removal.

[ ... ]

> +static int ad7768_configure_precharge_buffers(struct iio_dev *indio_dev,
> +					      struct ad7768_precharge_config *precharge_cfg)
> +{
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	u8 prebuf1_val, prebuf2_val;
> +	u16 prebuf_mask = 0;
> +	u8 refbufp_val = 0;
> +	u8 refbufn_val = 0;
> +	int ret;
> +
> +	for (unsigned int ch = 0; ch < indio_dev->num_channels; ch++) {
> +		u8 channel = indio_dev->channels[ch].channel;
> +
> +		if (precharge_cfg[channel].prebufp_en)
> +			prebuf_mask |= AD7768_PREBUF_POS_EN(channel);
> +
> +		if (precharge_cfg[channel].prebufn_en)
> +			prebuf_mask |= AD7768_PREBUF_NEG_EN(channel);
> +
> +		if (precharge_cfg[channel].refbufp)
> +			refbufp_val |= ad7768_channel_mask(st, channel);
> +
> +		if (precharge_cfg[channel].refbufn)
> +			refbufn_val |= ad7768_channel_mask(st, channel);
> +	}

[Severity: Medium]
Does this apply the incorrect polarity for the reference buffer configuration?
When adi,refbuf-pos-enable or adi,refbuf-neg-enable are provided in the device
tree, the driver sets the corresponding bit to 1. According to the AD7768
datasheet, setting a bit to 1 in the REFP_BUF and REFN_BUF registers powers down
the buffer.

[ ... ]

> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
> +		u32 channel;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &channel);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to parse reg of %pfwP\n",
> +					     child);
> +
> +		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: High]
Does this use logical indices instead of physical bits for the AD7768-4? The
AD7768-4 uses physical channels 0, 1, 4, and 5. Using BIT(channel) will update
physical bits 0-3 instead of the mapped hardware bits.

> +		if (ret)
> +			return ret;
> +

[ ... ]

> +	pm_runtime_set_autosuspend_delay(dev, 2000);
> +	pm_runtime_use_autosuspend(dev);
> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

[Severity: Medium]
Does this leave the device permanently active? This configures autosuspend and
enables runtime PM, which transitions the device to RPM_ACTIVE with a usage
count of 0, but fails to call pm_request_idle(dev). As a result, the autosuspend
timer is never triggered, leaving the device fully powered indefinitely until
a user explicitly accesses it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-ad7768-driver-v6-0-e4378f946bfb@analog.com?part=5