Message-ID: <5e3a1d31f3cec340650e2e63db79903b78ab9a1f.camel@gmail.com>
Date: Fri, 17 Oct 2025 16:02:24 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
 	devicetree@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Cc: jic23@...nel.org, michael.hennerich@...log.com, nuno.sa@...log.com, 
	eblanc@...libre.com, dlechner@...libre.com, andy@...nel.org,
 robh@...nel.org, 	krzk+dt@...nel.org, conor+dt@...nel.org, corbet@....net, 
	marcelo.schmitt1@...il.com, Trevor Gamblin <tgamblin@...libre.com>, Axel
 Haslam	 <ahaslam@...libre.com>
Subject: Re: [PATCH v5 5/7] iio: adc: ad4030: Add SPI offload support

On Tue, 2025-10-14 at 19:22 -0300, Marcelo Schmitt 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.
> 
> Co-developed-by: Trevor Gamblin <tgamblin@...libre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@...libre.com>
> Co-developed-by: Axel Haslam <ahaslam@...libre.com>
> Signed-off-by: Axel Haslam <ahaslam@...libre.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---

Hi Marcelo,

> Change log v4 -> v5
> - Made Kconfig entry depend on PWM and select other features.
> - Reused ad4030_exit_config_mode() in ad4030_offload_buffer_postenable().
> - Dropped common-mode voltage support on SPI offload setup.
> - Adjusted offload trigger period calculation.
> - No longer setting data frame mode from ad4030_set_avg_frame_len().
> - Rearranged code to reduce patch diff.
> 
>  drivers/iio/adc/Kconfig  |   5 +
>  drivers/iio/adc/ad4030.c | 425 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 416 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index b0580fcefef5..f76df0609b3d 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -60,9 +60,14 @@ config AD4030
>  	tristate "Analog Devices AD4030 ADC Driver"
>  	depends on SPI
>  	depends on GPIOLIB
> +	depends on PWM
>  	select REGMAP
>  	select IIO_BUFFER
> +	select IIO_BUFFER_DMA
> +	select IIO_BUFFER_DMAENGINE
>  	select IIO_TRIGGERED_BUFFER
> +	select SPI_OFFLOAD
> +	select SPI_OFFLOAD_TRIGGER_PWM
>  	help
>  	  Say yes here to build support for Analog Devices AD4030 and AD4630
> high speed
>  	  SPI analog to digital converters (ADC).
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> index b2847fd90271..3df0b593c808 100644
> --- a/drivers/iio/adc/ad4030.c
> +++ b/drivers/iio/adc/ad4030.c
> @@ -14,15 +14,25 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
> +#include <linux/dmaengine.h>
> +#include <linux/iio/buffer-dmaengine.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/limits.h>
> +#include <linux/log2.h>
> +#include <linux/math64.h>
> +#include <linux/minmax.h>
> +#include <linux/pwm.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/spi/offload/consumer.h>
>  #include <linux/spi/spi.h>
>  #include <linux/unaligned.h>
>  #include <linux/units.h>
> +#include <linux/types.h>

...

> 
> +
> +static int ad4030_set_sampling_freq(struct iio_dev *indio_dev, int freq_hz)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	/*
> +	 * We have no control over the sampling frequency without SPI offload
> +	 * triggering.
> +	 */
> +	if (!st->offload_trigger)
> +		return -ENODEV;
> 

Isn't the frequency control only available for offload channels? If I'm not
missing nothing the trigger isn't optional either so I would say the above
should never happen.

> +	if (!in_range(freq_hz, 1, st->chip->max_sample_rate_hz))
> +		return -EINVAL;
> +
> +	return ad4030_update_conversion_rate(st, freq_hz, st->avg_log2);
> +}
> +
>  static int ad4030_set_chan_calibscale(struct iio_dev *indio_dev,
>  				      struct iio_chan_spec const *chan,
>  				      int gain_int,
> @@ -512,11 +643,30 @@ static int ad4030_set_avg_frame_len(struct iio_dev *dev,
> int avg_val)
>  	struct ad4030_state *st = iio_priv(dev);
>  	unsigned int avg_log2 = ilog2(avg_val);
>  	unsigned int last_avg_idx = ARRAY_SIZE(ad4030_average_modes) - 1;
> +	int freq_hz;
>  	int ret;
>  
>  	if (avg_val < 0 || avg_val > ad4030_average_modes[last_avg_idx])
>  		return -EINVAL;
>  
> +	if (st->offload_trigger) {
> +		/*
> +		 * The sample averaging and sampling frequency configurations
> +		 * are mutually dependent one from another. That's because
> the
> +		 * effective data sample rate is fCNV / 2^N, where N is the
> +		 * number of samples being averaged.
> +		 *
> +		 * When SPI offload is supported and we have control over the
> +		 * sample rate, the conversion start signal (CNV) and the SPI
> +		 * offload trigger frequencies must be re-evaluated so data
> is
> +		 * fetched only after 'avg_val' conversions.
> +		 */
> +		ad4030_get_sampling_freq(st, &freq_hz);
> +		ret = ad4030_update_conversion_rate(st, freq_hz, avg_log2);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = regmap_write(st->regmap, AD4030_REG_AVG,
>  			   AD4030_REG_AVG_MASK_AVG_SYNC |
>  			   FIELD_PREP(AD4030_REG_AVG_MASK_AVG_VAL,
> avg_log2));
> @@ -769,6 +919,13 @@ static int ad4030_read_raw_dispatch(struct iio_dev
> *indio_dev,
>  		*val = BIT(st->avg_log2);
>  		return IIO_VAL_INT;
>  
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (!st->offload_trigger)
> +			return -ENODEV;

same

> +
> +		ad4030_get_sampling_freq(st, val);
> +		return IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -809,6 +966,9 @@ static int ad4030_write_raw_dispatch(struct iio_dev
> *indio_dev,
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		return ad4030_set_avg_frame_len(indio_dev, val);
>  
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return ad4030_set_sampling_freq(indio_dev, val);
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -898,6 +1058,104 @@ static const struct iio_buffer_setup_ops
> ad4030_buffer_setup_ops = {
>  	.validate_scan_mask = ad4030_validate_scan_mask,
>  };
>  
> +static void ad4030_prepare_offload_msg(struct iio_dev *indio_dev)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	u8 offload_bpw;
> +
> +	if (st->mode == AD4030_OUT_DATA_MD_30_AVERAGED_DIFF)
> +		offload_bpw = 32;
> +	else
> +		offload_bpw = st->chip->precision_bits;
> +
> +	st->offload_xfer.bits_per_word = offload_bpw;
> +	st->offload_xfer.len = spi_bpw_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);
> +}
> +
> +static int ad4030_offload_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	unsigned int reg_modes;
> +	int ret, ret2;
> +
> +	/*
> +	 * When data from 2 analog input channels is output through a single
> +	 * bus line (interleaved mode (LANE_MD == 0b11)) and gets pushed
> through
> +	 * DMA, extra hardware is required to do the de-interleaving. While
> we
> +	 * don't support such hardware configurations, disallow interleaved
> mode
> +	 * when using SPI offload.
> +	 */
> +	ret = regmap_read(st->regmap, AD4030_REG_MODES, &reg_modes);
> +	if (ret)
> +		return ret;
> +
> +	if (st->chip->num_voltage_inputs > 1 &&
> +	    FIELD_GET(AD4030_REG_MODES_MASK_LANE_MODE, reg_modes) ==
> AD4030_LANE_MD_INTERLEAVED)
> +		return -EINVAL;
> +
> +	ret = ad4030_exit_config_mode(st);
> +	if (ret)
> +		return ret;
> +
> +	ad4030_prepare_offload_msg(indio_dev);
> +	st->offload_msg.offload = st->offload;
> +	ret = spi_optimize_message(st->spi, &st->offload_msg);
> +	if (ret)
> +		goto out_reset_mode;
> +
> +	ret = pwm_set_waveform_might_sleep(st->cnv_trigger, &st->cnv_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;
> +
> +	return 0;
> +
> +out_pwm_disable:
> +	pwm_disable(st->cnv_trigger);
> +out_unoptimize:
> +	spi_unoptimize_message(&st->offload_msg);
> +out_reset_mode:
> +	/* reenter register configuration mode */
> +	ret2 = ad4030_enter_config_mode(st);

nit: if ret2 is not being used at all, maybe just

if (ad4030_enter_config_mode(st))
	
> +	if (ret2)
> +		dev_err(&st->spi->dev,
> +			"couldn't reenter register configuration mode: %d\n",
> +			ret2);
> +
> +	return ret;
> +}
> 

...

> 
>  static int ad4030_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> @@ -1045,24 +1346,61 @@ static int ad4030_probe(struct spi_device *spi)
>  		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
>  				     "Failed to get cnv gpio\n");
>  
> -	/*
> -	 * One hardware channel is split in two software channels when using
> -	 * common byte mode. Add one more channel for the timestamp.
> -	 */
> -	indio_dev->num_channels = 2 * st->chip->num_voltage_inputs + 1;
>  	indio_dev->name = st->chip->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &ad4030_iio_info;
> -	indio_dev->channels = st->chip->channels;
>  	indio_dev->available_scan_masks = st->chip->available_masks;
>  
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> -					      iio_pollfunc_store_time,
> -					      ad4030_trigger_handler,
> -					      &ad4030_buffer_setup_ops);
> -	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "Failed to setup triggered buffer\n");
> +	st->offload = devm_spi_offload_get(dev, spi, &ad4030_offload_config);
> +	ret = PTR_ERR_OR_ZERO(st->offload);
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to get offload\n");
> +
> +	/* Fall back to low speed usage when no SPI offload is available. */
> +	if (ret == -ENODEV) {
> +		/*
> +		 * One hardware channel is split in two software channels
> when
> +		 * using common byte mode. Add one more channel for the
> timestamp.
> +		 */
> +		indio_dev->num_channels = 2 * st->chip->num_voltage_inputs +
> 1;
> +		indio_dev->channels = st->chip->channels;
> +
> +		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +						     
> iio_pollfunc_store_time,
> +						      ad4030_trigger_handler,
> +						     
> &ad4030_buffer_setup_ops);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to setup triggered
> buffer\n");
> +	} else {
> +		/*
> +		 * One hardware channel is split in two software channels
> when
> +		 * using common byte mode. Offloaded SPI transfers can't
> support
> +		 * software timestamp so no additional timestamp channel is
> added.
> +		 */
> +		indio_dev->num_channels = 2 * st->chip->num_voltage_inputs;

Maybe I'm missing something but common mode is not supported for now so isn't
the above wrong?

- Nuno Sá

Powered by blists - more mailing lists