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

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

[Linus Walleij]

When introducing support for processed channels I needed
to invert the expression:

  if (!iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
      !iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE))
        dev_err(dev, "source channel does not support raw/scale\n");

To the inverse, meaning detect when we can usse raw+scale
rather than when we can not. This was the result:

  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");

Ooops. Spot the error. Yep old George Boole came up and bit me.
That should be an &&.

The current code "mostly works" because we have not run into
systems supporting only raw but not scale or only scale but not
raw, and I doubt there are few using the rescaler on anything
such, but let's fix the logic.

Cc: Liam Beguin <liambeguin@gmail.com>
Cc: stable@vger.kernel.org
Fixes: 53ebee949980 ("iio: afe: iio-rescale: Support processed channels")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/afe/iio-rescale.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 7e511293d6d1..dc426e1484f0 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -278,7 +278,7 @@ static int rescale_configure_channel(struct device *dev,
 	chan->ext_info = rescale->ext_info;
 	chan->type = rescale->cfg->type;
 
-	if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
+	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)) {
-- 
2.35.3

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

[Peter Rosin]

Hi!

2022-05-24 at 09:54, Linus Walleij wrote:
> When introducing support for processed channels I needed
> to invert the expression:
> 
>   if (!iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
>       !iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE))
>         dev_err(dev, "source channel does not support raw/scale\n");
> 
> To the inverse, meaning detect when we can usse raw+scale

Nit: use

> rather than when we can not. This was the result:
> 
>   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");
> 
> Ooops. Spot the error. Yep old George Boole came up and bit me.
> That should be an &&.

Good catch!

> The current code "mostly works" because we have not run into
> systems supporting only raw but not scale or only scale but not
> raw, and I doubt there are few using the rescaler on anything
> such, but let's fix the logic.

Scaling something that provides raw but not scale *could* have
been implemented by assuming an upstream scale of 1, but it is
not. Instead you get an error in that case and thus no scale at
all. While that is perhaps not wrong, it is also a pretty
useless situation.

Scaling something as if there are raw values when that something
only provides scale but not raw also seems pretty useless.

So, you have my

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

> 
> Cc: Liam Beguin <liambeguin@gmail.com>
> Cc: stable@vger.kernel.org
> Fixes: 53ebee949980 ("iio: afe: iio-rescale: Support processed channels")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/afe/iio-rescale.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 7e511293d6d1..dc426e1484f0 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -278,7 +278,7 @@ static int rescale_configure_channel(struct device *dev,
>  	chan->ext_info = rescale->ext_info;
>  	chan->type = rescale->cfg->type;
>  
> -	if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
> +	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)) {

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

[Liam Beguin]

On Tue, May 24, 2022 at 09:54:48AM +0200, Linus Walleij wrote:
> When introducing support for processed channels I needed
> to invert the expression:
> 
>   if (!iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
>       !iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE))
>         dev_err(dev, "source channel does not support raw/scale\n");
> 
> To the inverse, meaning detect when we can usse raw+scale
> rather than when we can not. This was the result:
> 
>   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");
> 
> Ooops. Spot the error. Yep old George Boole came up and bit me.
> That should be an &&.
> 
> The current code "mostly works" because we have not run into
> systems supporting only raw but not scale or only scale but not
> raw, and I doubt there are few using the rescaler on anything
> such, but let's fix the logic.

Maybe `iio: afe: rescale: Fix boolean logic bug` if you're resending,
otherwise,

Reviewed-by: Liam Beguin <liambeguin@gmail.com>

Thanks,
Liam

> Cc: Liam Beguin <liambeguin@gmail.com>
> Cc: stable@vger.kernel.org
> Fixes: 53ebee949980 ("iio: afe: iio-rescale: Support processed channels")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/afe/iio-rescale.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 7e511293d6d1..dc426e1484f0 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -278,7 +278,7 @@ static int rescale_configure_channel(struct device *dev,
>  	chan->ext_info = rescale->ext_info;
>  	chan->type = rescale->cfg->type;
>  
> -	if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
> +	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)) {
> -- 
> 2.35.3
> 

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

[Jonathan Cameron]

On Wed, 25 May 2022 21:29:27 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> On Tue, May 24, 2022 at 09:54:48AM +0200, Linus Walleij wrote:
> > When introducing support for processed channels I needed
> > to invert the expression:
> > 
> >   if (!iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
> >       !iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE))
> >         dev_err(dev, "source channel does not support raw/scale\n");
> > 
> > To the inverse, meaning detect when we can usse raw+scale
> > rather than when we can not. This was the result:
> > 
> >   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");
> > 
> > Ooops. Spot the error. Yep old George Boole came up and bit me.
> > That should be an &&.
> > 
> > The current code "mostly works" because we have not run into
> > systems supporting only raw but not scale or only scale but not
> > raw, and I doubt there are few using the rescaler on anything
> > such, but let's fix the logic.  
> 
> Maybe `iio: afe: rescale: Fix boolean logic bug` if you're resending,
> otherwise,

Makes sense - I tweaked that whilst applying.

Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> 
> Reviewed-by: Liam Beguin <liambeguin@gmail.com>
> 
> Thanks,
> Liam
> 
> > Cc: Liam Beguin <liambeguin@gmail.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 53ebee949980 ("iio: afe: iio-rescale: Support processed channels")
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/iio/afe/iio-rescale.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 7e511293d6d1..dc426e1484f0 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -278,7 +278,7 @@ static int rescale_configure_channel(struct device *dev,
> >  	chan->ext_info = rescale->ext_info;
> >  	chan->type = rescale->cfg->type;
> >  
> > -	if (iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
> > +	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)) {
> > -- 
> > 2.35.3
> >   

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

[Mighty]

> The current code "mostly works" because we have not run into
> systems supporting only raw but not scale or only scale but not
> raw, and I doubt there are few using the rescaler on anything
> such, but let's fix the logic.

This does break the logic for twl6030/6032 boards as the seem to only have only raw and processed masks[1]. What would a probable solution for it be, to modify the twl gpadc to include scale+raw or just revert back to the OR?

[1] https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L808-L840

Mithil

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

[Linus Walleij]

On Wed, Aug 9, 2023 at 3:48 PM Mighty <bavishimithil@gmail.com> wrote:

> > The current code "mostly works" because we have not run into
> > systems supporting only raw but not scale or only scale but not
> > raw, and I doubt there are few using the rescaler on anything
> > such, but let's fix the logic.
>
> This does break the logic for twl6030/6032 boards as the seem to
> only have only raw and processed masks[1]. What would a probable
> solution for it be, to modify the twl gpadc to include scale+raw or just
> revert back to the OR?
>
> [1] https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L808-L840

How does it break it?

It's a change to the AFE rescaler driver so it can't really "break"
twl6030-gpadc.

Isn't the complete picture involving some device tree using the prescaler
etc?

Can you try to dig out that so we can see the whole situation?

Yours,
Linus Walleij

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

[Mighty]

> How does it break it?
>
> It's a change to the AFE rescaler driver so it can't really "break"
> twl6030-gpadc.

Not necessarily the gpadc, but it breaks my current-sense-shunt which requires an adc channel for it to work, since the iio-rescale driver wont recognise the channel (as it only is IIO_CHAN_INFO_RAW, so the && breaks)

> Isn't the complete picture involving some device tree using the prescaler
> etc?

I'm not sure I understand that, could you explain it, maybe with some example as well?

Mithil

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

[Linus Walleij]

On Mon, Aug 21, 2023 at 6:45 PM Mighty <bavishimithil@gmail.com> wrote:
>
> > How does it break it?
> >
> > It's a change to the AFE rescaler driver so it can't really "break"
> > twl6030-gpadc.
>
> Not necessarily the gpadc, but it breaks my current-sense-shunt which requires an
> adc channel for it to work, since the iio-rescale driver wont recognise the channel
> (as it only is IIO_CHAN_INFO_RAW, so the && breaks)

OK so twl6030 is providing some channels with IIO_CHAN_INFO_RAW
without IIO_CHAN_INFO_SCALE.

How is the rescaler supposed to rescale the raw value without any
scale?

Say the raw value is 100, then 100 what? Microvolts? Certainly the
twl6030 has to provide a scale or a processed channel to be used
with the rescaler. If the rescaler would assume "no scale means
scale 1:1" then that is equivalent to a processed channel, and you
can just patch the twl6030 driver to convert all
IIO_CHAN_INFO_RAW to IIO_CHAN_INFO_PROCESSED.

Either the datasheet has the right scale for the channels, or it is
something design (board) specific and then it should be a parameter
to the driver e.g. through the device tree or similar mechanism.

Yours,
Linus Walleij

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

[Mighty]

On Wed, Aug 23, 2023 at 1:51 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> OK so twl6030 is providing some channels with IIO_CHAN_INFO_RAW
> without IIO_CHAN_INFO_SCALE.

Exactly, checking the driver there doesnt seem to be any scaler used. The two functions to read raw[1] and processed[2] are quite simple, with processed using the raw function as well. Since there is no mention of SCALE, could I just patch that in with the RAW, it wouldnt change anything in the driver (the driver has cases for RAW and PROCESSED only) and would fix the issue at hand as well.

> Say the raw value is 100, then 100 what? Microvolts?

I'd assume Volts, I couldnt find a datasheet for TWL6032 hence the assumption based on code https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L504.

> patch the twl6030 driver to convert all IIO_CHAN_INFO_RAW to IIO_CHAN_INFO_PROCESSED.

That would break the case here https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L541 hence I think we just comply to adding scale as well, even though it would be 1:1?
There is this https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L447 but I'm not very sure about how it changes the scale.

[1] https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L462
[2] https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L487

Mithil

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

[Linus Walleij]

On Thu, Aug 24, 2023 at 9:39 AM Mighty <bavishimithil@gmail.com> wrote:

> > patch the twl6030 driver to convert all IIO_CHAN_INFO_RAW to IIO_CHAN_INFO_PROCESSED.
>
> That would break the case here https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L541
> hence I think we just comply to adding scale as well, even though it would be 1:1?

Seems reasonable to me!

> There is this https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L447 but I'm not very sure about how it changes the scale.

That looks like the channel is actually processed, not raw, right?
i.e. that should only be done on channels marked as processed.

This should definitely be reflected in the scale attribute for the
raw channel instead, especially if you support buffered reads (I
don't know if the driver does this) because buffered reads usually
just read out the values from the hardware without any such
processing and rely on scale to correct them in userspace
afterwards.

Yours,
Linus Walleij

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

[Linus Walleij]

On Thu, Aug 24, 2023 at 10:24 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> This should definitely be reflected in the scale attribute for the
> raw channel instead,

Actually both IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_OFFSET
as it looks.

I usually use tools/iio/iio_generic_buffer.c to test the result after
applied scale and offset as it takes those into account.

Yours,
Linus Walleij

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

[Jonathan Cameron]

On Thu, 24 Aug 2023 10:28:01 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Aug 24, 2023 at 10:24 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > This should definitely be reflected in the scale attribute for the
> > raw channel instead,  
> 
> Actually both IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_OFFSET
> as it looks.
> 
> I usually use tools/iio/iio_generic_buffer.c to test the result after
> applied scale and offset as it takes those into account.
> 
> Yours,
> Linus Walleij

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)

1) If SCALE = 1.0 the driver is allowed not to provide it - the channel
   might still be raw if OFFSET != 0
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?

Jonathan

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

[Linus Walleij]

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.

Yours,
Linus Walleij

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

[Jonathan Cameron]

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

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

[Mighty]

On Thu, Aug 24, 2023 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> Seems reasonable to me!
I'd say I send a patch to the mailing list and see the response, I'm not very experienced with this device. The inputs of other people who worked on this driver would guide me in the right way then i guess.

> That looks like the channel is actually processed, not raw, right?
> i.e. that should only be done on channels marked as processed.

Yeah the raw channel function does call a correction function in some cases, not very sure why. https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L480
In the end the processed function also calls the raw function again dont know why, https://github.com/torvalds/linux/blob/master/drivers/iio/adc/twl6030-gpadc.c#L496. But in any case there is no mention of the scale attribute, so to make it comply with the condition I dont see an issue adding it to the properties

Regards,
Mithil

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

[Linus Walleij]

On Sun, Sep 3, 2023 at 7:11 PM Mighty <bavishimithil@gmail.com> wrote:
> On Thu, Aug 24, 2023 at 1:54 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > Seems reasonable to me!
>
> I'd say I send a patch to the mailing list and see the response, I'm not very
> experienced with this device. The inputs of other people who worked on
> this driver would guide me in the right way then i guess.

I sent a patch, it's at v2 already, sorry for not notifying:
https://lore.kernel.org/linux-iio/20230902-iio-rescale-only-offset-v2-1-988b807754c8@linaro.org/T/#u

Yours,
Linus Walleij

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

[Mighty]

On Mon, Aug 28, 2023 at 11:48 PM Jonathan Cameron <jic23@kernel.org> wrote:

> 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).

In this very specific case yes, it is being used as a current sense shunt for a light+prox sensor (gp2ap002), so I do think that it might be case 2 instead of 3. But with no other devices using the twl6030/32 gpadc for any features it could also be due to it not being updated like case 3. Also the fact that the adc would break in cases when its not just a light sensor as well, we just dont have any such devices yet.
I'm pretty lost at how the code handles RAW and PROCESSED anyways, cant seem to find a proper rescaler. Ideally BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) should be there in it, but since SCALE isnt used anywhere in the driver it wouldnt break any functionality, but it would lose logic. We'd have to look into the working of the gpadc again to understand how the factors fit in there.

Regards,
Mithil

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

[Linus Walleij]

On Sun, Sep 3, 2023 at 8:04 PM Mighty <bavishimithil@gmail.com> wrote:
> On Mon, Aug 28, 2023 at 11:48 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> > 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).
>
> In this very specific case yes, it is being used as a current sense shunt for a
> light+prox sensor (gp2ap002), so I do think that it might be case 2 instead of 3.
> But with no other devices using the twl6030/32 gpadc for any features it could
> also be due to it not being updated like case 3.

See if my patch fixes the issue for you, else we need to patch twl6030 as
indicated.

> Also the fact that the adc would break in cases when its not just a light sensor
> as well, we just dont have any such devices yet.

I have tested the gp2ap002 both with and without light sensor.
They have different product numbers and thus different device tree
compatibles if the component supports light sensoring and not just
proximity detection, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/iio/light/sharp,gp2ap002.yaml
So just use the right compatible (if you're using device tree) and
it'll be fine.

Yours,
Linus Walleij