Re: [PATCH v3 1/4] iio: adc: ad4000: Add support for SPI offload

[Marcelo Schmitt]

Hi David, thank you for your review.
Hope I don't sound harsh on my reply.
Comments inline.

Thanks

On 03/27, David Lechner wrote:
> On 3/26/25 8:24 AM, Marcelo Schmitt wrote:
> > FPGA HDL projects can include a PWM generator in addition to SPI-Engine.
> > The PWM IP is used to trigger SPI-Engine offload modules that in turn set
> > SPI-Engine to execute transfers to poll data from the ADC. That allows data
> > to be read at the maximum sample rates. Also, it is possible to set a
> > specific sample rate by setting the proper PWM duty cycle and related state
> > parameters, thus allowing an adjustable ADC sample rate when a PWM (offload
> > trigger) is used in combination with SPI-Engine.
> > 
> > Add support for SPI offload.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> > ---
> 
...
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/spi/spi.h>
> > +#include <linux/spi/offload/consumer.h>
> 
> Alphabetical order?

Ah drat, yes. Hope I've put that in the correct order now.

> 
> >  #include <linux/units.h>
> >  #include <linux/util_macros.h>
> >  #include <linux/iio/iio.h>
> >  
> >  #include <linux/iio/buffer.h>
> > +#include <linux/iio/buffer-dmaengine.h>
> >  #include <linux/iio/triggered_buffer.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  
Also changing to
...
#include <linux/units.h>
#include <linux/util_macros.h>

#include <linux/iio/iio.h>
#include <linux/iio/buffer.h>
#include <linux/iio/buffer-dmaengine.h>
...

> 
> ...
> 
> >  
> > +/*
> > + * When SPI offload is configured, transfers are executed withouth CPU
> 
> s/withouth/without/
Ack

> > + * intervention so no soft timestamp can be recorded when transfers run.
> > + * Because of that, the macros that set timestamp channel are only used when
> > + * transfers are not offloaded.
> > + */
...
> > @@ -784,7 +1081,10 @@ static int ad4000_probe(struct spi_device *spi)
> >  	switch (st->sdi_pin) {
> >  	case AD4000_SDI_MOSI:
> >  		indio_dev->info = &ad4000_reg_access_info;
> > -		indio_dev->channels = chip->reg_access_chan_spec;
> > +
> > +		/* Set CNV/CS high time for when turbo mode is used */
> > +		spi->cs_inactive.value = st->time_spec->t_quiet1_ns;
> > +		spi->cs_inactive.unit = SPI_DELAY_UNIT_NSECS;
> 
> This code path later calls ad4000_prepare_3wire_mode_message() which sets:
> 
> 	xfers[0].cs_change_delay.value = st->time_spec->t_conv_ns;
> 
> Which contradicts/overrides this.

Oof, good point. Though, it's probably not a problem if single-shot transfers
use a longer delay, right? Would that also override cs_inactive or be of any
trouble for SPI controllers?

...
> >  
> > +		if (st->using_offload) {
> > +			indio_dev->channels = &chip->reg_access_offload_chan_spec;
> > +			indio_dev->num_channels = 1;
> > +			ret = ad4000_prepare_offload_message(st, indio_dev->channels);
> > +			if (ret)
> > +				return dev_err_probe(dev, ret,
> > +						     "Failed to optimize SPI msg\n");
> > +		} else {
> > +			indio_dev->channels = chip->reg_access_chan_spec;
> > +			indio_dev->num_channels = ARRAY_SIZE(chip->reg_access_chan_spec);
> > +		}
> > +
> > +		/*
> > +		 * Call ad4000_prepare_3wire_mode_message() so single-shot read
> > +		 * SPI messages are always initialized.
> > +		 */
> >  		ret = ad4000_prepare_3wire_mode_message(st, &indio_dev->channels[0]);
> >  		if (ret)
> > -			return ret;
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to optimize SPI msg\n");
> >  
> >  		ret = ad4000_config(st);
> >  		if (ret < 0)
> > @@ -806,19 +1123,47 @@ static int ad4000_probe(struct spi_device *spi)
> >  
> >  		break;
> >  	case AD4000_SDI_VIO:
> > -		indio_dev->info = &ad4000_info;
> > -		indio_dev->channels = chip->chan_spec;
> > +		if (st->using_offload) {
> > +			indio_dev->info = &ad4000_offload_info;
> > +			indio_dev->channels = &chip->offload_chan_spec;
> > +			indio_dev->num_channels = 1;
> > +
> > +			/* Set CNV/CS high time for when turbo mode is not used */
> > +			if (!st->cnv_gpio) {
> > +				spi->cs_inactive.value = st->time_spec->t_conv_ns;
> > +				spi->cs_inactive.unit = SPI_DELAY_UNIT_NSECS;
> 
> I'm still not sold on this. We know it has no effect with AXI SPI Engine and
> it is writing over something that usually comes from DT. It is misleading.

I thought it was okay to set cs_inactive and call spi_setup() from the field
doc in include/linux/spi/spi.h.

	set_cs_timing() method is for SPI controllers that supports
	configuring CS timing.
	
	This hook allows SPI client drivers to request SPI controllers
	to configure specific CS timing through spi_set_cs_timing() after
	spi_setup().

Would it be better to set spi-cs-inactive-delay-ns in ADC dt node?
Or it still doesn't look like a proper use of cs_inactive?

> 
> And the non-offload case already does:
> 
> 	xfers[0].cs_change_delay.value = st->time_spec->t_conv_ns;
> 
> which actually does work with the AXI SPI Engine. So why not be consistent and
> do it the same way for the offload case?

One of the points in using `bits_per_word` in spi transfers was to reach high
frequency sample rate, right? I think it makes sense to use them for SPI offload
transfers. But we were also trying to set a proper CNV/CS dealy so that ADC
conversion could complete properly before starting requesting the data. That
also sound reasonable to me. But, spi_transfer struct doesn't provide a good
way of setting a CS inactive delay if only one transfer is executed. If we
use `cs_change_delay`, we would then be running two transfers, no? Plus, the ADC
would be doing two conversions (one after CS deasert of previous message and
one after CS deassert at the end of the first transfer) while we only read one
of them.

The offload message preparation would look like what we had in v2:

static int ad4000_prepare_offload_turbo_message(struct ad4000_state *st,
						const struct iio_chan_spec *chan)
{
	struct spi_transfer *xfers = st->offload_xfers;

	/* Dummy transfer to guarantee enough CS high time. */
	xfers[0].cs_change = 1;
	xfers[0].cs_change_delay.value = st->time_spec->t_quiet1_ns;
	xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;

	xfers[1].bits_per_word = chan->scan_type.realbits;
	xfers[1].len = chan->scan_type.realbits > 16 ? 4 : 2;
	xfers[1].delay.value = st->time_spec->t_quiet2_ns;
	xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
	xfers[1].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;

	spi_message_init_with_transfers(&st->offload_msg, xfers, 2);
	st->offload_msg.offload = st->offload;

	return devm_spi_optimize_message(&st->spi->dev, st->spi, &st->offload_msg);
}

Are we worried about a few clock cycles in between transfers but not worried
about running an entire dummy transfer?

Plus, I've tested the single-transfer offload message version with ADAQ4003 on
CoraZ7 and verified the results were correct.
FWIW, I put a copy of the dts I used for the tests at the end of this email.

> 
> It also seems safe to omit this altogether in the offload case and assume that
> the max sample rate will also ensure that the miniumum time for CS deasserted
> is respected.

If we can assume that, then I think that's another reason why we don't need
a dummy transfer to set CS high delay.

> 
> > +				ret = spi_setup(spi);
> > +				if (ret < 0)
> > +					return ret;
> > +			}
> > +
> > +			ret = ad4000_prepare_offload_message(st, indio_dev->channels);
> > +			if (ret)
> > +				return dev_err_probe(dev, ret,
> > +						     "Failed to optimize SPI msg\n");
> > +		} else {
> > +			indio_dev->info = &ad4000_info;
> > +			indio_dev->channels = chip->chan_spec;
> > +			indio_dev->num_channels = ARRAY_SIZE(chip->chan_spec);
> > +		}
> > +
> >  		ret = ad4000_prepare_3wire_mode_message(st, &indio_dev->channels[0]);
> >  		if (ret)
> > -			return ret;
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to optimize SPI msg\n");
> >  
> >  		break;


---------- zynq-coraz7s-adaq4003.dts { ------------------

// SPDX-License-Identifier: GPL-2.0
/*
 * Analog Devices ADAQ4003
 *
 * hdl_project: <pulsar_adc/coraz7s>
 * Link: https://github.com/analogdevicesinc/hdl/tree/main/projects/pulsar_adc
 * board_revision: <A>
 *
 * Copyright (C) 2023 Analog Devices Inc.
 */
/dts-v1/;
#include "zynq-coraz7s.dtsi"
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/gpio/gpio.h>

/ {
	adc_vref: regulator-vref {
		compatible = "regulator-fixed";
		regulator-name = "EVAL 5V Vref";
		regulator-min-microvolt = <5000000>;
		regulator-max-microvolt = <5000000>;
		regulator-always-on;
	};

	adc_vdd: regulator-vdd {
		compatible = "regulator-fixed";
		regulator-name = "Eval VDD supply";
		regulator-min-microvolt = <5000000>;
		regulator-max-microvolt = <5000000>;
		regulator-always-on;
	};

	adc_vio: regulator-vio {
		compatible = "regulator-fixed";
		regulator-name = "Eval VIO supply";
		regulator-min-microvolt = <3300000>;
		regulator-max-microvolt = <3300000>;
		regulator-always-on;
	};

	trigger_pwm: adc-pwm-trigger {
		compatible = "pwm-trigger";
		#trigger-source-cells = <0>;
		pwms = <&adc_trigger 0 1000000 0>;
	};
};

&fpga_axi {

	adc_trigger: pwm@0x44b00000 {
		compatible = "adi,axi-pwmgen-2.00.a";
		reg = <0x44b00000 0x1000>;
		label = "adc_conversion_trigger";
		#pwm-cells = <2>;
		clocks = <&spi_clk>;
	};

	spi_engine: spi@44a00000 {
		compatible = "adi,axi-spi-engine-1.00.a";
		reg = <0x44a00000 0x10000>;
		interrupt-parent = <&intc>;
		interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
		clocks = <&clkc 15 &spi_clk>;
		clock-names = "s_axi_aclk", "spi_clk";

		dmas = <&rx_dma 0>;
		dma-names = "offload0-rx";
		trigger-sources = <&trigger_pwm>;

		#address-cells = <0x1>;
		#size-cells = <0x0>;

		adc@0 {
			compatible = "adi,adaq4003";
			reg = <0>;
			spi-max-frequency = <100000000>;
			vdd-supply = <&adc_vdd>;
			vio-supply = <&adc_vio>;
			ref-supply = <&adc_vref>;
			adi,high-z-input;
			adi,gain-milli = /bits/ 16 <454>;
		};
	};

	rx_dma: rx-dmac@44a30000 {
		compatible = "adi,axi-dmac-1.00.a";
		reg = <0x44a30000 0x1000>;
		#dma-cells = <1>;
		interrupt-parent = <&intc>;
		interrupts = <0 57 IRQ_TYPE_LEVEL_HIGH>;
		clocks = <&clkc 16>;
	};

	spi_clk: axi-clkgen@0x44a70000 {
		compatible = "adi,axi-clkgen-2.00.a";
		reg = <0x44a70000 0x10000>;
		#clock-cells = <0>;
		clocks = <&clkc 15>, <&clkc 15>;
		clock-names = "s_axi_aclk", "clkin1";
		clock-output-names = "spi_clk";

		assigned-clocks = <&spi_clk>;
		assigned-clock-rates = <200000000>;
	};
};

---------- } zynq-coraz7s-adaq4003.dts ------------------