diff mbox series

iio: afe: rescale: Fix logic bug

Message ID 20220524075448.140238-1-linus.walleij@linaro.org (mailing list archive)
State Accepted
Headers show
Series iio: afe: rescale: Fix logic bug | expand

Commit Message

Linus Walleij May 24, 2022, 7:54 a.m. UTC
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(-)

Comments

Peter Rosin May 25, 2022, 2:08 p.m. UTC | #1
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)) {
Liam Beguin May 26, 2022, 1:29 a.m. UTC | #2
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
>
Jonathan Cameron May 28, 2022, 5:01 p.m. UTC | #3
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
> >
diff mbox series

Patch

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