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.
