[PATCH v2] hwmon: sht3x: wait predefined limits loading complete before access

[PATCH v2] hwmon: sht3x: wait predefined limits loading complete before access

[Emiliano Ingrassia]

An sht3x sensor include limits register which contains temperature
and humidity limit values. After a reset, pre-defined values are loaded
into that register. During the probe function, the driver reads the
limits register. However, if the reads are made too early, for example
because the I2C bus frequency is high (e.g. 400 kHz), the loading could be
not completed and the sensor returns a NACK which causes the probe to fail.
A delay of at least 500 us before the first read solves this issue.

Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
---
 drivers/hwmon/sht3x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
index 6ea99cd6ae79..f2098443df2f 100644
--- a/drivers/hwmon/sht3x.c
+++ b/drivers/hwmon/sht3x.c
@@ -732,6 +732,9 @@ static int sht3x_probe(struct i2c_client *client,
 	mutex_init(&data->i2c_lock);
 	mutex_init(&data->data_lock);
 
+	/* wait predefined limits loading complete before access */
+	usleep_range(500, 600);
+
 	ret = limits_update(data);
 	if (ret)
 		return ret;
-- 
2.15.1

Re: [PATCH v2] hwmon: sht3x: wait predefined limits loading complete before access

[Guenter Roeck]

On Fri, Jan 12, 2018 at 08:06:00PM +0100, Emiliano Ingrassia wrote:
> An sht3x sensor include limits register which contains temperature
> and humidity limit values. After a reset, pre-defined values are loaded
> into that register. During the probe function, the driver reads the
> limits register. However, if the reads are made too early, for example
> because the I2C bus frequency is high (e.g. 400 kHz), the loading could be
> not completed and the sensor returns a NACK which causes the probe to fail.
> A delay of at least 500 us before the first read solves this issue.
> 
> Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
> ---
>  drivers/hwmon/sht3x.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
> index 6ea99cd6ae79..f2098443df2f 100644
> --- a/drivers/hwmon/sht3x.c
> +++ b/drivers/hwmon/sht3x.c
> @@ -732,6 +732,9 @@ static int sht3x_probe(struct i2c_client *client,
>  	mutex_init(&data->i2c_lock);
>  	mutex_init(&data->data_lock);
>  
> +	/* wait predefined limits loading complete before access */

The commit log explains the problem much better. This here is cryptic.
Would you understand this if you would read it in an arbitrary driver ?
Please consider something like:

	/*
	 * If an attempt to read the limits is made too early after sending
	 * the SHT3X_CMD_LENGTH command, the chip may respond with NACK.
	 * This was observed on BeagleBone Black on an i2c bus clocked
	 * at 400 kHz. The delay was chosen empirically.
	 */

Answering your question, no, I don't have any personal experience
with this chip. My comments are generic.

Guenter

> +	usleep_range(500, 600);
> +
>  	ret = limits_update(data);
>  	if (ret)
>  		return ret;
> -- 
> 2.15.1
> 

Re: [PATCH v2] hwmon: sht3x: wait predefined limits loading complete before access

[Emiliano Ingrassia]

On Fri, Jan 12, 2018 at 11:44:30AM -0800, Guenter Roeck wrote:
> On Fri, Jan 12, 2018 at 08:06:00PM +0100, Emiliano Ingrassia wrote:
> > An sht3x sensor include limits register which contains temperature
> > and humidity limit values. After a reset, pre-defined values are loaded
> > into that register. During the probe function, the driver reads the
> > limits register. However, if the reads are made too early, for example
> > because the I2C bus frequency is high (e.g. 400 kHz), the loading could be
> > not completed and the sensor returns a NACK which causes the probe to fail.
> > A delay of at least 500 us before the first read solves this issue.
> > 
> > Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com>
> > ---
> >  drivers/hwmon/sht3x.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
> > index 6ea99cd6ae79..f2098443df2f 100644
> > --- a/drivers/hwmon/sht3x.c
> > +++ b/drivers/hwmon/sht3x.c
> > @@ -732,6 +732,9 @@ static int sht3x_probe(struct i2c_client *client,
> >  	mutex_init(&data->i2c_lock);
> >  	mutex_init(&data->data_lock);
> >  
> > +	/* wait predefined limits loading complete before access */
> 
> The commit log explains the problem much better. This here is cryptic.
> Would you understand this if you would read it in an arbitrary driver ?
> Please consider something like:
> 
> 	/*
> 	 * If an attempt to read the limits is made too early after sending
> 	 * the SHT3X_CMD_LENGTH command, the chip may respond with NACK.
> 	 * This was observed on BeagleBone Black on an i2c bus clocked
> 	 * at 400 kHz. The delay was chosen empirically.
> 	 */
>

Ok, I didn't want to be to verbose. I'll resubmit with a more clear
comment.

Thanks,

Emiliano

> Answering your question, no, I don't have any personal experience
> with this chip. My comments are generic.
> 
> Guenter
> 
> > +	usleep_range(500, 600);
> > +
> >  	ret = limits_update(data);
> >  	if (ret)
> >  		return ret;
> > -- 
> > 2.15.1
> >