Message-ID: <20240803104828.3c9159ef@jic23-huawei>
Date: Sat, 3 Aug 2024 10:48:28 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Esteban Blanc <eblanc@...libre.com>, 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 Wed, 31 Jul 2024 11:07:14 +0200
Nuno Sá <noname.nuno@...il.com> wrote:

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

For stuff that is coming out of the DMA / bulk interfaces, I'm rather
more flexible on new data layouts.  That tends to run against a narrow
range of tooling + generally users have a better idea of what they
are doing (after all they probably bought some pricey hardware :)

So there I'd be happy to see proposals for other storage format descriptions
or indeed relaxing the requirements on existing description.

If there is a software path to simply reorganize the data, keep
to existing interfaces with the power of 2 aligned data requirements.

Jonathan

> 
> - Nuno Sá

Powered by blists - more mailing lists