Re: [PATCH 07/15] iio: adc: ad4030: Add SPI offload support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 29 Aug 2025 21:42:50 -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 so to enable ADC data capture at maximum sample
> rates.
> 
> Cc: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Cc: Nuno Sa <nuno.sa@analog.com>
> Cc: Trevor Gamblin <tgamblin@baylibre.com>
> Cc: Axel Haslam <ahaslam@baylibre.com>
> Cc: David Lechner <dlechner@baylibre.com>
> Co-developed-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Co-developed-by: Nuno Sa <nuno.sa@analog.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.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>
> ---
> Most of the code in this patch is based on work from Sergiu Cuciurean, Nuno Sa,
> Axel Haslam, and Trevor Gamblin, hence the many co-developed-by tags. I also
> draw inspiration from other drivers supporting SPI offload, many of them written
> by David Lechner.

A few things inline. 

> +
> +static int ad4030_set_sampling_freq(struct iio_dev *indio_dev, unsigned int freq)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (PTR_ERR_OR_ZERO(st->offload))
> +		return -EINVAL;
> +
> +	if (!freq || freq > st->chip->max_sample_rate_hz)
> +		return -EINVAL;
> +
> +	ret = __ad4030_set_sampling_freq(st, freq);
> +	iio_device_release_direct(indio_dev);
Where is the claim?
> +
> +	return ret;
> +}


>  
>  static int ad4030_update_scan_mode(struct iio_dev *indio_dev,
> @@ -903,6 +1038,67 @@ static const struct iio_buffer_setup_ops ad4030_buffer_setup_ops = {
>  	.validate_scan_mask = ad4030_validate_scan_mask,
>  };
>  
> +static int ad4030_offload_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4030_REG_EXIT_CFG_MODE, BIT(0));
> +	if (ret)
> +		return ret;
> +
> +	st->offload_msg.offload = st->offload;
> +	ret = spi_optimize_message(st->spi, &st->offload_msg);
> +	if (ret < 0)
> +		goto out_reset_mode;
> +
> +	ret = pwm_set_waveform_might_sleep(st->conv_trigger, &st->conv_wf, false);
> +	if (ret)
> +		goto out_unoptimize;
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &st->offload_trigger_config);
> +	if (ret)
> +		goto out_pwm_disable;
Blank line here.

> +	return 0;
Blank line here.

> +out_pwm_disable:
> +	pwm_disable(st->conv_trigger);
> +out_unoptimize:
> +	spi_unoptimize_message(&st->offload_msg);
> +out_reset_mode:
> +	/* reenter register configuration mode */
> +	ret = ad4030_enter_config_mode(st);
> +	if (ret)
> +		dev_warn(&st->spi->dev,
> +			 "couldn't reenter register configuration mode\n");
> +	return ret;
> +}

> +
> +static void ad4030_prepare_offload_msg(struct ad4030_state *st)
> +{
> +	u8 data_width = st->chip->precision_bits;
> +	u8 offload_bpw;
> +
> +	if (st->lane_mode == AD4030_LANE_MD_INTERLEAVED)
> +		/*
> +		 * This means all channels on 1 lane.
> +		 */

Single line comment looks like enough here.

> +		offload_bpw = data_width * st->chip->num_voltage_inputs;
> +	else
> +		offload_bpw  = data_width;
> +
> +	st->offload_xfer.speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED;
> +	st->offload_xfer.bits_per_word = offload_bpw;
> +	st->offload_xfer.len = roundup_pow_of_two(BITS_TO_BYTES(offload_bpw));
> +	st->offload_xfer.offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> +}


> @@ -1103,6 +1393,20 @@ static const struct iio_scan_type ad4030_24_scan_types[] = {
>  		.shift = 2,
>  		.endianness = IIO_BE,
>  	},
> +	[AD4030_OFFLOAD_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 24,
> +		.shift = 0,
> +		.endianness = IIO_CPU,
> +	},
> +	[AD4030_OFFLOAD_SCAN_TYPE_AVG] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 30,
> +		.shift = 2,
> +		.endianness = IIO_CPU,
> +	},
>  };
>  
>  static const struct iio_scan_type ad4030_16_scan_types[] = {
> @@ -1119,7 +1423,21 @@ static const struct iio_scan_type ad4030_16_scan_types[] = {
>  		.realbits = 30,
>  		.shift = 2,
>  		.endianness = IIO_BE,
> -	}
> +	},
> +	[AD4030_OFFLOAD_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 16,
> +		.shift = 0,
> +		.endianness = IIO_CPU,
> +	},
> +	[AD4030_OFFLOAD_SCAN_TYPE_AVG] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 30,
> +		.shift = 2,
> +		.endianness = IIO_CPU,
> +	},
>  };
>  
>  static const struct ad4030_chip_info ad4030_24_chip_info = {
> @@ -1130,10 +1448,15 @@ static const struct ad4030_chip_info ad4030_24_chip_info = {
>  		AD4030_CHAN_CMO(1, 0),
>  		IIO_CHAN_SOFT_TIMESTAMP(2),
>  	},
> +	.offload_channels = {
> +		AD4030_OFFLOAD_CHAN_DIFF(0, ad4030_24_scan_types),

This array still has the non offload cases.  Do they make sense?

> +		AD4030_CHAN_CMO(1, 0),
> +	},
>  	.grade = AD4030_REG_CHIP_GRADE_AD4030_24_GRADE,
>  	.precision_bits = 24,
>  	.num_voltage_inputs = 1,
>  	.tcyc_ns = AD4030_TCYC_ADJUSTED_NS,
> +	.max_sample_rate_hz = 2 * MEGA,
>  };