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

[David Lechner <dlechner@baylibre.com>]

On 4/9/26 10:28 AM, Radu Sabau via B4 Relay 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>
> ---
>  drivers/iio/adc/Kconfig  |   2 +
>  drivers/iio/adc/ad4691.c | 398 ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 395 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 3e5caa0972eb..839ea7f44c78 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
> @@ -22,6 +22,8 @@
>  #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>
>  #include <linux/units.h>
>  #include <linux/unaligned.h>
>  
> @@ -43,6 +45,11 @@
>  
>  #define AD4691_CNV_DUTY_CYCLE_NS		380
>  #define AD4691_CNV_HIGH_TIME_NS			430
> +/*
> + * Conservative default for the manual offload periodic trigger. Low enough
> + * to work safely out of the box across all OSR and channel count combinations.
> + */
> +#define AD4691_OFFLOAD_INITIAL_TRIGGER_HZ	(100 * HZ_PER_KHZ)
>  
>  #define AD4691_SPI_CONFIG_A_REG			0x000
>  #define AD4691_SW_RESET				(BIT(7) | BIT(0))
> @@ -95,6 +102,8 @@
>  #define AD4691_ACC_IN(n)			(0x252 + (3 * (n)))
>  #define AD4691_ACC_STS_DATA(n)			(0x283 + (4 * (n)))
>  
> +#define AD4691_OFFLOAD_BITS_PER_WORD		16

This is just the same as realbits in scan info. So could use that
directly instead.

> +
>  static const char * const ad4691_supplies[] = { "avdd", "vio" };
>  
>  enum ad4691_ref_ctrl {
> @@ -114,6 +123,7 @@ struct ad4691_chip_info {
>  	const char *name;
>  	unsigned int max_rate;
>  	const struct ad4691_channel_info *sw_info;
> +	const struct ad4691_channel_info *offload_info;
>  };
>  
>  #define AD4691_CHANNEL(ch)						\
> @@ -177,6 +187,18 @@ static const struct ad4691_channel_info ad4693_sw_info = {
>  	.num_channels = ARRAY_SIZE(ad4693_channels),
>  };
>  
> +static const struct ad4691_channel_info ad4691_offload_info = {
> +	.channels = ad4691_channels,
> +	/* No soft timestamp; num_channels caps access to 16. */
> +	.num_channels = 16,

`ARRAY_SIZE(ad4691_channels) - 1` would make sense too.

> +};
> +
> +static const struct ad4691_channel_info ad4693_offload_info = {
> +	.channels = ad4693_channels,
> +	/* No soft timestamp; num_channels caps access to 8. */
> +	.num_channels = 8,
> +};
> +
>  /*
>   * Internal oscillator frequency table. Index is the OSC_FREQ_REG[3:0] value.
>   * Index 0 (1 MHz) is only valid for AD4692/AD4694; AD4691/AD4693 support
> @@ -207,24 +229,36 @@ static const struct ad4691_chip_info ad4691_chip_info = {
>  	.name = "ad4691",
>  	.max_rate = 500 * HZ_PER_KHZ,
>  	.sw_info = &ad4691_sw_info,
> +	.offload_info = &ad4691_offload_info,
>  };
>  
>  static const struct ad4691_chip_info ad4692_chip_info = {
>  	.name = "ad4692",
>  	.max_rate = 1 * HZ_PER_MHZ,
>  	.sw_info = &ad4691_sw_info,
> +	.offload_info = &ad4691_offload_info,
>  };
>  
>  static const struct ad4691_chip_info ad4693_chip_info = {
>  	.name = "ad4693",
>  	.max_rate = 500 * HZ_PER_KHZ,
>  	.sw_info = &ad4693_sw_info,
> +	.offload_info = &ad4693_offload_info,
>  };
>  
>  static const struct ad4691_chip_info ad4694_chip_info = {
>  	.name = "ad4694",
>  	.max_rate = 1 * HZ_PER_MHZ,
>  	.sw_info = &ad4693_sw_info,
> +	.offload_info = &ad4693_offload_info,
> +};
> +
> +struct ad4691_offload_state {
> +	struct spi_offload *spi;

I would call this "offload" or "instance". "spi" is usally the SPI
device handle.

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

...

> +
> +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->spi, offload->trigger);
> +
> +	ret = ad4691_sampling_enable(st, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   AD4691_SEQ_ALL_CHANNELS_OFF);

Why this extra step? We don't have it when unwinding in the
error path of the postenable function.

> +	if (ret)
> +		return ret;
> +
> +	spi_unoptimize_message(&st->scan_msg);
> +
> +	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)