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

[Nuno Sá]

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.

- Nuno Sá