Message-ID: <aNK8ZZu74mK0_ygB@debian-BULLSEYE-live-builder-AMD64>
Date: Tue, 23 Sep 2025 12:27:33 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
	jic23@...nel.org, michael.hennerich@...log.com, nuno.sa@...log.com,
	eblanc@...libre.com, andy@...nel.org, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, corbet@....net,
	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

Hi David, thanks for the insightful review.

On 09/22, David Lechner wrote:
> On 9/18/25 12:39 PM, 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.
> 
> I tried testing this with AD4630-24 but didn't have luck in actually
> capturing data. I'm 100% sure the problem is with the FPGA. And the
> evaluation board doesn't have any place to attach a logic analyzer for
> debugging. That means that I wasn't able to reliabably test this code
> yet. But I don't expect my problems to be solved any time soon, so I
> don't want to let that hold up progress. I would have really liked to
> have been able to see the actual timings over the wire to make sure
> we got all of that correct.
> 
Even if you hook up probes to the SPI lines, you might not be able to logic
analyze the transfers at frequencies like 100 MHz or even at 80 MHz unless you
have a very fast logic analyzer or oscilloscope. To debug these signals we
usually change the HDL verilog to add ILA debug cores to record the signals on
the FPGA. I'll see if I can get or build the project with those ILA cores set.
Another thing is getting the correct combination of HDL + device tree because
we have a few possible HDL build configurations (for number of lanes, clock mode,
DDR/DTR, and capture zone) and the device tree must be coherent with what runs
on the FPGA. I'll send you some of boot files I was using during my tests.

> > ---
...
> > [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.
> 
> I don't follow. The "REG" in AD4030_SPI_MAX_REG_XFER_SPEED stands for
> "register". The actual max speed for reading sample data should be coming
> from the devicetree since it is faster and depends on the wiring and VIO
> voltage. It could be as much as 102 MHz.
> 
I have finally I noticed the SPI compatible mode timings. Sure, will adapt to
use faster sample rate when possible.


> Unrelated to this series, I still think 80 MHz is faster that it needs
> to be for AD4030_SPI_MAX_REG_XFER_SPEED. It is fine to do them slower,
> e.g. at 10 MHz to reduce the risk of errors and also makes it easier to
> debug using a logic analyzer.

Sure, will do that.

> 
> > 
> >  drivers/iio/adc/Kconfig  |   3 +
> >  drivers/iio/adc/ad4030.c | 485 +++++++++++++++++++++++++++++++++++----
> >  2 files changed, 445 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 58a14e6833f6..2a44fcaccf54 100644
...
> > +	cnv_wf.period_length_ns = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq);
> > +	/*
> > +	 * The datasheet lists a minimum time of 9.8 ns, but no maximum. If the
> > +	 * rounded PWM's value is less than 10, increase the target value by 10
> > +	 * and attempt to round the waveform again, until the value is at least
> > +	 * 10 ns. Use a separate variable to represent the target in case the
> > +	 * rounding is severe enough to keep putting the first few results under
> > +	 * the minimum 10ns condition checked by the while loop.
> > +	 */
> > +	do {
> > +		cnv_wf.duty_length_ns = target;
> > +		ret = pwm_round_waveform_might_sleep(st->cnv_trigger, &cnv_wf);
> > +		if (ret)
> > +			return ret;
> > +		target += AD4030_TCNVH_NS;
> > +	} while (cnv_wf.duty_length_ns < AD4030_TCNVH_NS);
> > +
> > +	if (!in_range(cnv_wf.period_length_ns, AD4030_TCYC_NS, INT_MAX))
> > +		return -EINVAL;
> 
> I hit this error during testing with the default max_sample_rate_hz assigned
> in probe. We could have a loop for this too to try to get the closest valid
> period rather than erroring if the exact value isn't available.
> 
Yes, this makes sense. Though, looping to try to get a suitable period wouldn't
potentially also change the duty_length we settled above?

> > +
> > +	offload_period_ns = cnv_wf.period_length_ns;
> > +	if (st->mode == AD4030_OUT_DATA_MD_30_AVERAGED_DIFF)
> 
...
> > +static int ad4030_set_sampling_freq(struct iio_dev *indio_dev, int freq)
> > +{
> > +	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;
> > +
> > +	if (!in_range(freq, 1, st->chip->max_sample_rate_hz))
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&st->lock);
> 
> Why not iio_device_claim_direct() instead of a new lock? We wouldn't
> want to change the sampling frequency during a buffered read anyway.
> This driver already uses iio_device_claim_direct() to protect other
> register access.

The new lock is to protect concurrent updates of the oversampling and sampling
frequency. Since, oversampling and the sampling frequency properties are
mutually dependent one from another, a simultaneous write to those attributes
could lead to an invalid oversamp + samp freq configuration.

> 
> > +	return __ad4030_set_sampling_freq(st, freq, st->avg_log2);
> > +}
> > +
...
> > +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.speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED;
> 
> As mentioned at the beginning, drop this line and let it use the max
> speed from the devicetree.
> 
> > +	st->offload_xfer.bits_per_word = roundup_pow_of_two(offload_bpw);
> 
> Why roundup_pow_of_two()? The SPI controller can do 24 bits per word.
> And if we are reading both a 24-bit value and the common mode voltage,
> this would cause both to be read in 1 word.
> 
> Speaking of which, I think this will need a possible second xfer with
> bpw=8 if we want to read the common mode voltage.
> 
> Or, if the intention was to not allow it, we need different scan masks.
> But I don't see a reason why we could not allow it.
> 
Nothing says we couldn't support offloading transfers with
differential + common-mode data, at least in theory. So, I didn't felt like it
should be prevented. Though, offloading differential + common-mode data is
a configuration I couldn't really test with ADAQ4216 because the HDL is ... peculiar.


> Or, if this is making a assumptions about extra hardware being present
> to move bits around between reading them over the SPI bus and pushing the
> values to DMA, then there should be some comments about that. More on that
> below.
> 
> > +	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_spi_offload_setup(struct iio_dev *indio_dev,
> > +				    struct ad4030_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	struct dma_chan *rx_dma;
> > +
> > +	indio_dev->setup_ops = &ad4030_offload_buffer_setup_ops;
> > +
> > +	st->offload_trigger = devm_spi_offload_trigger_get(dev, st->offload,
> > +							   SPI_OFFLOAD_TRIGGER_PERIODIC);
> > +	if (IS_ERR(st->offload_trigger))
> > +		return dev_err_probe(dev, PTR_ERR(st->offload_trigger),
> > +				     "failed to get offload trigger\n");
> > +
> > +	st->offload_trigger_config.type = SPI_OFFLOAD_TRIGGER_PERIODIC;
> 
> If we want to be really strict/generic here, we should not be allowing
> chips with num_voltage_inputs == 2 and a single SPI bus/deserializer (i.e.
> channel data is interleaved). In this case, extra hardware is required
> to do the de-interleaving (i.e. the spi_axis_reorder IP block).

By channel data is interleaved you mean data from both channels going out on
SDO0 (LANE_MD == 0b11)? In that case, yes, I think so. Only the ADC driver would
know about data being interleaved and it would not be able to descramble it when
data gets pushed up through DMA.

> 
> We could take the easy way out and just always assume that is there.
> In that case, we should makes some comments here about such assumptions.
> 
> Or we could actually describe it properly in the devicetree and check
> for that here. This came up during the discussions when I was upstreaming
> SPI offload support. It would look something like this...
> 
> In the devicetree, instead of having the DMA connected to the SPI controller,
> we now have a separate IP block with it's own node between them.
> 
> /* spi_axis_reorder IP block */
> reorder: offload-stream-sink@...0000 {
> 	compatible = "adi,axi-spi-reorder";
> 	reg = <0x4000000 0x1000>;
> 	clocks = <&spi_clk>;
> 	dmas = <&adc_dma>;
> };
> 
> spi@...0000 {
> 	compatible = "adi,axi-spi-engine-1.00.a
> 	reg = <0x4000000 0x1000>;
> 	clocks = <&clkc 15>, <&spi_clk>;
> 	clock-name "s_axi_aclk", "spi_clk";
> 
> 	trigger-sources = <&pwm_trigger>;
> 	offload-streams = <&reorder>;
> 	offload-stream-names = "offload0-rx";
> 
> 	...
> };
> 
> Then here in the driver, we would need a different (non-existing)
> API to get the DMA from this offload-stream rather than calling
> devm_spi_offload_rx_stream_request_dma_chan(). Or extend the SPI
> controller to handle that.
> 
> Or 3rd option: If easy way is not acceptable and "right way" is too much
> work, we could just return error here for num_voltage_inputs == 2 until
> we add support for SPI controllers with two buses/deserializers.
> 
3rd option sounds more reasonable for the moment.
I think this might, alternatively, be supported as something associated with
spi-rx/tx-bus-width dt property. The question we seem to be trying to answer is,
how data coming from a multi-line bus is expected to be delivered?


> > +
> > +	rx_dma = devm_spi_offload_rx_stream_request_dma_chan(dev, st->offload);
> > +	if (IS_ERR(rx_dma))
> > +		return dev_err_probe(dev, PTR_ERR(rx_dma),
> > +				     "failed to get offload RX DMA\n");
> > +
> > +	return devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev, rx_dma,
> > +							   IIO_BUFFER_DIRECTION_IN);
> > +}
> > +

Thanks,
Marcelo

Powered by blists - more mailing lists