Re: [PATCH] iio: afe: iio-rescale: Support processed channels

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 14 Dec 2020 09:34:40 +0100
Peter Rosin <peda@axentia.se> wrote:

> On 2020-12-13 13:16, Jonathan Cameron wrote:
> > On Sun, 13 Dec 2020 00:22:17 +0100
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> On 2020-12-12 13:26, Linus Walleij wrote:  
> >>> On Mon, Nov 2, 2020 at 12:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>     
> >>>> It happens that an ADC will only provide raw or processed
> >>>> voltage conversion channels. (adc/ab8500-gpadc.c).
> >>>> On the Samsung GT-I9070 this is used for a light sensor
> >>>> and current sense amplifier so we need to think of something.
> >>>>
> >>>> The idea is to allow processed channels and scale them
> >>>> with 1/1 and then the rescaler can modify the result
> >>>> on top.
> >>>>
> >>>> Cc: Peter Rosin <peda@axentia.se>
> >>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>    
> >>>
> >>> Did we reach any conclusion on this? I really need to use
> >>> the rescaler on an ADC that only handles processed channels...
> >>>
> >>> I'm sorry that I can't make this ADC disappear :D    
> >>
> >> Hi!
> >>
> >> My conclusion was that the patch is buggy since it presents inconsistent
> >> information. That needs to be fixed one way or the other. If the offending
> >> information cannot be filtered out for some reason, I don't know what to
> >> do. Details in my previous comment [1]. BTW, I still do not know the answer
> >> to the .read_avail question at the end of that message, and I don't have
> >> time to dig into it. Sorry.  
> > 
> > Unless I'm missing something, I think it presents no information unless
> > we strangely have a driver providing read_avail for _RAW but only
> > _PROCESSED channels which is a bug.  I'm not that bothered about
> > missing information in this particular, somewhat obscure, corner case.
> > 
> > So I think we should take the patch as it stands.  It's missed the
> > merge window now anyway unfortunately.  So Peter, I would suggest we
> > take this and perhaps revisit to tidy up loose corners when we all have
> > more time.  
> 
> My concern was a driver with a raw channel, including read_avail, providing
> raw sample values but that no easy conversion existed to get from that to
> the processed values. One option for the driver in that case would be to
> provide these raw values, but then have no scaling info.

Generally I resist this a lot. The reason is that it is impossible to write
generic userspace software against it. The one time we did let this happen
was with some of the heart rate sensors (pulse oximeters) where the algorithm
to derive the eventual value is both complex - based on published literature,
and proprietary (what was actually readily usable). What the measurement being
provided to userspace was is well documented, but not how on earth you get from
that to something useable for what the sensor is designed to measure.

> I.e. the way I see
> it, it is perfectly reasonable for a driver to provide raw with read_avail,
> no scaling but also processed values.

Why?  What use would the raw values actually be?  There are a couple of historical
drivers where they evolved to this state, but it is not one we would normally accept.
We go to a lot of effort to try and avoid this.
 
> And that gets transformed by the
> rescaler into the processed values being presented as raw, with rescaling
> added on top, but with the read_avail info for this new raw channel being
> completely wrong.
> 
> For the intended driver (ab8500-gpadc) this is not the case (it has no
> read_avail for its raw channel). But it does have a raw channel, so adding
> read_avail seems easy and I can easily see other drivers already doing it.
> Haven't checked that though...

Drat. I'd failed to register this is one of those corner cases.


> 
> But if you say that this never happens, fine. Otherwise, since it's too
> late for the merge window anyway, the patch might as well be updated such
> that the rescaler blocks the read_avail channel in this situation, if it
> exists.

That's fair enough.  A sanity check and then suitable warning message to explain
why it is blocked makes sense.

Jonathan

> 
> Cheers,
> Peter