Message-ID: <f6c9f7730541a6a97e4ce0005bb4cf600973e691.camel@gmail.com>
Date: Wed, 31 Jul 2024 11:07:14 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, Esteban Blanc <eblanc@...libre.com>
Cc: baylibre-upstreaming@...ups.io, Lars-Peter Clausen <lars@...afoo.de>, 
 Michael Hennerich <Michael.Hennerich@...log.com>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
 linux-kernel@...r.kernel.org, David Lechner <dlechner@...libre.com>
Subject: Re: [PATCH RFC 2/5] iio: adc: ad4030: add driver for ad4030-24

On Sun, 2024-07-28 at 16:04 +0100, Jonathan Cameron wrote:
> One quick comment form me inline.
> 
> The short version is non power of 2 storage is not an option because
> it is a major ABI change and we aren't paying the cost of complexity
> that brings to userspace for a very small number of drivers where
> there is any real advantage to packing them tighter.
> 
> > 
> > > 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.
> > 
> > Intrestingly, a similar setup works great on AD4630-16:
> > - Ch0 diff with 16 bits of realbits and 16 bits of storagebits
> > - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> > - Ch1 diff with 16 bits of realbits and 16 bits of storagebits
> > - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> > 
> > In `iio_compute_scan_bytes` we will have ALIGN(3, 2) which will output
> > 4, everything is fine. The output of iio-oscilloscope is as expected,
> > a clean sinewave and iio_readdev does not throw an error.
> > 
> > All this to say that at the moment, I'm not sure how I will be able to
> > put the CM byte in a separate channel for AD4630-24 without buffering it.
> > It would be useful to return a "packed" buffer.
> 
> Whilst it might be useful to allow non power of 2 storage formats,
> that's a break of the IIO userspace ABI so that isn't an approach to
> consider. You must shuffle the data in the driver.

Yeah, I do agree the breakage is really not the way to go...

OTOH, I'm seeing more and more of these devices with kind of multiplexed
data/channels in one sample (like cm and diff in this case) and while in SW
buffering we can workaround it in the driver, for HW buffering things may be not
that "simple".

Not sure what we can do about it but one concept that came to mind when I was
giving some thinking about this was kind of a virtual scan element that would
essentially map/link to a (physical) scan element (so virtual_scan + scan =
storage_size of the real scan element). Kind of a basic idea for now and I'm not
really sure how much work would this be or even how could we expose this to
userspace without breaking current ABI (basically if it's doable at all :)).

The only other option I see for these kind of devices (if there's nothing we can
do in HW for shuffling data) is to expose a different channel setup that does
not "lie" about the sample size. And it's up to userspace to parse the sample
data. Far from pretty though...

- Nuno Sá

Powered by blists - more mailing lists