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

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

On Thu, Apr 30, 2026 at 01:16:46PM +0300, 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 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.

Unneeded detail in the commit message. It's visible from the patch.
You probably wanted to explain "why?" it's required.

...

>  #include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/spi/spi.h>
> +#include <linux/spi/offload/consumer.h>
> +#include <linux/spi/offload/provider.h>

You missed at some point types.h (in this patch or earlier).

>  #include <linux/units.h>
>  #include <linux/unaligned.h>

...

> -	/* max 16 + 1 NOOP (manual) or 2*16 + 2 (CNV burst). */
> +	/*
> +	 * Non-offload: max 16 + 1 NOOP (manual) or 2*16 + 2 (CNV burst).
> +	 * Offload reuses this array — both paths are mutually exclusive.
> +	 */

Can you make sure this will be in the end result from the beginning, id est
in the previous change put it as

	/*
	 * Non-offload: max 16 + 1 NOOP (manual) or 2*16 + 2 (CNV burst).
	 * Offload is not supported.
	 */

OR

	/*
	 * Non-offload: max 16 + 1 NOOP (manual) or 2*16 + 2 (CNV burst).
	 */

OR

	/*
	 * max 16 + 1 NOOP (manual) or 2*16 + 2 (CNV burst).
	 */

This will minimize the churn.

>  	struct spi_transfer scan_xfers[34];

...

> +static bool ad4691_offload_trigger_match(struct spi_offload_trigger *trigger,
> +					 enum spi_offload_trigger_type type,
> +					 u64 *args, u32 nargs)
> +{
> +	return type == SPI_OFFLOAD_TRIGGER_DATA_READY &&
> +	       nargs == 1 && args[0] <= 3;

Seems there is a room in the previous line for more.

> +}

...

> -	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> -			   ~bitmap_read(indio_dev->active_scan_mask, 0,
> -				iio_get_masklength(indio_dev)) & GENMASK(15, 0));
> +	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);

Make it like this in the previous patch.

>  	if (ret)
>  		goto err_unoptimize;

...

> +	offload = devm_kzalloc(dev, sizeof(*offload), GFP_KERNEL);

Do you have device/devres.h already included (either explicitly or via device.h
or platform_device.h)?

> +	if (!offload)
> +		return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko