Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] staging:iio:ad7150: fix threshold mode config bit
@ 2019-05-19  1:04 Melissa Wen
  2019-05-19 10:29 ` Jonathan Cameron
  0 siblings, 1 reply; 3+ messages in thread
From: Melissa Wen @ 2019-05-19  1:04 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
	Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
	Greg Kroah-Hartman, Barry Song
  Cc: linux-iio, devel, linux-kernel, kernel-usp

According to the AD7150 configuration register description, bit 7 assumes
value 1 when the threshold mode is fixed and 0 when it is adaptive,
however, the operation that identifies this mode was considering the
opposite values.

This patch renames the boolean variable to describe it correctly and
properly replaces it in the places where it is used.

Fixes: 531efd6aa0991 ("staging:iio:adc:ad7150: chan_spec conv + i2c_smbus commands + drop unused poweroff timeout control.")
Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
---
 drivers/staging/iio/cdc/ad7150.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index dd7fcab8e19e..e075244c602b 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -5,6 +5,7 @@
  * Copyright 2010-2011 Analog Devices Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
@@ -130,7 +131,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
 {
 	int ret;
 	u8 threshtype;
-	bool adaptive;
+	bool thrfixed;
 	struct ad7150_chip_info *chip = iio_priv(indio_dev);
 
 	ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
@@ -138,21 +139,23 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
 		return ret;
 
 	threshtype = (ret >> 5) & 0x03;
-	adaptive = !!(ret & 0x80);
+
+	/*check if threshold mode is fixed or adaptive*/
+	thrfixed = FIELD_GET(AD7150_CFG_FIX, ret);
 
 	switch (type) {
 	case IIO_EV_TYPE_MAG_ADAPTIVE:
 		if (dir == IIO_EV_DIR_RISING)
-			return adaptive && (threshtype == 0x1);
-		return adaptive && (threshtype == 0x0);
+			return !thrfixed && (threshtype == 0x1);
+		return !thrfixed && (threshtype == 0x0);
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
 		if (dir == IIO_EV_DIR_RISING)
-			return adaptive && (threshtype == 0x3);
-		return adaptive && (threshtype == 0x2);
+			return !thrfixed && (threshtype == 0x3);
+		return !thrfixed && (threshtype == 0x2);
 	case IIO_EV_TYPE_THRESH:
 		if (dir == IIO_EV_DIR_RISING)
-			return !adaptive && (threshtype == 0x1);
-		return !adaptive && (threshtype == 0x0);
+			return thrfixed && (threshtype == 0x1);
+		return thrfixed && (threshtype == 0x0);
 	default:
 		break;
 	}
-- 
2.20.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] staging:iio:ad7150: fix threshold mode config bit
  2019-05-19  1:04 [PATCH] staging:iio:ad7150: fix threshold mode config bit Melissa Wen
@ 2019-05-19 10:29 ` Jonathan Cameron
  2019-05-20  9:59   ` Alexandru Ardelean
  0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2019-05-19 10:29 UTC (permalink / raw)
  To: Melissa Wen
  Cc: Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
	Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
	Barry Song, linux-iio, devel, linux-kernel, kernel-usp

On Sat, 18 May 2019 22:04:56 -0300
Melissa Wen <melissa.srw@gmail.com> wrote:

> According to the AD7150 configuration register description, bit 7 assumes
> value 1 when the threshold mode is fixed and 0 when it is adaptive,
> however, the operation that identifies this mode was considering the
> opposite values.
> 
> This patch renames the boolean variable to describe it correctly and
> properly replaces it in the places where it is used.
> 
> Fixes: 531efd6aa0991 ("staging:iio:adc:ad7150: chan_spec conv + i2c_smbus commands + drop unused poweroff timeout control.")
> Signed-off-by: Melissa Wen <melissa.srw@gmail.com>

Looks good to me.  Applied to the fixes-togreg branch of iio.git pushed out as
as testing-fixes for the autobuilders to see if they can find anything
we have missed.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/cdc/ad7150.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index dd7fcab8e19e..e075244c602b 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -5,6 +5,7 @@
>   * Copyright 2010-2011 Analog Devices Inc.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/interrupt.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
> @@ -130,7 +131,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
>  {
>  	int ret;
>  	u8 threshtype;
> -	bool adaptive;
> +	bool thrfixed;
>  	struct ad7150_chip_info *chip = iio_priv(indio_dev);
>  
>  	ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> @@ -138,21 +139,23 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
>  		return ret;
>  
>  	threshtype = (ret >> 5) & 0x03;
> -	adaptive = !!(ret & 0x80);
> +
> +	/*check if threshold mode is fixed or adaptive*/
> +	thrfixed = FIELD_GET(AD7150_CFG_FIX, ret);
>  
>  	switch (type) {
>  	case IIO_EV_TYPE_MAG_ADAPTIVE:
>  		if (dir == IIO_EV_DIR_RISING)
> -			return adaptive && (threshtype == 0x1);
> -		return adaptive && (threshtype == 0x0);
> +			return !thrfixed && (threshtype == 0x1);
> +		return !thrfixed && (threshtype == 0x0);
>  	case IIO_EV_TYPE_THRESH_ADAPTIVE:
>  		if (dir == IIO_EV_DIR_RISING)
> -			return adaptive && (threshtype == 0x3);
> -		return adaptive && (threshtype == 0x2);
> +			return !thrfixed && (threshtype == 0x3);
> +		return !thrfixed && (threshtype == 0x2);
>  	case IIO_EV_TYPE_THRESH:
>  		if (dir == IIO_EV_DIR_RISING)
> -			return !adaptive && (threshtype == 0x1);
> -		return !adaptive && (threshtype == 0x0);
> +			return thrfixed && (threshtype == 0x1);
> +		return thrfixed && (threshtype == 0x0);
>  	default:
>  		break;
>  	}


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] staging:iio:ad7150: fix threshold mode config bit
  2019-05-19 10:29 ` Jonathan Cameron
@ 2019-05-20  9:59   ` Alexandru Ardelean
  0 siblings, 0 replies; 3+ messages in thread
From: Alexandru Ardelean @ 2019-05-20  9:59 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Melissa Wen, Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
	Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
	Barry Song, linux-iio, devel, LKML, kernel-usp,
	Alexandru Ardelean

On Sun, May 19, 2019 at 8:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 18 May 2019 22:04:56 -0300
> Melissa Wen <melissa.srw@gmail.com> wrote:
>
> > According to the AD7150 configuration register description, bit 7 assumes
> > value 1 when the threshold mode is fixed and 0 when it is adaptive,
> > however, the operation that identifies this mode was considering the
> > opposite values.
> >
> > This patch renames the boolean variable to describe it correctly and
> > properly replaces it in the places where it is used.
> >
> > Fixes: 531efd6aa0991 ("staging:iio:adc:ad7150: chan_spec conv + i2c_smbus commands + drop unused poweroff timeout control.")
> > Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
>
> Looks good to me.  Applied to the fixes-togreg branch of iio.git pushed out as
> as testing-fixes for the autobuilders to see if they can find anything
> we have missed.
>
> Thanks,
>
> Jonathan
>
> > ---
> >  drivers/staging/iio/cdc/ad7150.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> > index dd7fcab8e19e..e075244c602b 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -5,6 +5,7 @@
> >   * Copyright 2010-2011 Analog Devices Inc.
> >   */
> >
> > +#include <linux/bitfield.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> > @@ -130,7 +131,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
> >  {
> >       int ret;
> >       u8 threshtype;
> > -     bool adaptive;
> > +     bool thrfixed;
> >       struct ad7150_chip_info *chip = iio_priv(indio_dev);
> >
> >       ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> > @@ -138,21 +139,23 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
> >               return ret;
> >
> >       threshtype = (ret >> 5) & 0x03;
> > -     adaptive = !!(ret & 0x80);
> > +
> > +     /*check if threshold mode is fixed or adaptive*/
> > +     thrfixed = FIELD_GET(AD7150_CFG_FIX, ret);

nitpick: i would have kept the original variable name as "adaptive",
mostly for consistency.
"adaptive" is used in other places as well;

as i recall, the fix is just oneliner in this case:

- adaptive = !!(ret & 0x80);
+ adaptive = !(ret & 0x80);


> >
> >       switch (type) {
> >       case IIO_EV_TYPE_MAG_ADAPTIVE:
> >               if (dir == IIO_EV_DIR_RISING)
> > -                     return adaptive && (threshtype == 0x1);
> > -             return adaptive && (threshtype == 0x0);
> > +                     return !thrfixed && (threshtype == 0x1);
> > +             return !thrfixed && (threshtype == 0x0);
> >       case IIO_EV_TYPE_THRESH_ADAPTIVE:
> >               if (dir == IIO_EV_DIR_RISING)
> > -                     return adaptive && (threshtype == 0x3);
> > -             return adaptive && (threshtype == 0x2);
> > +                     return !thrfixed && (threshtype == 0x3);
> > +             return !thrfixed && (threshtype == 0x2);
> >       case IIO_EV_TYPE_THRESH:
> >               if (dir == IIO_EV_DIR_RISING)
> > -                     return !adaptive && (threshtype == 0x1);
> > -             return !adaptive && (threshtype == 0x0);
> > +                     return thrfixed && (threshtype == 0x1);
> > +             return thrfixed && (threshtype == 0x0);
> >       default:
> >               break;
> >       }
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-19  1:04 [PATCH] staging:iio:ad7150: fix threshold mode config bit Melissa Wen
2019-05-19 10:29 ` Jonathan Cameron
2019-05-20  9:59   ` Alexandru Ardelean

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox