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

[Jonathan Cameron <jic23@kernel.org>]

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.