[04/12] iio: adc: max9611: Fix misuse of GENMASK macro
diff mbox series

Message ID 2929234bd4ecec41c0d012edc52416ef80f3e368.1562734889.git.joe@perches.com
State New
Headers show
Series
  • treewide: Fix GENMASK misuses
Related show

Commit Message

Joe Perches July 10, 2019, 5:04 a.m. UTC
Arguments are supposed to be ordered high then low.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/iio/adc/max9611.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jonathan Cameron July 14, 2019, 11:54 a.m. UTC | #1
On Tue,  9 Jul 2019 22:04:17 -0700
Joe Perches <joe@perches.com> wrote:

> Arguments are supposed to be ordered high then low.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
Applied to the fixes-togreg branch of iio.git and marked for
stable etc.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/max9611.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> index 917223d5ff5b..0e3c6529fc4c 100644
> --- a/drivers/iio/adc/max9611.c
> +++ b/drivers/iio/adc/max9611.c
> @@ -83,7 +83,7 @@
>  #define MAX9611_TEMP_MAX_POS		0x7f80
>  #define MAX9611_TEMP_MAX_NEG		0xff80
>  #define MAX9611_TEMP_MIN_NEG		0xd980
> -#define MAX9611_TEMP_MASK		GENMASK(7, 15)
> +#define MAX9611_TEMP_MASK		GENMASK(15, 7)
>  #define MAX9611_TEMP_SHIFT		0x07
>  #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
>  #define MAX9611_TEMP_SCALE_NUM		1000000
Joe Perches July 14, 2019, 12:19 p.m. UTC | #2
On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote:
> On Tue,  9 Jul 2019 22:04:17 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > Arguments are supposed to be ordered high then low.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> 
> Applied to the fixes-togreg branch of iio.git and marked for
> stable etc.

This mask is used in an init function called from a probe.

I don't have this hardware but it looks as if it could
never have worked so I doubt the driver and the hardware
have ever been tested.

Does anyone have this device in actual use?


	regval = ret & MAX9611_TEMP_MASK;

	if ((regval > MAX9611_TEMP_MAX_POS &&
	     regval < MAX9611_TEMP_MIN_NEG) ||
	     regval > MAX9611_TEMP_MAX_NEG) {
		dev_err(max9611->dev,
			"Invalid value received from ADC 0x%4x: aborting\n",
			regval);
		return -EIO;
	}


> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/max9611.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > index 917223d5ff5b..0e3c6529fc4c 100644
> > --- a/drivers/iio/adc/max9611.c
> > +++ b/drivers/iio/adc/max9611.c
> > @@ -83,7 +83,7 @@
> >  #define MAX9611_TEMP_MAX_POS		0x7f80
> >  #define MAX9611_TEMP_MAX_NEG		0xff80
> >  #define MAX9611_TEMP_MIN_NEG		0xd980
> > -#define MAX9611_TEMP_MASK		GENMASK(7, 15)
> > +#define MAX9611_TEMP_MASK		GENMASK(15, 7)
> >  #define MAX9611_TEMP_SHIFT		0x07
> >  #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
> >  #define MAX9611_TEMP_SCALE_NUM		1000000
Jacopo Mondi July 14, 2019, 2:37 p.m. UTC | #3
Hi Joe, Jonathan,

On Sun, Jul 14, 2019 at 05:19:32AM -0700, Joe Perches wrote:
> On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote:
> > On Tue,  9 Jul 2019 22:04:17 -0700
> > Joe Perches <joe@perches.com> wrote:
> >
> > > Arguments are supposed to be ordered high then low.
> > >
> > > Signed-off-by: Joe Perches <joe@perches.com>
> >
> > Applied to the fixes-togreg branch of iio.git and marked for
> > stable etc.
>
> This mask is used in an init function called from a probe.
>
> I don't have this hardware but it looks as if it could
> never have worked so I doubt the driver and the hardware
> have ever been tested.
>

Ups, this is embarassing! Thanks for noticing

I actually tested the device before sending the driver of course

> Does anyone have this device in actual use?
>

The driver is currently in use in Renesas Gen3 Salvator boards:
arch/arm64/boot/dts/renesas/salvator-common.dtsi:466

I might need some time before I can actually test and tell what's
happening though. It might work by pure chance. Fortunately the mask
is only used for validation of the temperature reading at probe time,
and I can tell this passes (we would have noticed otherwise, Salvator
is one of the reference Gen3 platforms for upstream development).

As said I might need some time before I can test this, but the change
is indeed correct.

Thanks
   j

>
> 	regval = ret & MAX9611_TEMP_MASK;
>
> 	if ((regval > MAX9611_TEMP_MAX_POS &&
> 	     regval < MAX9611_TEMP_MIN_NEG) ||
> 	     regval > MAX9611_TEMP_MAX_NEG) {
> 		dev_err(max9611->dev,
> 			"Invalid value received from ADC 0x%4x: aborting\n",
> 			regval);
> 		return -EIO;
> 	}
>
>
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > >  drivers/iio/adc/max9611.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > > index 917223d5ff5b..0e3c6529fc4c 100644
> > > --- a/drivers/iio/adc/max9611.c
> > > +++ b/drivers/iio/adc/max9611.c
> > > @@ -83,7 +83,7 @@
> > >  #define MAX9611_TEMP_MAX_POS		0x7f80
> > >  #define MAX9611_TEMP_MAX_NEG		0xff80
> > >  #define MAX9611_TEMP_MIN_NEG		0xd980
> > > -#define MAX9611_TEMP_MASK		GENMASK(7, 15)
> > > +#define MAX9611_TEMP_MASK		GENMASK(15, 7)
> > >  #define MAX9611_TEMP_SHIFT		0x07
> > >  #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
> > >  #define MAX9611_TEMP_SCALE_NUM		1000000
>
Jacopo Mondi July 29, 2019, 9:52 p.m. UTC | #4
Hello,
  so I finally run some test and...

On Sun, Jul 14, 2019 at 05:19:32AM -0700, Joe Perches wrote:
> On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote:
> > On Tue,  9 Jul 2019 22:04:17 -0700
> > Joe Perches <joe@perches.com> wrote:
> >
> > > Arguments are supposed to be ordered high then low.
> > >
> > > Signed-off-by: Joe Perches <joe@perches.com>
> >
> > Applied to the fixes-togreg branch of iio.git and marked for
> > stable etc.

I don't see it in v5.3-rc2, has it been collected or are we still in
time for an additional fix?

>
> This mask is used in an init function called from a probe.
>
> I don't have this hardware but it looks as if it could
> never have worked so I doubt the driver and the hardware
> have ever been tested.
>
> Does anyone have this device in actual use?

Because it turns out this is 2 times embarrassing. The mask definition
is indeed wrong, as Joe reported and fixed, but also this line
>
> 	regval = ret & MAX9611_TEMP_MASK;

is very wrong as regval is read as:
        ret = max9611_read_single(max9611, CONF_TEMP, &regval);

So that should actually be:
        regval &= MAX9611_TEMP_MASK;
not
 	regval = ret & MAX9611_TEMP_MASK;
Ups...

Yes, it worked by chance, as regval was always 0, which is in the
range of acceptable temperatures :/

>
> 	if ((regval > MAX9611_TEMP_MAX_POS &&
> 	     regval < MAX9611_TEMP_MIN_NEG) ||
> 	     regval > MAX9611_TEMP_MAX_NEG) {

Also reading this condition and how I had defined the temperature
calculation formula makes me wonder if this it totally correct, but
for the moment:

1) if Joe's patch has been collected, I can send an additional patch to
fix how regval is computed.
2) If Joe's patch still have to be collected, the regval computation
might be fixed there.

Sorry for taking so long to get back to you and thanks for noticing.

Thanks
  j

> 		dev_err(max9611->dev,
> 			"Invalid value received from ADC 0x%4x: aborting\n",
> 			regval);
> 		return -EIO;
> 	}
>
>
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > >  drivers/iio/adc/max9611.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > > index 917223d5ff5b..0e3c6529fc4c 100644
> > > --- a/drivers/iio/adc/max9611.c
> > > +++ b/drivers/iio/adc/max9611.c
> > > @@ -83,7 +83,7 @@
> > >  #define MAX9611_TEMP_MAX_POS		0x7f80
> > >  #define MAX9611_TEMP_MAX_NEG		0xff80
> > >  #define MAX9611_TEMP_MIN_NEG		0xd980
> > > -#define MAX9611_TEMP_MASK		GENMASK(7, 15)
> > > +#define MAX9611_TEMP_MASK		GENMASK(15, 7)
> > >  #define MAX9611_TEMP_SHIFT		0x07
> > >  #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
> > >  #define MAX9611_TEMP_SCALE_NUM		1000000
>
Jonathan Cameron July 31, 2019, 8:37 a.m. UTC | #5
On Mon, 29 Jul 2019 23:52:14 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hello,
>   so I finally run some test and...
> 
> On Sun, Jul 14, 2019 at 05:19:32AM -0700, Joe Perches wrote:
> > On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote:  
> > > On Tue,  9 Jul 2019 22:04:17 -0700
> > > Joe Perches <joe@perches.com> wrote:
> > >  
> > > > Arguments are supposed to be ordered high then low.
> > > >
> > > > Signed-off-by: Joe Perches <joe@perches.com>  
> > >
> > > Applied to the fixes-togreg branch of iio.git and marked for
> > > stable etc.  
> 
> I don't see it in v5.3-rc2, has it been collected or are we still in
> time for an additional fix?
> 
> >
> > This mask is used in an init function called from a probe.
> >
> > I don't have this hardware but it looks as if it could
> > never have worked so I doubt the driver and the hardware
> > have ever been tested.
> >
> > Does anyone have this device in actual use?  
> 
> Because it turns out this is 2 times embarrassing. The mask definition
> is indeed wrong, as Joe reported and fixed, but also this line
> >
> > 	regval = ret & MAX9611_TEMP_MASK;  
> 
> is very wrong as regval is read as:
>         ret = max9611_read_single(max9611, CONF_TEMP, &regval);
> 
> So that should actually be:
>         regval &= MAX9611_TEMP_MASK;
> not
>  	regval = ret & MAX9611_TEMP_MASK;
> Ups...
> 
> Yes, it worked by chance, as regval was always 0, which is in the
> range of acceptable temperatures :/
> 
> >
> > 	if ((regval > MAX9611_TEMP_MAX_POS &&
> > 	     regval < MAX9611_TEMP_MIN_NEG) ||
> > 	     regval > MAX9611_TEMP_MAX_NEG) {  
> 
> Also reading this condition and how I had defined the temperature
> calculation formula makes me wonder if this it totally correct, but
> for the moment:
> 
> 1) if Joe's patch has been collected, I can send an additional patch to
> fix how regval is computed.
> 2) If Joe's patch still have to be collected, the regval computation
> might be fixed there.

I think this will have hit linux-next on the same day as your email.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/iio/adc?id=ae8cc91a7d85e018c0c267f580820b2bb558cd48

So follow up patch please.

Thanks!

Jonathan
> 
> Sorry for taking so long to get back to you and thanks for noticing.
> 
> Thanks
>   j
> 
> > 		dev_err(max9611->dev,
> > 			"Invalid value received from ADC 0x%4x: aborting\n",
> > 			regval);
> > 		return -EIO;
> > 	}
> >
> >  
> > > Thanks,
> > >
> > > Jonathan
> > >  
> > > > ---
> > > >  drivers/iio/adc/max9611.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > > > index 917223d5ff5b..0e3c6529fc4c 100644
> > > > --- a/drivers/iio/adc/max9611.c
> > > > +++ b/drivers/iio/adc/max9611.c
> > > > @@ -83,7 +83,7 @@
> > > >  #define MAX9611_TEMP_MAX_POS		0x7f80
> > > >  #define MAX9611_TEMP_MAX_NEG		0xff80
> > > >  #define MAX9611_TEMP_MIN_NEG		0xd980
> > > > -#define MAX9611_TEMP_MASK		GENMASK(7, 15)
> > > > +#define MAX9611_TEMP_MASK		GENMASK(15, 7)
> > > >  #define MAX9611_TEMP_SHIFT		0x07
> > > >  #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
> > > >  #define MAX9611_TEMP_SCALE_NUM		1000000  
> >  
>

Patch
diff mbox series

diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
index 917223d5ff5b..0e3c6529fc4c 100644
--- a/drivers/iio/adc/max9611.c
+++ b/drivers/iio/adc/max9611.c
@@ -83,7 +83,7 @@ 
 #define MAX9611_TEMP_MAX_POS		0x7f80
 #define MAX9611_TEMP_MAX_NEG		0xff80
 #define MAX9611_TEMP_MIN_NEG		0xd980
-#define MAX9611_TEMP_MASK		GENMASK(7, 15)
+#define MAX9611_TEMP_MASK		GENMASK(15, 7)
 #define MAX9611_TEMP_SHIFT		0x07
 #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
 #define MAX9611_TEMP_SCALE_NUM		1000000