Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] iio: adc: max9611: Fix too short conversion time delay
@ 2019-12-02  8:55 Geert Uytterhoeven
  2019-12-02  9:23 ` Wolfram Sang
  0 siblings, 1 reply; 3+ messages in thread
From: Geert Uytterhoeven @ 2019-12-02  8:55 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler
  Cc: Jacopo Mondi, 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.  Maxim Technical Support
confirmed this was tested with temperature Ta=25 degreeC, and promised
to inform me if a maximum/minimum value is available (they didn't get
back to me, so I assume it is not).

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 3000-3300 µ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>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
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.

v2:
  - Add Reviewed-by,
  - Add feedback from Maxim Technical Support,
  - Increase delay from 2000-2200 µs to 3000-3300 µs to play safe.
---
 drivers/iio/adc/max9611.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
index da073d72f649f829..135793db4fad6b2c 100644
--- a/drivers/iio/adc/max9611.c
+++ b/drivers/iio/adc/max9611.c
@@ -89,6 +89,12 @@
 #define MAX9611_TEMP_SCALE_NUM		1000000
 #define MAX9611_TEMP_SCALE_DIV		2083
 
+/*
+ * Conversion time is 2 ms (typically) at Ta=25 degreeC
+ * No maximum value is known, so play it safe.
+ */
+#define MAX9611_CONV_TIME_US_RANGE	3000, 3300
+
 struct max9611_dev {
 	struct device *dev;
 	struct i2c_client *i2c_client;
@@ -238,9 +244,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 +513,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	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] iio: adc: max9611: Fix too short conversion time delay
  2019-12-02  8:55 [PATCH v2] iio: adc: max9611: Fix too short conversion time delay Geert Uytterhoeven
@ 2019-12-02  9:23 ` Wolfram Sang
  2019-12-07 10:33   ` Jonathan Cameron
  0 siblings, 1 reply; 3+ messages in thread
From: Wolfram Sang @ 2019-12-02  9:23 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jacopo Mondi, Wolfram Sang, linux-iio,
	linux-renesas-soc, linux-kernel

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

On Mon, Dec 02, 2019 at 09:55:46AM +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.  Maxim Technical Support
> confirmed this was tested with temperature Ta=25 degreeC, and promised
> to inform me if a maximum/minimum value is available (they didn't get
> back to me, so I assume it is not).
> 
> 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 3000-3300 µ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>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

One minor nit, though:

>  	/*
>  	 * need a delay here to make register configuration
> -	 * stabilize. 1 msec at least, from empirical testing.
> +	 * stabilize.

This could be a one line comment now?


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

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

* Re: [PATCH v2] iio: adc: max9611: Fix too short conversion time delay
  2019-12-02  9:23 ` Wolfram Sang
@ 2019-12-07 10:33   ` Jonathan Cameron
  0 siblings, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2019-12-07 10:33 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Geert Uytterhoeven, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Jacopo Mondi, Wolfram Sang, linux-iio,
	linux-renesas-soc, linux-kernel

On Mon, 2 Dec 2019 10:23:29 +0100
Wolfram Sang <wsa@the-dreams.de> wrote:

> On Mon, Dec 02, 2019 at 09:55:46AM +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.  Maxim Technical Support
> > confirmed this was tested with temperature Ta=25 degreeC, and promised
> > to inform me if a maximum/minimum value is available (they didn't get
> > back to me, so I assume it is not).
> > 
> > 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 3000-3300 µ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>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>  
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> One minor nit, though:
> 
> >  	/*
> >  	 * need a delay here to make register configuration
> > -	 * stabilize. 1 msec at least, from empirical testing.
> > +	 * stabilize.  
> 
> This could be a one line comment now?
> 

Applied to the fixes-togreg branch of iio.git and tidied up.

Thanks,

Jonathan

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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02  8:55 [PATCH v2] iio: adc: max9611: Fix too short conversion time delay Geert Uytterhoeven
2019-12-02  9:23 ` Wolfram Sang
2019-12-07 10:33   ` Jonathan Cameron

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git