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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 30 Apr 2026 13:16:46 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add SPI offload support to enable DMA-based, CPU-independent data
> acquisition using the SPI Engine offload framework.
> 
> When an SPI offload is available (devm_spi_offload_get() succeeds),
> the driver registers a DMA engine IIO buffer and uses dedicated buffer
> setup operations. If no offload is available the existing software
> triggered buffer path is used unchanged.
> 
> Both CNV Burst Mode and Manual Mode support offload, but use different
> trigger mechanisms:
> 
> CNV Burst Mode: the SPI Engine is triggered by the ADC's DATA_READY
> signal on the GP pin specified by the trigger-source consumer reference
> in the device tree (one cell = GP pin number 0-3). For this mode the
> driver acts as both an SPI offload consumer (DMA RX stream, message
> optimization) and a trigger source provider: it registers the
> GP/DATA_READY output via devm_spi_offload_trigger_register() so the
> offload framework can match the '#trigger-source-cells' phandle and
> automatically fire the SPI Engine DMA transfer at end-of-conversion.
> 
> Manual Mode: the SPI Engine is triggered by a periodic trigger at
> the configured sampling frequency. The pre-built SPI message uses
> the pipelined CNV-on-CS protocol: N+1 16-bit transfers are issued
> for N active channels (the first result is discarded as garbage from
> the pipeline flush) and the remaining N results are captured by DMA.
> 
> 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.
> 
> Kconfig gains a dependency on IIO_BUFFER_DMAENGINE.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
In general have a read through Sashiko reviews once they come in and
if you agree with them reply to your own patches to say what you are
changing.
https://sashiko.dev/#/patchset/20260430-ad4692-multichannel-sar-adc-driver-v9-0-33e439e4fb87%40analog.com
No perfect but another one in here is something I missed completely.

A few of them called out here but please make sure you've addressed them
all or established them to be false (which happens!)


> +
> +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];
These two buffers share cachelines with trigger_hz. That can
be written via sysfs in the middle of a non coherent DMA transfer.
If it is you may loose the written value when a post transfer
flush copies back a stale cacheline (with the new data in tx_cmd
/ tx_reset).  These need marking to force them to have their own cache lines.


>  };
>  

> +
> +static int ad4691_manual_offload_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	struct ad4691_offload_state *offload = st->offload;
> +
> +	spi_offload_trigger_disable(offload->offload, offload->trigger);
> +	spi_unoptimize_message(&st->scan_msg);
> +
> +	return ad4691_exit_conversion_mode(st);
> +}
> +
> +static const struct iio_buffer_setup_ops ad4691_manual_offload_buffer_setup_ops = {
> +	.postenable = &ad4691_manual_offload_buffer_postenable,
> +	.predisable = &ad4691_manual_offload_buffer_predisable,
> +};
> +
> +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);
> +	struct spi_offload_trigger_config config = {
> +		.type = SPI_OFFLOAD_TRIGGER_DATA_READY,
> +	};
> +	unsigned int bpw = indio_dev->channels[0].scan_type.realbits;
> +	unsigned int acc_mask;
> +	unsigned int bit, k;
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   bitmap_read(indio_dev->active_scan_mask, 0,
> +				       iio_get_masklength(indio_dev)));
> +	if (ret)
> +		return ret;
> +
> +	acc_mask = ~bitmap_read(indio_dev->active_scan_mask, 0,
> +				iio_get_masklength(indio_dev)) & GENMASK(15, 0);
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG, acc_mask);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4691_enter_conversion_mode(st);
> +	if (ret)
> +		return ret;
> +
> +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> +
> +	/*
> +	 * Each AVG_IN register read uses two 16-bit transfers:
> +	 *   TX: [reg_hi | 0x80, reg_lo]  (address, CS stays asserted)
> +	 *   RX: [data_hi, data_lo]       (data, storagebits=16, shift=0)
> +	 * The state reset is also split into two 16-bit transfers
> +	 * (address then value) to keep bits_per_word uniform throughout.
> +	 */
> +	k = 0;
> +	iio_for_each_active_channel(indio_dev, bit) {
> +		put_unaligned_be16(0x8000 | AD4691_AVG_IN(bit), offload->tx_cmd[k]);

Sashiko makes the interesting point that you are setting bpw to 16 at least sometimes
and that means the SPI bus will be dealing in u16s. As such the endian swap seems
odd.  Which also makes a mess if the word length isn't 16 bits. I'm assuming it
always is. If that's the case hard code it rather than giving impression this
will work otherwise.  The endian swap probably wants to be unconditional rather
than only if little endian host.

There is a related question whether the larger words interact with the channel
endianness marking. I think that should be fine but please check.


> +
> +		/* 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;
> +
> +		/* 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]);
> +	st->scan_xfers[2 * k + 1].bits_per_word = bpw;
> +	st->scan_xfers[2 * k + 1].cs_change = 1;
This is odd. Add a comment on why if you do want to leave CS active
after this sequence.

> +
> +	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);
There are some questions from Sashiko around whether the offload trigger should be enabled
first to avoid getting the device into an odd state. I haven't read the datasheet
in enough depth to check - so over to you to argue one way or the other!

> +	if (ret)
> +		goto err_unoptimize;
> +
> +	ret = spi_offload_trigger_enable(offload->offload, offload->trigger, &config);
> +	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;
> +}