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

On Fri, 2024-07-26 at 15:38 +0200, Esteban Blanc wrote:

Hi Nuno,

quoted

quoted

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

quoted

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.

quoted

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.

quoted

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:

quoted

@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á