Message-ID: <20250920104251.3f7dcbb2@jic23-huawei>
Date: Sat, 20 Sep 2025 10:42:51 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-doc@...r.kernel.org>, <linux-spi@...r.kernel.org>,
 <linux-kernel@...r.kernel.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>,
 Sergiu Cuciurean <sergiu.cuciurean@...log.com>, Trevor Gamblin
 <tgamblin@...libre.com>, Axel Haslam <ahaslam@...libre.com>
Subject: Re: [PATCH v2 6/8] iio: adc: ad4030: Add SPI offload support

On Thu, 18 Sep 2025 14:39:10 -0300
Marcelo Schmitt <marcelo.schmitt@...log.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.
> 
> Co-developed-by: Sergiu Cuciurean <sergiu.cuciurean@...log.com>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@...log.com>
> Co-developed-by: Nuno Sa <nuno.sa@...log.com>
> Signed-off-by: Nuno Sa <nuno.sa@...log.com>
> 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>
> ---
> Most of the code for SPI offload support is based on work from Sergiu Cuciurean,
> Nuno Sa, Axel Haslam, and Trevor Gamblin. Thus, this patch comes with many
> co-developed-by tags. I also draw inspiration from other drivers supporting SPI
> offload, many of them written by David Lechner.
> 
> Change log v1 -> v2
> - Dropped all clock-modes and DDR related stuff for now as those will require
>   further changes to the SPI subsystem or to SPI controller drivers.
> - Update the modes register with proper output data mode bits when sample
>   averaging (oversampling_ratio) is set.
> - Lock on device state mutex before updating oversampling and sampling frequency.
> - Made sampling_frequency shared by all channels.
> - Better checking the requested sampling frequency is valid.
> - Adjusted to SPI offload data capture preparation and stop procedures.
> - Error out if try to get/set sampling frequency without offload trigger.
> - Depend on PWM so build always succeed.
> - Drop unmatched/unbalanced call to iio_device_release_direct().
> - No longer shadowing error codes.
> 
> Suggestions to v1 that I did not comply to:
> [SPI]
> > I would be tempted to put the loop check here [in drivers/spi/spi-offload-trigger-pwm.c]:
> > 
> > 	offload_offset_ns = periodic->offset_ns;
> > 
> > 	do {
> > 		wf.offset_ns = offload_offset_ns;
> > 		ret = pwm_round_waveform_might_sleep(st->pwm, &wf);
> > 		if (ret)
> > 			return ret;
> > 		offload_offset_ns += 10;
> > 
> > 	} while (wf.offset_ns < periodic->offset_ns);
> > 
> > 	wf.duty_offset_ns = periodic->offset_ns;
> > 
> > instead of in the ADC driver so that all future callers don't have to
> > repeat this.  
> 
> Not sure implementing the PWM trigger phase approximation/rounding/setup within
> spi-offload-trigger-pwm is actually desirable. The PWM phase
> approximation/rounding/setup done in AD4030 iterates over the configuration of a
> second PWM (the PWM connected to the CNV pin). I haven't seen any other device
> that would use such double PWM setup schema so pushing an additional argument to
> spi_offload_trigger_pwm_validate() doesn't seem worth it.
> 
> [IIO]
> > Why using slower speed for offload?  
> Looks like it's the same max speed for both register access and data sample.
> So, just reusing the existing define for the max transfer speed.
> 
>  drivers/iio/adc/Kconfig  |   3 +
>  drivers/iio/adc/ad4030.c | 485 +++++++++++++++++++++++++++++++++++----
>  2 files changed, 445 insertions(+), 43 deletions(-)
Hi Marcelo

Just one thing I noticed today.  If nothing else comes up I can fix that
up whilst applying.  However, this will benefit from review from others
+ the IIO tree is effectively closed for this cycle so we have lots of time
to tidy up any remaining stuff.

Thanks,

Jonathan
 
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> index aa0e27321869..52805c779934 100644
> --- a/drivers/iio/adc/ad4030.c
> +++ b/drivers/iio/adc/ad4030.c

> +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;
> +
> +	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 */
> +	ret = ad4030_enter_config_mode(st);

This blows away the original error.  I'd do something like
	if (ad40303_enter_config_mode(st))
		dev_err(...)

	return ret;
so we preserve whatever went wrong first.


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

Powered by blists - more mailing lists