Re: [PATCH v9 5/8] iio: adc: ad4030: Add SPI offload support

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Feb 2026 12:00:39 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> AD4030 and similar ADCs can capture data at sample rates up to 2 mega
> samples per second (MSPS). Not all SPI controllers are able to achieve such
> high throughputs and even when the controller is fast enough to run
> transfers at the required speed, it may be costly to the CPU to handle
> transfer data at such high sample rates. Add SPI offload support for AD4030
> and similar ADCs to enable data capture at maximum sample rates.
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Co-developed-by: Trevor Gamblin <tgamblin@baylibre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> Co-developed-by: Axel Haslam <ahaslam@baylibre.com>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

Hi. One really small question on ordering inline. The other thing is mostly
me expressing surprise around the PWM handling being necessary rather
than any request to change anything.

Thanks,

Jonathan

> @@ -971,6 +1205,24 @@ static int ad4030_detect_chip_info(const struct ad4030_state *st)
>  	return 0;
>  }
>  
> +static int ad4030_pwm_get(struct ad4030_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +
> +	st->cnv_trigger = devm_pwm_get(dev, NULL);
> +	if (IS_ERR(st->cnv_trigger))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_trigger),
> +				     "Failed to get CNV PWM\n");
> +
> +	/*
> +	 * Preemptively disable the PWM, since we only want to enable it with
> +	 * the buffer.
> +	 */
> +	pwm_disable(st->cnv_trigger);

Feels like there should really be a way to get a pwm disabled in one call
so there isn't an edge case of it being on briefly.
I'm a bit surprised it defaults to on.  I guess this is because DT can provide
the parameters?


> +
> +	return 0;
> +}




> +static const struct iio_scan_type ad4030_24_offload_scan_types[] = {
> +	[AD4030_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 24,

Really trivial, but why this order?  To me keeping to the
order of the fields in the structure definition makes a tiny
bit more sense here.   So realbits, then storagebits, then shift.

> +		.shift = 0,
> +		.endianness = IIO_CPU,
> +	},
> +	[AD4030_SCAN_TYPE_AVG] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 30,
> +		.shift = 2,
> +		.endianness = IIO_CPU,
> +	},
> +};