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