linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iio: adc: max9611: Fix too short conversion time delay
@ 2019-11-13  9:21 Geert Uytterhoeven
  2019-11-13  9:38 ` Jacopo Mondi
  2019-11-13 18:00 ` Wolfram Sang
  0 siblings, 2 replies; 7+ messages in thread
From: Geert Uytterhoeven @ 2019-11-13  9:21 UTC (permalink / raw)
  To: Jacopo Mondi, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler
  Cc: Wolfram Sang, linux-iio, linux-renesas-soc, linux-kernel,
	Geert Uytterhoeven

As of commit b9ddd5091160793e ("iio: adc: max9611: Fix temperature
reading in probe"), max9611 initialization sometimes fails on the
Salvator-X(S) development board with:

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

The max9611 driver tests communications with the chip by reading the die
temperature during the probe function, which returns an invalid value.

According to the datasheet, the typical ADC conversion time is 2 ms, but
no minimum or maximum values are provided.  However, the driver assumes
a 1 ms conversion time.  Usually the usleep_range() call returns after
more than 1.8 ms, hence it succeeds.  When it returns earlier, the data
register may be read too early, and the previous measurement value will
be returned.  After boot, this is the temperature POR (power-on reset)
value, causing the failure above.

Fix this by increasing the delay from 1000-2000 µs to 2000-2200 µs.

Note that this issue has always been present, but it was exposed by the
aformentioned commit.

Fixes: 69780a3bbc0b1e7e ("iio: adc: Add Maxim max9611 ADC driver")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This problem was exposed in v5.3.

After this patch, probing of the two max9611 sensors succeeded during
ca. 3000 boot cycles on Salvator-X(S) boards, equipped with various
R-Car H3/M3-W/M3-N SoCs.
---
 drivers/iio/adc/max9611.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
index da073d72f649f829..b0755f25356d700d 100644
--- a/drivers/iio/adc/max9611.c
+++ b/drivers/iio/adc/max9611.c
@@ -89,6 +89,11 @@
 #define MAX9611_TEMP_SCALE_NUM		1000000
 #define MAX9611_TEMP_SCALE_DIV		2083
 
+/*
+ * Conversion time is 2 ms (typically)
+ */
+#define MAX9611_CONV_TIME_US_RANGE	2000, 2200
+
 struct max9611_dev {
 	struct device *dev;
 	struct i2c_client *i2c_client;
@@ -238,9 +243,9 @@ static int max9611_read_single(struct max9611_dev *max9611,
 
 	/*
 	 * need a delay here to make register configuration
-	 * stabilize. 1 msec at least, from empirical testing.
+	 * stabilize.
 	 */
-	usleep_range(1000, 2000);
+	usleep_range(MAX9611_CONV_TIME_US_RANGE);
 
 	ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
 	if (ret < 0) {
@@ -507,7 +512,7 @@ static int max9611_init(struct max9611_dev *max9611)
 			MAX9611_REG_CTRL2, 0);
 		return ret;
 	}
-	usleep_range(1000, 2000);
+	usleep_range(MAX9611_CONV_TIME_US_RANGE);
 
 	return 0;
 }
-- 
2.17.1


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

* Re: [PATCH] iio: adc: max9611: Fix too short conversion time delay
  2019-11-13  9:21 [PATCH] iio: adc: max9611: Fix too short conversion time delay Geert Uytterhoeven
@ 2019-11-13  9:38 ` Jacopo Mondi
  2019-11-13  9:46   ` Geert Uytterhoeven
  2019-11-13 18:00 ` Wolfram Sang
  1 sibling, 1 reply; 7+ messages in thread
From: Jacopo Mondi @ 2019-11-13  9:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Jacopo Mondi, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Wolfram Sang,
	linux-iio, linux-renesas-soc, linux-kernel

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

Hi Geert,

On Wed, Nov 13, 2019 at 10:21:33AM +0100, Geert Uytterhoeven wrote:
> As of commit b9ddd5091160793e ("iio: adc: max9611: Fix temperature
> reading in probe"), max9611 initialization sometimes fails on the
> Salvator-X(S) development board with:
>
>     max9611 4-007f: Invalid value received from ADC 0x8000: aborting
>     max9611: probe of 4-007f failed with error -5
>
> The max9611 driver tests communications with the chip by reading the die
> temperature during the probe function, which returns an invalid value.
>
> According to the datasheet, the typical ADC conversion time is 2 ms, but
> no minimum or maximum values are provided.  However, the driver assumes
> a 1 ms conversion time.  Usually the usleep_range() call returns after
> more than 1.8 ms, hence it succeeds.  When it returns earlier, the data
> register may be read too early, and the previous measurement value will
> be returned.  After boot, this is the temperature POR (power-on reset)
> value, causing the failure above.
>
> Fix this by increasing the delay from 1000-2000 µs to 2000-2200 µs.
>
> Note that this issue has always been present, but it was exposed by the
> aformentioned commit.
>
> Fixes: 69780a3bbc0b1e7e ("iio: adc: Add Maxim max9611 ADC driver")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This problem was exposed in v5.3.
>
> After this patch, probing of the two max9611 sensors succeeded during
> ca. 3000 boot cycles on Salvator-X(S) boards, equipped with various
> R-Car H3/M3-W/M3-N SoCs.
> ---
>  drivers/iio/adc/max9611.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> index da073d72f649f829..b0755f25356d700d 100644
> --- a/drivers/iio/adc/max9611.c
> +++ b/drivers/iio/adc/max9611.c
> @@ -89,6 +89,11 @@
>  #define MAX9611_TEMP_SCALE_NUM		1000000
>  #define MAX9611_TEMP_SCALE_DIV		2083
>
> +/*
> + * Conversion time is 2 ms (typically)
> + */
> +#define MAX9611_CONV_TIME_US_RANGE	2000, 2200
> +

Is a 20% sleep range enough or should it be slightly lengthen ?

Apart from this, thanks a lot for finding the issue root cause!

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

>  struct max9611_dev {
>  	struct device *dev;
>  	struct i2c_client *i2c_client;

> @@ -238,9 +243,9 @@ static int max9611_read_single(struct max9611_dev *max9611,
>
>  	/*
>  	 * need a delay here to make register configuration
> -	 * stabilize. 1 msec at least, from empirical testing.
> +	 * stabilize.
>  	 */
> -	usleep_range(1000, 2000);
> +	usleep_range(MAX9611_CONV_TIME_US_RANGE);
>
>  	ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
>  	if (ret < 0) {
> @@ -507,7 +512,7 @@ static int max9611_init(struct max9611_dev *max9611)
>  			MAX9611_REG_CTRL2, 0);
>  		return ret;
>  	}
> -	usleep_range(1000, 2000);
> +	usleep_range(MAX9611_CONV_TIME_US_RANGE);
>
>  	return 0;
>  }
> --
> 2.17.1
>

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

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

* Re: [PATCH] iio: adc: max9611: Fix too short conversion time delay
  2019-11-13  9:38 ` Jacopo Mondi
@ 2019-11-13  9:46   ` Geert Uytterhoeven
  2019-11-13  9:58     ` Jacopo Mondi
  0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2019-11-13  9:46 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Geert Uytterhoeven, Jacopo Mondi, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Wolfram Sang, linux-iio, Linux-Renesas,
	Linux Kernel Mailing List

Hi Jacopo,

On Wed, Nov 13, 2019 at 10:36 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> On Wed, Nov 13, 2019 at 10:21:33AM +0100, Geert Uytterhoeven wrote:
> > As of commit b9ddd5091160793e ("iio: adc: max9611: Fix temperature
> > reading in probe"), max9611 initialization sometimes fails on the
> > Salvator-X(S) development board with:
> >
> >     max9611 4-007f: Invalid value received from ADC 0x8000: aborting
> >     max9611: probe of 4-007f failed with error -5
> >
> > The max9611 driver tests communications with the chip by reading the die
> > temperature during the probe function, which returns an invalid value.
> >
> > According to the datasheet, the typical ADC conversion time is 2 ms, but
> > no minimum or maximum values are provided.  However, the driver assumes
> > a 1 ms conversion time.  Usually the usleep_range() call returns after
> > more than 1.8 ms, hence it succeeds.  When it returns earlier, the data
> > register may be read too early, and the previous measurement value will
> > be returned.  After boot, this is the temperature POR (power-on reset)
> > value, causing the failure above.
> >
> > Fix this by increasing the delay from 1000-2000 µs to 2000-2200 µs.
> >
> > Note that this issue has always been present, but it was exposed by the
> > aformentioned commit.
> >
> > Fixes: 69780a3bbc0b1e7e ("iio: adc: Add Maxim max9611 ADC driver")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > This problem was exposed in v5.3.
> >
> > After this patch, probing of the two max9611 sensors succeeded during
> > ca. 3000 boot cycles on Salvator-X(S) boards, equipped with various
> > R-Car H3/M3-W/M3-N SoCs.
> > ---
> >  drivers/iio/adc/max9611.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > index da073d72f649f829..b0755f25356d700d 100644
> > --- a/drivers/iio/adc/max9611.c
> > +++ b/drivers/iio/adc/max9611.c
> > @@ -89,6 +89,11 @@
> >  #define MAX9611_TEMP_SCALE_NUM               1000000
> >  #define MAX9611_TEMP_SCALE_DIV               2083
> >
> > +/*
> > + * Conversion time is 2 ms (typically)
> > + */
> > +#define MAX9611_CONV_TIME_US_RANGE   2000, 2200
> > +
>
> Is a 20% sleep range enough or should it be slightly lengthen ?

10%?

This only impacts the variation, so what really happens depends on the
rate of the hrtimer (if present).
On R-Car Gen3, I think that uses the ARM Architectured Timer (cp15),
which has a period of 120 ns.

> Apart from this, thanks a lot for finding the issue root cause!
>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks!

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] 7+ messages in thread

* Re: [PATCH] iio: adc: max9611: Fix too short conversion time delay
  2019-11-13  9:46   ` Geert Uytterhoeven
@ 2019-11-13  9:58     ` Jacopo Mondi
  0 siblings, 0 replies; 7+ messages in thread
From: Jacopo Mondi @ 2019-11-13  9:58 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Geert Uytterhoeven, Jacopo Mondi, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Wolfram Sang, linux-iio, Linux-Renesas,
	Linux Kernel Mailing List

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

Hi Geert,

On Wed, Nov 13, 2019 at 10:46:21AM +0100, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Wed, Nov 13, 2019 at 10:36 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > On Wed, Nov 13, 2019 at 10:21:33AM +0100, Geert Uytterhoeven wrote:
> > > As of commit b9ddd5091160793e ("iio: adc: max9611: Fix temperature
> > > reading in probe"), max9611 initialization sometimes fails on the
> > > Salvator-X(S) development board with:
> > >
> > >     max9611 4-007f: Invalid value received from ADC 0x8000: aborting
> > >     max9611: probe of 4-007f failed with error -5
> > >
> > > The max9611 driver tests communications with the chip by reading the die
> > > temperature during the probe function, which returns an invalid value.
> > >
> > > According to the datasheet, the typical ADC conversion time is 2 ms, but
> > > no minimum or maximum values are provided.  However, the driver assumes
> > > a 1 ms conversion time.  Usually the usleep_range() call returns after
> > > more than 1.8 ms, hence it succeeds.  When it returns earlier, the data
> > > register may be read too early, and the previous measurement value will
> > > be returned.  After boot, this is the temperature POR (power-on reset)
> > > value, causing the failure above.
> > >
> > > Fix this by increasing the delay from 1000-2000 µs to 2000-2200 µs.
> > >
> > > Note that this issue has always been present, but it was exposed by the
> > > aformentioned commit.
> > >
> > > Fixes: 69780a3bbc0b1e7e ("iio: adc: Add Maxim max9611 ADC driver")
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > > This problem was exposed in v5.3.
> > >
> > > After this patch, probing of the two max9611 sensors succeeded during
> > > ca. 3000 boot cycles on Salvator-X(S) boards, equipped with various
> > > R-Car H3/M3-W/M3-N SoCs.
> > > ---
> > >  drivers/iio/adc/max9611.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > > index da073d72f649f829..b0755f25356d700d 100644
> > > --- a/drivers/iio/adc/max9611.c
> > > +++ b/drivers/iio/adc/max9611.c
> > > @@ -89,6 +89,11 @@
> > >  #define MAX9611_TEMP_SCALE_NUM               1000000
> > >  #define MAX9611_TEMP_SCALE_DIV               2083
> > >
> > > +/*
> > > + * Conversion time is 2 ms (typically)
> > > + */
> > > +#define MAX9611_CONV_TIME_US_RANGE   2000, 2200
> > > +
> >
> > Is a 20% sleep range enough or should it be slightly lengthen ?
>
> 10%?

Ehrm... yes :/

>
> This only impacts the variation, so what really happens depends on the
> rate of the hrtimer (if present).
> On R-Car Gen3, I think that uses the ARM Architectured Timer (cp15),
> which has a period of 120 ns.
>

I'm not questioning the hrtimer rate, I'm questioning what would be an
ideal interval to coalesce this with as much other delays as possible,
but I think we're good and this is really a minor thing mostly for my
personal education, as I've seen mentioned in other reviews a 20%
range is usually suggested (found no mention of that in
timers-howto.rst though)

Thanks again for fixing this
  j

> > Apart from this, thanks a lot for finding the issue root cause!
> >
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Thanks!
>
> 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] 7+ messages in thread

* Re: [PATCH] iio: adc: max9611: Fix too short conversion time delay
  2019-11-13  9:21 [PATCH] iio: adc: max9611: Fix too short conversion time delay Geert Uytterhoeven
  2019-11-13  9:38 ` Jacopo Mondi
@ 2019-11-13 18:00 ` Wolfram Sang
  2019-11-13 18:32   ` Geert Uytterhoeven
  1 sibling, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2019-11-13 18:00 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Jacopo Mondi, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Wolfram Sang,
	linux-iio, linux-renesas-soc, linux-kernel

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


> According to the datasheet, the typical ADC conversion time is 2 ms, but
> no minimum or maximum values are provided.

That sentence makes me want to increase the delay to 4ms.

Maybe we can ask someone at Analog in parallel? I recently found this
patch from an Analog developer in my inbox "[RESEND PATCH] drm: bridge:
adv7511: Fix low refresh rate register for ADV7533/5" Or do you guys
know a better contact?


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

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

* Re: [PATCH] iio: adc: max9611: Fix too short conversion time delay
  2019-11-13 18:00 ` Wolfram Sang
@ 2019-11-13 18:32   ` Geert Uytterhoeven
  2019-11-14  9:08     ` Wolfram Sang
  0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2019-11-13 18:32 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Geert Uytterhoeven, Jacopo Mondi, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Wolfram Sang, linux-iio, Linux-Renesas,
	Linux Kernel Mailing List

Hi Wolfram,

On Wed, Nov 13, 2019 at 7:01 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > According to the datasheet, the typical ADC conversion time is 2 ms, but
> > no minimum or maximum values are provided.
>
> That sentence makes me want to increase the delay to 4ms.

That sounds a bit excessive to me.
According to my measurements, it's 1.8 ms.

=> 3 ms?

> Maybe we can ask someone at Analog in parallel? I recently found this
> patch from an Analog developer in my inbox "[RESEND PATCH] drm: bridge:
> adv7511: Fix low refresh rate register for ADV7533/5" Or do you guys
> know a better contact?

s/Analog/Maxim/?

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] 7+ messages in thread

* Re: [PATCH] iio: adc: max9611: Fix too short conversion time delay
  2019-11-13 18:32   ` Geert Uytterhoeven
@ 2019-11-14  9:08     ` Wolfram Sang
  0 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2019-11-14  9:08 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Geert Uytterhoeven, Jacopo Mondi, Jonathan Cameron,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Wolfram Sang, linux-iio, Linux-Renesas,
	Linux Kernel Mailing List

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

Hi Geert,

> That sounds a bit excessive to me.
> According to my measurements, it's 1.8 ms.
> 
> => 3 ms?

[Quoting "Life of Brian"]

HARRY THE HAGGLER: This bloke won't haggle.
BURT: Won't haggle?!

OK :)

> s/Analog/Maxim/?

Uhh, yes. Wishful thinking...

Kind regards,

   Wolfram


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

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

end of thread, other threads:[~2019-11-14  9:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-13  9:21 [PATCH] iio: adc: max9611: Fix too short conversion time delay Geert Uytterhoeven
2019-11-13  9:38 ` Jacopo Mondi
2019-11-13  9:46   ` Geert Uytterhoeven
2019-11-13  9:58     ` Jacopo Mondi
2019-11-13 18:00 ` Wolfram Sang
2019-11-13 18:32   ` Geert Uytterhoeven
2019-11-14  9:08     ` Wolfram Sang

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).