On Wed, 31 Jul 2024 11:07:14 +0200 Nuno Sá <noname.nuno@xxxxxxxxx> 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á
