iio: adc: max9611: Fix temperature reading in probe

Commit Message

Jacopo Mondi Aug. 5, 2019, 3:55 p.m. UTC

The max9611 driver reads the die temperature at probe time to validate
the communication channel. Use the actual read value to perform the test
instead of the read function return value, which was mistakenly used so
far.

The temperature reading test was only successful because the 0 return
value is in the range of supported temperatures.

Fixes: 69780a3bbc0b ("iio: adc: Add Maxim max9611 ADC driver")
Signed-off-by: Jacopo Mondi <[e-mail]>
---
 drivers/iio/adc/max9611.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.22.0

Comments

Jonathan Cameron Aug. 5, 2019, 5:12 p.m. UTC | #1

On Mon,  5 Aug 2019 17:55:15 +0200
Jacopo Mondi <[e-mail]> wrote:

> The max9611 driver reads the die temperature at probe time to validate
> the communication channel. Use the actual read value to perform the test
> instead of the read function return value, which was mistakenly used so
> far.
> 
> The temperature reading test was only successful because the 0 return
> value is in the range of supported temperatures.
> 
> Fixes: 69780a3bbc0b ("iio: adc: Add Maxim max9611 ADC driver")
> Signed-off-by: Jacopo Mondi <[e-mail]>

Applied to the fixes-togreg branch of iio.git and marked for
stable.  That'll be a bit fiddly given other changes around this
so we may need to do backports.


> ---
>  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..e9f6b1da1b94 100644
> --- a/drivers/iio/adc/max9611.c
> +++ b/drivers/iio/adc/max9611.c
> @@ -480,7 +480,7 @@ static int max9611_init(struct max9611_dev *max9611)
>  	if (ret)
>  		return ret;
> 
> -	regval = ret & MAX9611_TEMP_MASK;
> +	regval &= MAX9611_TEMP_MASK;
> 
>  	if ((regval > MAX9611_TEMP_MAX_POS &&
>  	     regval < MAX9611_TEMP_MIN_NEG) ||
> --
> 2.22.0
>

Jacopo Mondi Aug. 6, 2019, 7:31 a.m. UTC | #2

Hi Jonathan,

On Mon, Aug 05, 2019 at 06:12:44PM +0100, Jonathan Cameron wrote:
> On Mon,  5 Aug 2019 17:55:15 +0200
> Jacopo Mondi <[e-mail]> wrote:
>
> > The max9611 driver reads the die temperature at probe time to validate
> > the communication channel. Use the actual read value to perform the test
> > instead of the read function return value, which was mistakenly used so
> > far.
> >
> > The temperature reading test was only successful because the 0 return
> > value is in the range of supported temperatures.
> >
> > Fixes: 69780a3bbc0b ("iio: adc: Add Maxim max9611 ADC driver")
> > Signed-off-by: Jacopo Mondi <[e-mail]>
>
> Applied to the fixes-togreg branch of iio.git and marked for
> stable.  That'll be a bit fiddly given other changes around this
> so we may need to do backports.
>

Indeed, I should have mentioned this patch depends on Joe's
ae8cc91a7d85 ("iio: adc: max9611: Fix misuse of GENMASK macro")
which is now in linux-next, otherwise it might atually trigger errors
due to the wrong mask value.

I wonder if there's a way to keep track of these dependencies for the
sake of backporting, or it's an operation that has to be carried out
manually...

Thanks
   j

>
> > ---
> >  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..e9f6b1da1b94 100644
> > --- a/drivers/iio/adc/max9611.c
> > +++ b/drivers/iio/adc/max9611.c
> > @@ -480,7 +480,7 @@ static int max9611_init(struct max9611_dev *max9611)
> >  	if (ret)
> >  		return ret;
> >
> > -	regval = ret & MAX9611_TEMP_MASK;
> > +	regval &= MAX9611_TEMP_MASK;
> >
> >  	if ((regval > MAX9611_TEMP_MAX_POS &&
> >  	     regval < MAX9611_TEMP_MIN_NEG) ||
> > --
> > 2.22.0
> >
>

Jonathan Cameron Aug. 11, 2019, 8:24 a.m. UTC | #3

On Tue, 6 Aug 2019 09:31:14 +0200
Jacopo Mondi <[e-mail]> wrote:

> Hi Jonathan,
> 
> On Mon, Aug 05, 2019 at 06:12:44PM +0100, Jonathan Cameron wrote:
> > On Mon,  5 Aug 2019 17:55:15 +0200
> > Jacopo Mondi <[e-mail]> wrote:
> >  
> > > The max9611 driver reads the die temperature at probe time to validate
> > > the communication channel. Use the actual read value to perform the test
> > > instead of the read function return value, which was mistakenly used so
> > > far.
> > >
> > > The temperature reading test was only successful because the 0 return
> > > value is in the range of supported temperatures.
> > >
> > > Fixes: 69780a3bbc0b ("iio: adc: Add Maxim max9611 ADC driver")
> > > Signed-off-by: Jacopo Mondi <[e-mail]>  
> >
> > Applied to the fixes-togreg branch of iio.git and marked for
> > stable.  That'll be a bit fiddly given other changes around this
> > so we may need to do backports.
> >  
> 
> Indeed, I should have mentioned this patch depends on Joe's
> ae8cc91a7d85 ("iio: adc: max9611: Fix misuse of GENMASK macro")
> which is now in linux-next, otherwise it might atually trigger errors
> due to the wrong mask value.
> 
> I wonder if there's a way to keep track of these dependencies for the
> sake of backporting, or it's an operation that has to be carried out
> manually...
A note in the commit message is normally enough as all the stable
maintainers check that first.  In this particular case both patches
are marked for stable so will get picked up automatically in the right
order (hopefully!).

Thanks,

Jonathan

> 
> Thanks
>    j
> 
> >  
> > > ---
> > >  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..e9f6b1da1b94 100644
> > > --- a/drivers/iio/adc/max9611.c
> > > +++ b/drivers/iio/adc/max9611.c
> > > @@ -480,7 +480,7 @@ static int max9611_init(struct max9611_dev *max9611)
> > >  	if (ret)
> > >  		return ret;
> > >
> > > -	regval = ret & MAX9611_TEMP_MASK;
> > > +	regval &= MAX9611_TEMP_MASK;
> > >
> > >  	if ((regval > MAX9611_TEMP_MAX_POS &&
> > >  	     regval < MAX9611_TEMP_MIN_NEG) ||
> > > --
> > > 2.22.0
> > >  
> >

Geert Uytterhoeven Aug. 21, 2019, 11:28 a.m. UTC | #4

Hi Jonathan, Jacopo,

On Mon, Aug 5, 2019 at 7:15 PM Jonathan Cameron <[e-mail]> wrote:
> On Mon,  5 Aug 2019 17:55:15 +0200
> Jacopo Mondi <[e-mail]> wrote:
>
> > The max9611 driver reads the die temperature at probe time to validate
> > the communication channel. Use the actual read value to perform the test
> > instead of the read function return value, which was mistakenly used so
> > far.
> >
> > The temperature reading test was only successful because the 0 return
> > value is in the range of supported temperatures.
> >
> > Fixes: 69780a3bbc0b ("iio: adc: Add Maxim max9611 ADC driver")
> > Signed-off-by: Jacopo Mondi <[e-mail]>
>
> Applied to the fixes-togreg branch of iio.git and marked for
> stable.  That'll be a bit fiddly given other changes around this
> so we may need to do backports.

This is now commit b9ddd5091160793e ("iio: adc: max9611: Fix temperature
reading in probe") in v5.3-rc5, and has been backported to 4.14, 4.19,
and 5.2.

> > --- a/drivers/iio/adc/max9611.c
> > +++ b/drivers/iio/adc/max9611.c
> > @@ -480,7 +480,7 @@ static int max9611_init(struct max9611_dev *max9611)
> >       if (ret)
> >               return ret;
> >
> > -     regval = ret & MAX9611_TEMP_MASK;
> > +     regval &= MAX9611_TEMP_MASK;
> >
> >       if ((regval > MAX9611_TEMP_MAX_POS &&
> >            regval < MAX9611_TEMP_MIN_NEG) ||

While this did fix a bug, it also introduced a regression: on Salvator-XS,
which has two max9611 instances, I now see intermittent failures

    max9611 4-007c: Invalid value received from ADC 0x8000: aborting
    max9611: probe of 4-007c failed with error -5

and/or

    max9611 4-007f: Invalid value received from ADC 0x8000: aborting
    max9611: probe of 4-007f failed with error -5

during boot.

Retrying on failure fixes the issue, e.g.:

    max9611_init:483: regval = 0x8000
    max9611 4-007f: Invalid value received from ADC 0x8000: aborting
    max9611_init:483: regval = 0x2780

According to the datasheet, 0x8000 is the Power-On Reset value.
Looks like it should be ignored, and retried?

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Jacopo Mondi Aug. 21, 2019, 12:34 p.m. UTC | #5

Hi Geert

On Wed, Aug 21, 2019 at 01:28:16PM +0200, Geert Uytterhoeven wrote:
> Hi Jonathan, Jacopo,
>
> On Mon, Aug 5, 2019 at 7:15 PM Jonathan Cameron <[e-mail]> wrote:
> > On Mon,  5 Aug 2019 17:55:15 +0200
> > Jacopo Mondi <[e-mail]> wrote:
> >
> > > The max9611 driver reads the die temperature at probe time to validate
> > > the communication channel. Use the actual read value to perform the test
> > > instead of the read function return value, which was mistakenly used so
> > > far.
> > >
> > > The temperature reading test was only successful because the 0 return
> > > value is in the range of supported temperatures.
> > >
> > > Fixes: 69780a3bbc0b ("iio: adc: Add Maxim max9611 ADC driver")
> > > Signed-off-by: Jacopo Mondi <[e-mail]>
> >
> > Applied to the fixes-togreg branch of iio.git and marked for
> > stable.  That'll be a bit fiddly given other changes around this
> > so we may need to do backports.
>
> This is now commit b9ddd5091160793e ("iio: adc: max9611: Fix temperature
> reading in probe") in v5.3-rc5, and has been backported to 4.14, 4.19,
> and 5.2.
>
> > > --- a/drivers/iio/adc/max9611.c
> > > +++ b/drivers/iio/adc/max9611.c
> > > @@ -480,7 +480,7 @@ static int max9611_init(struct max9611_dev *max9611)
> > >       if (ret)
> > >               return ret;
> > >
> > > -     regval = ret & MAX9611_TEMP_MASK;
> > > +     regval &= MAX9611_TEMP_MASK;
> > >
> > >       if ((regval > MAX9611_TEMP_MAX_POS &&
> > >            regval < MAX9611_TEMP_MIN_NEG) ||
>
> While this did fix a bug, it also introduced a regression: on Salvator-XS,
> which has two max9611 instances, I now see intermittent failures
>
>     max9611 4-007c: Invalid value received from ADC 0x8000: aborting
>     max9611: probe of 4-007c failed with error -5
>
> and/or
>
>     max9611 4-007f: Invalid value received from ADC 0x8000: aborting
>     max9611: probe of 4-007f failed with error -5
>
> during boot.

AH! I didn't notice! I booted the board a few times only, maybe it
didn't trigger (it was a Salvator-X H3, not an XS, but it shouldn't
make any difference).

>
> Retrying on failure fixes the issue, e.g.:
>
>     max9611_init:483: regval = 0x8000
>     max9611 4-007f: Invalid value received from ADC 0x8000: aborting
>     max9611_init:483: regval = 0x2780
>
> According to the datasheet, 0x8000 is the Power-On Reset value.
> Looks like it should be ignored, and retried?

Indeed... I haven't found a characterization of the delay required to
release registers from their POR values after power up, so I guess we
could read the register value again with a little timeout between
reads (whose value would be arbitrary, anyway..)

I'm a bit suprised though.. The max9611 chips are powered from the
+3.3V rail, and should have exited POR long before the driver gets
to probe, isn't it?

Thanks for reporting and sorry for having missed it in first place


>
> Gr{oetje,eeting}s,
>
>                         Geert
>
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Patch

diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
index 917223d5ff5b..e9f6b1da1b94 100644
--- a/drivers/iio/adc/max9611.c
+++ b/drivers/iio/adc/max9611.c
@@ -480,7 +480,7 @@  static int max9611_init(struct max9611_dev *max9611)
 	if (ret)
 		return ret;

-	regval = ret & MAX9611_TEMP_MASK;
+	regval &= MAX9611_TEMP_MASK;

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