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

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

Hi, comments inline.

On 03/17, David Lechner wrote:
> On 3/17/25 10:31 AM, Marcelo Schmitt wrote:
> 
> 
> > ...
> >>> +/*
> >>> + * 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!)
> > 
> > TL;DR: The invalid sample goes into the buffer as a valid one.
> > 
> > In AD4000 '3-wire' mode, data capture has a latency (delay) of one sample.
> > 
> > The ADC begins sampling data N at CNV rising edge
> >           |   +-- CNV (usually SPI CS) is brought low to begin reading the data
> >           |   |                                +-- Data N + 1 that will be read
> >           |   |                                |   on the next transfer starts 
> >           v   v                                v   being sampled at end of transfer N.
> >            ___                                  ____            
> > CNV  _____/   \________________________________/    \_____
> >                     _     _             _
> > SCLK ______________/ \___/ \_ ...   ___/ \_______________
> >                    ___   ___           ___
> > SDO  _____________/___\_/___\ ...   __/___\_______________
> >                     ^
> >                     |
> >              Data from conversion N is output from here on
> > 
> > A better drawing can be found in datasheet page 29, Figure 57.
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADAQ4003.pdf
> > 
> > In sum, we're always reading a conversion that started at the end of the
> > previous SPI transfer or, in other words, the data comes out with a latency
> > (delay) of one read.
> > 
> > Datasheet somehow mentions that by saying
> > 	When turbo mode is enabled, the conversion result read on SDO corresponds to
> > 	the result of the previous conversion.
> > 
> > I think I can do a dummy SPI transfer on buffer preenable so at least the
> > first data is not invalid. Would that be better?
> 
> Not really. There will be a relatively long delay between that conversion
> trigger and when the sample is read. So the data might be slightly less stale
> in that case, but still not particularly useful, e.g. if you are doing any
> kind of signal processing that expects equal time between all samples.
> 
> On similar chips, like ad7944, we just documented that the first sample does
> not contain valid data and needs to be discarded.
> 
Okay, I'll assume that to be acceptable and do the same for this one.

...

> > I also didn't expect to find out HDL support for 16-bit data width was removed.
> > We used to have a build parameter for 16-bit precision ADCs.
> > https://github.com/analogdevicesinc/hdl/commit/b2dc91b30dae891b6319d88e083f26e726f43ba0#diff-1117c2618353232e5f22aa6a12e8ae976757fa897b3425f470a12123cae26535L13
> 
> A while back the HDL engineers mentioned to us that they wanted to standardize
> on 32-bit data words everywhere. While not the most efficient use of memory,
> having fewer options does make things simpler across the entire software stack.
> 
Ack

> > 
> > Would something like 'because SPI offloading leads to data being pushed to
> > memory in CPU endianness' be a reasonable comment?
> 
> Another way to say it is that SPI offload reads data in complete words and not
> in separate 8-bit xfers (bits_per_word = realbits vs. bits_per_word = 8).
> 
Ah sure, I recall the effect of setting .bits_per_word now.
Will add a comment explaining why the difference in endianness.

Thanks,
Marcelo