Re: [PATCH 04/12] iio: adc: max9611: Fix misuse of GENMASK macro

[Jacopo Mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]

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
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]