* [PATCH] iio: adc: max9611: Fix temperature reading in probe @ 2019-08-05 15:55 Jacopo Mondi 2019-08-05 17:12 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Jacopo Mondi @ 2019-08-05 15:55 UTC (permalink / raw) To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Kuninori Morimoto, Stefan Agner Cc: Jacopo Mondi, linux-iio, linux-kernel 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 <jacopo+renesas@jmondi.org> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: max9611: Fix temperature reading in probe 2019-08-05 15:55 [PATCH] iio: adc: max9611: Fix temperature reading in probe Jacopo Mondi @ 2019-08-05 17:12 ` Jonathan Cameron 2019-08-06 7:31 ` Jacopo Mondi 2019-08-21 11:28 ` Geert Uytterhoeven 0 siblings, 2 replies; 6+ messages in thread From: Jonathan Cameron @ 2019-08-05 17:12 UTC (permalink / raw) To: Jacopo Mondi Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Kuninori Morimoto, Stefan Agner, linux-iio, linux-kernel On Mon, 5 Aug 2019 17:55:15 +0200 Jacopo Mondi <jacopo+renesas@jmondi.org> 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 <jacopo+renesas@jmondi.org> 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: max9611: Fix temperature reading in probe 2019-08-05 17:12 ` Jonathan Cameron @ 2019-08-06 7:31 ` Jacopo Mondi 2019-08-11 8:24 ` Jonathan Cameron 2019-08-21 11:28 ` Geert Uytterhoeven 1 sibling, 1 reply; 6+ messages in thread From: Jacopo Mondi @ 2019-08-06 7:31 UTC (permalink / raw) To: Jonathan Cameron Cc: Jacopo Mondi, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Kuninori Morimoto, Stefan Agner, linux-iio, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1823 bytes --] 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 <jacopo+renesas@jmondi.org> 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 <jacopo+renesas@jmondi.org> > > 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 > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: max9611: Fix temperature reading in probe 2019-08-06 7:31 ` Jacopo Mondi @ 2019-08-11 8:24 ` Jonathan Cameron 0 siblings, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2019-08-11 8:24 UTC (permalink / raw) To: Jacopo Mondi Cc: Jacopo Mondi, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Kuninori Morimoto, Stefan Agner, linux-iio, linux-kernel On Tue, 6 Aug 2019 09:31:14 +0200 Jacopo Mondi <jacopo@jmondi.org> 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 <jacopo+renesas@jmondi.org> 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 <jacopo+renesas@jmondi.org> > > > > 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 > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: max9611: Fix temperature reading in probe 2019-08-05 17:12 ` Jonathan Cameron 2019-08-06 7:31 ` Jacopo Mondi @ 2019-08-21 11:28 ` Geert Uytterhoeven 2019-08-21 12:34 ` Jacopo Mondi 1 sibling, 1 reply; 6+ messages in thread From: Geert Uytterhoeven @ 2019-08-21 11:28 UTC (permalink / raw) To: Jonathan Cameron, Jacopo Mondi Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Kuninori Morimoto, Stefan Agner, linux-iio, Linux Kernel Mailing List, Linux-Renesas, stable Hi Jonathan, Jacopo, On Mon, Aug 5, 2019 at 7:15 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 5 Aug 2019 17:55:15 +0200 > Jacopo Mondi <jacopo+renesas@jmondi.org> 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 <jacopo+renesas@jmondi.org> > > 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 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adc: max9611: Fix temperature reading in probe 2019-08-21 11:28 ` Geert Uytterhoeven @ 2019-08-21 12:34 ` Jacopo Mondi 0 siblings, 0 replies; 6+ messages in thread From: Jacopo Mondi @ 2019-08-21 12:34 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Jonathan Cameron, Jacopo Mondi, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Kuninori Morimoto, Stefan Agner, linux-iio, Linux Kernel Mailing List, Linux-Renesas, stable [-- Attachment #1: Type: text/plain, Size: 3281 bytes --] 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 <jic23@kernel.org> wrote: > > On Mon, 5 Aug 2019 17:55:15 +0200 > > Jacopo Mondi <jacopo+renesas@jmondi.org> 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 <jacopo+renesas@jmondi.org> > > > > 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 -- geert@linux-m68k.org > > 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-21 12:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-05 15:55 [PATCH] iio: adc: max9611: Fix temperature reading in probe Jacopo Mondi 2019-08-05 17:12 ` Jonathan Cameron 2019-08-06 7:31 ` Jacopo Mondi 2019-08-11 8:24 ` Jonathan Cameron 2019-08-21 11:28 ` Geert Uytterhoeven 2019-08-21 12:34 ` Jacopo Mondi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).