RE: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support

[Sabau, Radu bogdan]

Addressing Sashiko's review for the offload support patch.

> -----Original Message-----
> From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@xxxxxxxxxx>
> Sent: Thursday, April 30, 2026 1:17 PM

...

> All offload transfers use 16-bit frames (bits_per_word=16, len=2).
> The channel scan_type (storagebits=16, shift=0, IIO_BE) is shared
> between the software triggered-buffer and offload paths; no separate
> scan_type or channel array is needed for the offload case. The
> ad4691_manual_channels[] array introduced in the triggered-buffer
> commit is reused here: it hides the IIO_CHAN_INFO_OVERSAMPLING_RATIO
> attribute, which is not applicable in Manual Mode.

"This isn't a bug, but ad4691_manual_channels and the oversampling attribute
don't appear to exist in this driver. Was this copied from another driver?"

Agreed on the commit message as per Jonathan's input as well.
Removed the two sentences referencing ad4691_manual_channels[] and
IIO_CHAN_INFO_OVERSAMPLING_RATIO — both were carried over from a
draft that included OSR support, which was split into the following commit.
The updated message now calls out explicitly that per-mode channel array
distinctions and oversampling support are introduced in the next commit.

> 
> Kconfig gains a dependency on IIO_BUFFER_DMAENGINE.
> 
> Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
> ---
>  drivers/iio/adc/Kconfig  |   2 +
>  drivers/iio/adc/ad4691.c | 392
> ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 389 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index d498f16c0816..fdc6565933c5 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -143,8 +143,10 @@ config AD4691
>  	tristate "Analog Devices AD4691 Family ADC Driver"
>  	depends on SPI
>  	select IIO_BUFFER
> +	select IIO_BUFFER_DMAENGINE
>  	select IIO_TRIGGERED_BUFFER
>  	select REGMAP
> +	select SPI_OFFLOAD
>  	help
>  	  Say yes here to build support for Analog Devices AD4691 Family
> MuxSAR
>  	  SPI analog to digital converters (ADC).
> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index c1e3406fef18..50d81d87d4a0 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
> @@ -23,10 +23,14 @@

...

> +struct ad4691_offload_state {
> +	struct spi_offload *offload;
> +	struct spi_offload_trigger *trigger;
> +	u64 trigger_hz;
> +	u8 tx_cmd[17][2];
> +	u8 tx_reset[4];
>  };

"Could using these buffers for DMA transfers cause memory corruption on
architectures with non-coherent caches? 
They are not marked with __aligned(IIO_DMA_MINALIGN), so they share cachelines
with each other and adjacent struct members like trigger_hz."

Valid. struct ad4691_offload_state is allocated with devm_kzalloc
which does not guarantee cache-line alignment, so tx_cmd and tx_reset
shared cache lines with trigger_hz and the pointer fields. Removed both
from ad4691_offload_state entirely. The existing scan_tx[17] field in
struct ad4691_state is large enough for all 17 AVG_IN addresses and is
already __aligned(IIO_DMA_MINALIGN); reused it for the offload TX
commands (the two paths are mutually exclusive at probe).

> 
>  struct ad4691_state {
> @@ -252,7 +286,10 @@ struct ad4691_state {
>  	 *		    transfers in one go.
>  	 */
>  	struct spi_message scan_msg;

...

> +
> +static int ad4691_cnv_burst_offload_buffer_postenable(struct iio_dev
> *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	struct ad4691_offload_state *offload = st->offload;
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct spi_device *spi = to_spi_device(dev);

...

> +		/* TX: address phase, CS stays asserted into data phase */
> +		st->scan_xfers[2 * k].tx_buf = offload->tx_cmd[k];
> +		st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> +		st->scan_xfers[2 * k].bits_per_word = bpw;

"When bits_per_word is greater than 8 (like bpw = 16 here), the SPI framework
treats tx_buf as an array of native 16-bit words.
On little-endian architectures, the controller will byte-swap the data before
transmitting it. Will using a u8 array and put_unaligned_be16() result in the
command bytes being reversed on the wire?"

Switched to cpu_to_be16() assigned directly into __be16 scan_tx[],
matching the non-offload path. This makes the intended wire format
self-evident and sidesteps the byte-ordering question entirely.

> +
> +		/* RX: data phase, CS toggles after to delimit the next register
> op */
> +		st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);
> +		st->scan_xfers[2 * k + 1].bits_per_word = bpw;
> +		st->scan_xfers[2 * k + 1].offload_flags =
> SPI_OFFLOAD_XFER_RX_STREAM;
> +		st->scan_xfers[2 * k + 1].cs_change = 1;
> +		k++;
> +	}
> +
> +	/* State reset to re-arm DATA_READY for the next scan. */
> +	put_unaligned_be16(AD4691_STATE_RESET_REG, offload->tx_reset);
> +	offload->tx_reset[2] = AD4691_STATE_RESET_ALL;
> +
> +	st->scan_xfers[2 * k].tx_buf = offload->tx_reset;
> +	st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> +	st->scan_xfers[2 * k].bits_per_word = bpw;
> +
> +	st->scan_xfers[2 * k + 1].tx_buf = &offload->tx_reset[2];
> +	st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);

"Will passing &offload->tx_reset[2] directly as tx_buf cause DMA mapping
issues, since it is only 2-byte aligned?"

Addressed by the second concern's fix: no sub-aligned pointer into the middle
of a DMA buffer — the full 4-byte scan_tx_reset[] is passed as tx_buf
from its own cache-line-aligned start address.

> +	st->scan_xfers[2 * k + 1].bits_per_word = bpw;
> +	st->scan_xfers[2 * k + 1].cs_change = 1;

The ADI SPI Engine always deasserts CS at the end of its programmed
sequence regardless, though I will drop this in order to avoid confusion.

> +
> +	spi_message_init_with_transfers(&st->scan_msg, st->scan_xfers, 2 * k
> + 2);
> +	st->scan_msg.offload = offload->offload;
> +
> +	ret = spi_optimize_message(spi, &st->scan_msg);
> +	if (ret)
> +		goto err_exit_conversion;
> +
> +	ret = ad4691_sampling_enable(st, true);
> +	if (ret)
> +		goto err_unoptimize;
> +
> +	ret = spi_offload_trigger_enable(offload->offload, offload->trigger,
> &config);

"If the PWM starts generating DATA_READY trigger edges in ad4691_sampling_enable
before the SPI offload trigger is enabled, could the offload engine miss the
first trigger edge?
If that first edge is missed, wouldn't the DATA_READY line remain permanently
asserted (because the STATE_RESET command is never sent), deadlocking the
entire capture?"

Valid. Swapped the order: spi_offload_trigger_enable() first, then
ad4691_sampling_enable(), so the offload engine is armed before the first
CNV pulse fires. Without this, if DATA_READY asserts in the window between
sampling_enable() and trigger_enable(), the SPI Engine misses the rising
edge and the capture deadlocks.

> +	if (ret)
> +		goto err_sampling_disable;
> +
> +	return 0;
> +
> +err_sampling_disable:
> +	ad4691_sampling_enable(st, false);
> +err_unoptimize:
> +	spi_unoptimize_message(&st->scan_msg);
> +err_exit_conversion:
> +	ad4691_exit_conversion_mode(st);
> +	return ret;
> +}
> +
> +static int ad4691_cnv_burst_offload_buffer_predisable(struct iio_dev
> *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	struct ad4691_offload_state *offload = st->offload;
> +	int ret;
> +
> +	spi_offload_trigger_disable(offload->offload, offload->trigger);
> +
> +	ret = ad4691_sampling_enable(st, false);
> +	if (ret)
> +		return ret;
> +
> +	spi_unoptimize_message(&st->scan_msg);

"If ad4691_sampling_enable() fails and returns early, won't this bypass
spi_unoptimize_message() and permanently leak the DMA mappings and memory
allocated during SPI message optimization?"

Valid. spi_unoptimize_message() is now always called before returning
from predisable, regardless of sampling_enable() result.

> +
> +	return ad4691_exit_conversion_mode(st);
> +}
> +
> +static const struct iio_buffer_setup_ops
> ad4691_cnv_burst_offload_buffer_setup_ops = {
> +	.postenable = &ad4691_cnv_burst_offload_buffer_postenable,
> +	.predisable = &ad4691_cnv_burst_offload_buffer_predisable,
> +};
> +
>  static ssize_t sampling_frequency_show(struct device *dev,
>  				       struct device_attribute *attr,
>  				       char *buf)
> @@ -810,6 +1093,9 @@ static ssize_t sampling_frequency_show(struct
> device *dev,
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct ad4691_state *st = iio_priv(indio_dev);
> 
> +	if (st->manual_mode && st->offload)
> +		return sysfs_emit(buf, "%llu\n", st->offload->trigger_hz);

Valid. Using READ_ONCE in show and WRITE_ONCE in store for trigger_hz;
on 32-bit architectures a bare u64 read/write is not atomic.

> +
>  	return sysfs_emit(buf, "%lu\n", NSEC_PER_SEC / st->cnv_period_ns);
>  }
> 
> @@ -829,6 +1115,19 @@ static ssize_t sampling_frequency_store(struct
> device *dev,
>  	if (IIO_DEV_ACQUIRE_FAILED(claim))
>  		return -EBUSY;
> 
> +	if (st->manual_mode && st->offload) {
> +		struct spi_offload_trigger_config config = {
> +			.type = SPI_OFFLOAD_TRIGGER_PERIODIC,
> +			.periodic = { .frequency_hz = freq },
> +		};

"Does this code safely handle zero or negative inputs?
kstrtoint parses freq as a signed int, and the existing validation
(if (!freq) return -EINVAL) is bypassed for the offload path.
A negative value would sign-extend to a massive u64 value when assigned to
frequency_hz."

Addressed per previous patch.

> +
> +		ret = spi_offload_trigger_validate(st->offload->trigger,
> &config);

...

> -	ret = ad4691_setup_triggered_buffer(indio_dev, st);
> +	if (spi_offload) {
> +		indio_dev->channels = st->info->offload_info->channels;
> +		indio_dev->num_channels = st->info->offload_info-
> >num_channels;
> +		ret = ad4691_setup_offload(indio_dev, st, spi_offload);

"The offload path reuses the existing channels array which defines
scan_type.endianness = IIO_BE.
Since the SPI offload DMA RX stream delivers data as native 16-bit words,
which are little-endian on LE platforms, will the IIO subsystem incorrectly
byte-swap the native data before passing it to userspace?"

Agreed. AD4691_CHANNEL no longer sets .endianness, matching the
ad4695 approach and defaulting to IIO_CPU.