Re: [PATCH] iio: afe: rescale: Fix logic bug

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

On Tue, 29 Aug 2023 09:17:00 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Mon, Aug 28, 2023 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > Not 100% the follow is relevant as I've lost track of the discussion
> > but maybe it is useful.
> >
> > Worth noting there are a few reasons why RAW and PROCESSED can coexist
> > in drivers and indeed why SCALE can be absent.. (OFFSET is much the same)  
> 
> That's fine. If we have PROCESSED the rescaler will use that first.
> 
> What we're discussing are channels that have just RAW
> and no PROCESSED, nor SCALE or OFFSET yet are connected to
> a rescaler.
> 
> > 1) If SCALE = 1.0 the driver is allowed not to provide it - the channel
> >    might still be raw if OFFSET != 0  
> 
> I'm not so sure the rescaler can handle that though. Just no-one
> ran into it yet...
> 
> > 2) If the channel has a horrible non linear and none invertable conversion
> >    to standard units and events support the you might need PROCESSED to
> >    provide the useful value, but RAW to give you clue what the current value
> >    is for setting an event (light sensors are usual place we see this).
> >
> > 3) Historical ABI errors.  If we first had RAW and no scale or offset because
> >    we had no known values for them.  Then later we discovered that there
> >    was a non linear transform involved (often when someone found a magic
> >    calibration code somewhere).  Given the RAW interface might be in use
> >    and isn't a bug as such, we can't easily remove it.  The new PROCESSED
> >    interface needs to be there because of the non linear transform..
> >
> > Odd corner cases...  In this particular case the original code made no
> > sense but might have allowed for case 3 by accident?  
> 
> I think it's fine, we make PROCESSED take precedence in all cases
> as long as SCALE is not there as well.
> 
> rescale_configure_channel() does this:
> 
>         if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) &&
>             iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) {
>                 dev_info(dev, "using raw+scale source channel\n");
>         } else if (iio_channel_has_info(schan, IIO_CHAN_INFO_PROCESSED)) {
>                 dev_info(dev, "using processed channel\n");
>                 rescale->chan_processed = true;
>         } else {
>                 dev_err(dev, "source channel is not supported\n");
>                 return -EINVAL;
>         }
> 
> I think the first line should be
> 
> if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) &&
>     (iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE ||
>      iio_channel_has_info(schan,IIO_CHAN_INFO_OFFSET)))
> 
> right? We just never ran into it.

Makes sense to me.

Jonathan

> 
> Yours,
> Linus Walleij