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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 17 Mar 2025 14:21:13 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

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

Hmm. Doesn't really pass the useability 'smell' test but I guess
if there is precedence and clear docs it should be fine.

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

Great

> 
> Thanks,
> Marcelo