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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 14 Mar 2025 14:18:39 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> 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@analog.com>

A few minor things inline.  

Jonathan

> diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> index 4fe8dee48da9..6c9b71e7a2fb 100644
> --- a/drivers/iio/adc/ad4000.c
> +++ b/drivers/iio/adc/ad4000.c

> +
> +static int ad4000_offload_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +
> +	spi_offload_trigger_disable(st->offload, st->offload_trigger);

Trivial. Prefer a blank line before a 'simple return' like this one.

> +	return 0;
> +}


> +
> +/*
> + * This executes a data sample transfer when using SPI offloading for when the
> + * device connections are in "3-wire" mode, selected when the adi,sdi-pin device
> + * tree property is absent. In this connection mode, the ADC SDI pin is
> + * connected to MOSI or to VIO and ADC CNV pin is connected to a SPI controller
> + * CS (it can't be connected to a GPIO).
> + *
> + * In order to achieve the maximum sample rate, we only do one transfer per
> + * SPI offload trigger. This has the effect that the first sample data is not
> + * valid because it is reading the previous conversion result. We also use

As below. Should mention what happens with that invalid sample.

> + * bits_per_word to ensure the minimum of SCLK cycles are used. And a delay is
> + * added to make sure we meet the minimum quiet time before releasing the CS
> + * line. Plus the CS change delay is set to ensure that we meet the minimum
> + * quiet time before asserting CS again.
> + *
> + * This timing is only valid if turbo mode is enabled (reading during conversion).
> + */
> +static int ad4000_prepare_offload_turbo_message(struct ad4000_state *st,
> +						const struct iio_chan_spec *chan)
> +{
> +
> +/*
> + * This executes a data sample transfer when using SPI offloading for when the
> + * device connections are in "3-wire" mode, selected when the adi,sdi-pin device
> + * tree property is set to "high". In this connection mode, the ADC SDI pin is
> + * connected to VIO and ADC CNV pin is connected to a SPI controller CS (it
> + * can't be connected to a GPIO).
> + *
> + * In order to achieve the maximum sample rate, we only do one transfer per
> + * SPI offload trigger. This has the effect that the first sample data is not
> + * valid because it is reading the previous conversion result. We also use

Say what happens to that invalid sample.  Is it dropped or provided to userspace
as if it were valid?  (I hope dropped!)

> + * bits_per_word to ensure the minimum of SCLK cycles are used. And a delay is
> + * added to make sure we meet the minimum quiet time before releasing the CS
> + * line. Plus the CS change delay is set to ensure that we meet the minimum
> + * conversion time before asserting CS again.
> + *
> + * This timing is only valid if turbo mode is disabled (reading during acquisition).
> + */
> +static int ad4000_prepare_offload_message(struct ad4000_state *st,
> +					  const struct iio_chan_spec *chan)
> +

...

>  /*
>   * This executes a data sample transfer for when the device connections are
>   * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
> @@ -689,7 +975,9 @@ static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
>  	xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>  
>  	xfers[1].rx_buf = &st->scan.data;
> -	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> +	xfers[1].len = chan->scan_type.realbits > 16 ? 4 : 2;
> +	if (chan->scan_type.endianness != IIO_BE)

As below. Endianness being related to bits_per_word is odd. So needs
explanation.

> +		xfers[1].bits_per_word = chan->scan_type.realbits;
>  	xfers[1].delay.value = st->time_spec->t_quiet2_ns;
>  	xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
>  
> @@ -719,7 +1007,9 @@ static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
>  	xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
>  
>  	xfers[1].rx_buf = &st->scan.data;
> -	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> +	xfers[1].len = chan->scan_type.realbits > 16 ? 4 : 2;
> +	if (chan->scan_type.endianness != IIO_BE)

This is odd enough to require a comment.  Why is endianness relevant?

> +		xfers[1].bits_per_word = chan->scan_type.realbits;
> @@ -733,6 +1023,9 @@ static int ad4000_config(struct ad4000_state *st)
>  	if (device_property_present(&st->spi->dev, "adi,high-z-input"))
>  		reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
>  
> +	if (st->using_offload)
> +		reg_val |= FIELD_PREP(AD4000_CFG_TURBO, 1);
> +
>  	return ad4000_write_reg(st, reg_val);
>  }
>  


> +		if (st->using_offload) {
> +			indio_dev->channels = &chip->reg_access_offload_chan_spec;
Set num_channels here
> +			ret = ad4000_prepare_offload_turbo_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;
and here as well. I think you can use ARRAY_SIZE() to make the connection even more
obvious.

> +		}
> +
> +		/*
> +		 * 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 +1134,36 @@ 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;

here as well.

> +
> +			spi->cs_hold.value = AD4000_TCONV_NS;
> +			spi->cs_hold.unit = SPI_DELAY_UNIT_NSECS;
> +			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;

Also set size here.  Obviously this means a little duplication but still good
to keep them together.


> +		}

>  	case AD4000_SDI_GND:
> @@ -830,7 +1175,10 @@ static int ad4000_probe(struct spi_device *spi)
>  	}
>  
>  	indio_dev->name = chip->dev_name;
> -	indio_dev->num_channels = 2;
> +	if (st->using_offload)
> +		indio_dev->num_channels = 1;
> +	else
> +		indio_dev->num_channels = 2;

Move this up to where you set channels so that the array
and size are set together.

>  
>  	ret = devm_mutex_init(dev, &st->lock);
>  	if (ret)
> @@ -853,12 +1201,6 @@ static int ad4000_probe(struct spi_device *spi)
>  
>  	ad4000_fill_scale_tbl(st, &indio_dev->channels[0]);
>  
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> -					      &iio_pollfunc_store_time,
> -					      &ad4000_trigger_handler, NULL);
> -	if (ret)
> -		return ret;
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> @@ -947,3 +1289,4 @@ module_spi_driver(ad4000_driver);
>  MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@analog.com>");
>  MODULE_DESCRIPTION("Analog Devices AD4000 ADC driver");
>  MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_DMAENGINE_BUFFER");