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

[David Lechner]

On 3/27/25 12:56 PM, Marcelo Schmitt wrote:

...

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

We would still only be doing one xfer per SPI offload trigger. The only
difference it would make is that it would ensure that the second trigger
could not come too soon after the CS deassert of the previous message.

In other words, the sampling frequency is already supplying this delay
between subsequent SPI messages. Setting xfers[0].cs_change_delay just
adds insurance that if the next trigger comes too soon, it will be ignored.
This could happen, e.g. if someone sets the max SCLK rate to something
low enough that the single xfer takes longer than the trigger period at
the max sampling frequency.

In most cases though, we won't actually see cs_change_delay having any
effect because the trigger to start the next SPI message doesn't come
until later.

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

I agree we don't need two xfers, especially in turbo mode.