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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Mar 20, 2026 at 01:03:58PM +0200, Radu Sabau via B4 Relay wrote:

> 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 4-byte 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 32-bit frames (bits_per_word=32, len=4) for
> DMA word alignment. This patch promotes the channel scan_type from
> storagebits=16 (triggered-buffer path) to storagebits=32 to match the
> DMA word size; the triggered-buffer paths are updated to the same layout
> for consistency. CNV Burst Mode channel data arrives in the lower 16
> bits of the 32-bit word (shift=0); Manual Mode data arrives in the upper
> 16 bits (shift=16), matching the 4-byte SPI transfer layout
> [data_hi, data_lo, 0, 0]. A separate ad4691_manual_channels[] array
> encodes the shift=16 scan type for manual mode.
> 
> Kconfig gains a dependency on IIO_BUFFER_DMAENGINE.

...

> +	struct spi_offload		*offload;
> +	/* SPI offload trigger - periodic (MANUAL) or DATA_READY (CNV_BURST) */
> +	struct spi_offload_trigger	*offload_trigger;
> +	u64				offload_trigger_hz;
> +	struct spi_message		offload_msg;
> +	/* Max 16 channel xfers + 1 state-reset or NOOP */
> +	struct spi_transfer		offload_xfer[17];
> +	u32				offload_tx_cmd[17];
> +	u32				offload_tx_reset;

Ouch! Can you guarantee this kilobytes (isn't it?) of memory will be used in
majority of the cases? When I got comment on replacing a single u8 by unsigned
long in one well used data structure in the kernel I was laughing, but this
single driver may beat the recode of memory waste on the embedded platforms.
Perhaps having a separate structure and allocate it separately when we sure
the offload is supported?

Cc'ed to Wolfram.

...

> +	/* Scan buffer: one slot per channel (u32) plus timestamp */
>  	struct {
> -		u16 vals[16];
> +		u32 vals[16];
>  		s64 ts __aligned(8);

This might break the existing cases or make code ugly and not actually
compatible with u32 layout.

>  	} scan __aligned(IIO_DMA_MINALIGN);

...

> +static int ad4691_cnv_burst_offload_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	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 n_active = hweight_long(*indio_dev->active_scan_mask);

Should be bitmap_weight() with properly given amount of bits.


> +	unsigned int bit, k;
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> +			   (u16)~(*indio_dev->active_scan_mask));

This is not how we work with bitmaps. Use bitmap_read().

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   *indio_dev->active_scan_mask);

Ditto.

> +	if (ret)
> +		return ret;
> +
> +	iio_for_each_active_channel(indio_dev, bit) {
> +		ret = regmap_write(st->regmap, AD4691_ACC_COUNT_LIMIT(bit),
> +				   AD4691_ACC_COUNT_VAL);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = ad4691_enter_conversion_mode(st);
> +	if (ret)
> +		return ret;
> +
> +	memset(st->offload_xfer, 0, sizeof(st->offload_xfer));
> +
> +	/*
> +	 * N transfers to read N AVG_IN registers plus one state-reset
> +	 * transfer (no RX) to re-arm DATA_READY.
> +	 *   TX: [reg_hi | 0x80, reg_lo, 0x00, 0x00]
> +	 *   RX: [0x00, 0x00, data_hi, data_lo]   (shift=0)
> +	 */
> +	k = 0;
> +	iio_for_each_active_channel(indio_dev, bit) {
> +		unsigned int reg = AD4691_AVG_IN(bit);
> +
> +		st->offload_tx_cmd[k] =

> +			cpu_to_be32(((reg >> 8 | 0x80) << 24) |
> +				    ((reg & 0xFF) << 16));

Isn't this is just a cpu_to_be16(0x8000 | reg) ?

> +		st->offload_xfer[k].tx_buf = &st->offload_tx_cmd[k];
> +		st->offload_xfer[k].len = sizeof(u32);
> +		st->offload_xfer[k].bits_per_word = AD4691_OFFLOAD_BITS_PER_WORD;
> +		st->offload_xfer[k].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> +		if (k < n_active - 1)
> +			st->offload_xfer[k].cs_change = 1;
> +		k++;
> +	}
> +
> +	/* State reset to re-arm DATA_READY for the next scan. */
> +	st->offload_tx_reset =
> +		cpu_to_be32(((AD4691_STATE_RESET_REG >> 8) << 24) |
> +			    ((AD4691_STATE_RESET_REG & 0xFF) << 16) |
> +			    (AD4691_STATE_RESET_ALL << 8));

In similar way

		cpu_to_be32((AD4691_STATE_RESET_REG << 16) |
			    (AD4691_STATE_RESET_ALL << 8));

> +	st->offload_xfer[k].tx_buf = &st->offload_tx_reset;
> +	st->offload_xfer[k].len = sizeof(u32);
> +	st->offload_xfer[k].bits_per_word = AD4691_OFFLOAD_BITS_PER_WORD;
> +	k++;
> +
> +	spi_message_init_with_transfers(&st->offload_msg, st->offload_xfer, k);
> +	st->offload_msg.offload = st->offload;
> +
> +	ret = spi_optimize_message(spi, &st->offload_msg);
> +	if (ret)
> +		goto err_exit_conversion;
> +
> +	ret = ad4691_sampling_enable(st, true);
> +	if (ret)
> +		goto err_unoptimize;
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->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->offload_msg);
> +err_exit_conversion:
> +	ad4691_exit_conversion_mode(st);
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko