Re: [PATCH RFC 2/5] iio: adc: ad4030: add driver for ad4030-24

["Esteban Blanc" <eblanc@baylibre.com>]

On Sat Aug 3, 2024 at 11:51 AM CEST, Jonathan Cameron wrote:
> On Wed, 31 Jul 2024 10:56:22 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Fri, 2024-07-26 at 15:38 +0200, Esteban Blanc wrote:
> > > Hi Nuno,
> > >   
> > > > > +			struct {
> > > > > +				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
> > > > > +				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];  
> > > > 
> > > > Not sure common makes sense as it comes aggregated with the sample. Maybe
> > > > this
> > > > could as simple as:
> > > > 
> > > > struct {
> > > > 	s32 val;
> > > > 	u64 timestamp __aligned(8);
> > > > } rx_data ...  
> > > 
> > > See below my answer on channels order and storagebits.
> > >   
> > > > So, from the datasheet, figure 39 we have something like a multiplexer where
> > > > we
> > > > can have:
> > > > 
> > > > - averaged data;
> > > > - normal differential;
> > > > - test pattern (btw, useful to have it in debugfs - but can come later);
> > > > - 8 common mode bits;
> > > > 
> > > > While the average, normal and test pattern are really mutual exclusive, the
> > > > common mode voltage is different in the way that it's appended to
> > > > differential
> > > > sample. Making it kind of an aggregated thingy. Thus I guess it can make
> > > > sense
> > > > for us to see them as different channels from a SW perspective (even more
> > > > since
> > > > gain and offset only apply to the differential data). But there are a couple
> > > > of
> > > > things I don't like (have concerns):
> > > > 
> > > > * You're pushing the CM channels into the end. So when we a 2 channel device
> > > > we'll have:
> > > > 
> > > >  in_voltage0 - diff
> > > >  in_voltage1 - diff
> > > >  in_voltage2 - CM associated with chan0
> > > >  in_voltage0 - CM associated with chan1
> > > > 
> > > > I think we could make it so the CM channel comes right after the channel
> > > > where
> > > > it's data belongs too. So for example, odd channels would be CM channels
> > > > (and
> > > > labels could also make sense).  
> > > 
> > > I must agree with you it would make more sense.
> > >   
> > > > Other thing that came to mind is if we could somehow use differential = true
> > > > here. Having something like:
> > > > 
> > > > in_voltage1_in_voltage0_raw - diff data
> > > > ...
> > > > And the only thing for CM would be:
> > > > 
> > > > in_voltage1_raw
> > > > in_voltage1_scale
> > > > 
> > > > (not sure if the above is doable with ext_info - maybe only with
> > > > device_attrs)
> > > > 
> > > > The downside of the above is that we don't have a way to separate the scan
> > > > elements. Meaning that we don't have a way to specify the scan_type for both
> > > > the
> > > > common mode and differential voltage. That said, I wonder if it is that
> > > > useful
> > > > to buffer the common mode stuff? Alternatively, we could just have the
> > > > scan_type
> > > > for the diff data and apps really wanting the CM voltage could still access
> > > > the
> > > > raw data. Not pretty, I know...  
> > > 
> > > At the moment the way I "separate" them is by looking at the
> > > `active_scan_mask`. If the user asked for differential channel only, I put the
> > > chip in differential only mode. If all the channels are asked, I put
> > > the chip in differential + common mode. This way there is no need to
> > > separate anything in differential mode. See below for an example where
> > > this started.
> > >   
> > > > However, even if we go with the two separate channels there's one thing that
> > > > concerns me. Right now we have diff data with 32 for storage bits and CM
> > > > data
> > > > with 8 storage bits which means the sample will be 40 bits and in reality we
> > > > just have 32. Sure, now we have SW buffering so we can make it work but the
> > > > ultimate goal is to support HW buffering where we won't be able to touch the
> > > > sample and thus we can't lie about the sample size. Could you run any test
> > > > with
> > > > this approach on a HW buffer setup?   
> > > 
> > > Let's take AD4630-24 in diff+cm mode as an example. We would have 4 channels:
> > > - Ch0 diff with 24 bits of realbits and 24 bits of storagebits
> > > - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> > > - Ch1 diff with 24 bits of realbits and 24 bits of storagebits
> > > - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> > > ChX diff realbits + ChX cm realbits = 32 bits which is one sample as sent
> > > by the chip.
> > > 
> > > The problem I faced when trying to do this in this series is that IIO doesn't
> > > seem to like 24 storagebits and the data would get garbled. In diff only
> > > mode with the same channel setup (selecting only Ch0 diff and Ch1 diff)
> > > the data is also garbled. I used iio-oscilloscope software to test this setup.
> > > Here is the output with iio_readdev:
> > > ```
> > > # iio_readdev -s 1 ad4630-24 voltage0
> > > WARNING: High-speed mode not enabled
> > > Unable to refill buffer: Invalid argument (22)
> > > ```
> > > 
> > > I think this is happening when computing the padding to align ch1 diff.
> > > In `iio_compute_scan_bytes` this line `bytes = ALIGN(bytes, length);`
> > > will be invoked with bytes = 3 and length = 3 when selecting ch0 diff
> > > and ch1 diff (AD4630-24 in differential mode). The output is 5. As
> > > specified in linux/align.h:  
> > > > @a is a power of 2  
> > > In our case `a` is `length`, and 3 is not a power of 2.
> > > 
> > > It works fine with Ch0/1 diff with 24 realbits and 32 storagebits with 8
> > > bits shift.  
> > 
> > Yes, I do understand that and that we need a power of 2 for storage bits. My
> > concern, as stated, is that if we have HW buffering (High speed enabled) CHO
> > will have a sample size of (with diff + cm) of 40 which is not really what comes
> > from HW. I wonder if it will work in that case. Maybe we can (as this often
> > happens on an FGPA) have the HW guys doing some data shuffling so things work in
> > the high speed mode as well.
>
> If it's possible to unscramble the data in an fpga, that would make our life easier
> though things may get messy if we get multiple versions of that and some
> unscramble, others don't.

At the moment the HDL I could test for this chip is unscrambling the
data, so there is nothing to do on the software side on that front.

Best regards,

-- 
Esteban "Skallwar" Blanc
BayLibre