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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <35f4d22a-e478-4a43-bbb6-f9d34ce1f888@baylibre.com>
Date: Thu, 27 Mar 2025 11:12:08 -0500
From: David Lechner <dlechner@...libre.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: jic23@...nel.org, lars@...afoo.de, Michael.Hennerich@...log.com,
 corbet@....net, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v3 1/4] iio: adc: ad4000: Add support for SPI offload

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@...log.com>
> ---

...

> diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> index 4fe8dee48da9..9fc56853265e 100644
> --- a/drivers/iio/adc/ad4000.c
> +++ b/drivers/iio/adc/ad4000.c
> @@ -16,11 +16,13 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> +#include <linux/spi/offload/consumer.h>

Alphabetical order?

>  #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>
>  

...

>  
> +/*
> + * When SPI offload is configured, transfers are executed withouth CPU

s/withouth/without/

> + * 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.

>  
>  		/*
>  		 * In "3-wire mode", the ADC SDI line must be kept high when
> @@ -796,9 +1096,26 @@ static int ad4000_probe(struct spi_device *spi)
>  		if (ret < 0)
>  			return ret;
>  
> +		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.

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?

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.

> +				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;