Re: [PATCH v2 6/8] iio: adc: ad4030: Add SPI offload support
From: Jonathan Cameron <jic23@kernel.org>
Date: 2025-09-20 09:43:03
Also in:
linux-devicetree, linux-iio, linux-spi, lkml
On Thu, 18 Sep 2025 14:39:10 -0300 Marcelo Schmitt [off-list ref] 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@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 <redacted> Signed-off-by: Axel Haslam <redacted> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.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]quoted
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]quoted
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
quoted hunk ↗ jump to hunk
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; +}