* [PATCH] hwmon: (sht21) Don't use clock stretching
@ 2018-01-26 23:37 Danilo Bargen
2018-01-27 16:26 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Danilo Bargen @ 2018-01-26 23:37 UTC (permalink / raw
To: linux-hwmon; +Cc: jdelvare, linux, andreas.brauchli, Danilo Bargen
The current driver uses clock stretching (hold master) mode to retrieve
the measurements. This blocks the bus and may cause problems on some
systems like the BCM2835 (Raspberry Pi) that have buggy I2C hardware¹.
¹ http://www.advamation.com/knowhow/raspberrypi/rpi-i2c-bug.html
Signed-off-by: Danilo Bargen <mail@dbrgn.ch>
Reviewed-by: Andreas Brauchli <andreas.brauchli@sensirion.com>
---
drivers/hwmon/sht21.c | 79 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 60 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/sht21.c b/drivers/hwmon/sht21.c
index 984f14b3e89a..56aa13b15682 100644
--- a/drivers/hwmon/sht21.c
+++ b/drivers/hwmon/sht21.c
@@ -25,15 +25,24 @@
#include <linux/mutex.h>
#include <linux/device.h>
#include <linux/jiffies.h>
+#include <linux/delay.h>
-/* I2C command bytes */
-#define SHT21_TRIG_T_MEASUREMENT_HM 0xe3
-#define SHT21_TRIG_RH_MEASUREMENT_HM 0xe5
+/* I2C commands */
+static const char sht21_trig_t_measurement_nh[] = { 0xf3 };
+static const char sht21_trig_rh_measurement_nh[] = { 0xf5 };
#define SHT21_READ_SNB_CMD1 0xFA
#define SHT21_READ_SNB_CMD2 0x0F
#define SHT21_READ_SNAC_CMD1 0xFC
#define SHT21_READ_SNAC_CMD2 0xC9
+/* I2C lengths */
+#define SHT21_CMD_LENGTH 1
+#define SHT21_RESPONSE_LENGTH 3
+
+/* Max waiting times in ms at max resolution, datasheet table 7 */
+#define SHT21_WAIT_T 85
+#define SHT21_WAIT_RH 29
+
/**
* struct sht21 - SHT21 device specific data
* @hwmon_dev: device registered with hwmon
@@ -93,6 +102,7 @@ static inline int sht21_rh_ticks_to_per_cent_mille(int ticks)
static int sht21_update_measurements(struct device *dev)
{
int ret = 0;
+ unsigned char buf[SHT21_RESPONSE_LENGTH];
struct sht21 *sht21 = dev_get_drvdata(dev);
struct i2c_client *client = sht21->client;
@@ -103,22 +113,60 @@ static int sht21_update_measurements(struct device *dev)
* maximum two measurements per second at 12bit accuracy shall be made.
*/
if (time_after(jiffies, sht21->last_update + HZ / 2) || !sht21->valid) {
- ret = i2c_smbus_read_word_swapped(client,
- SHT21_TRIG_T_MEASUREMENT_HM);
- if (ret < 0)
+ /*
+ * Read from the SHT21 in non blocking mode
+ * (no clock stretching).
+ */
+
+ /* Trigger temperature measurement */
+ ret = i2c_master_send(client,
+ sht21_trig_t_measurement_nh,
+ SHT21_CMD_LENGTH);
+ if (ret != SHT21_CMD_LENGTH) {
+ dev_err(dev, "failed to send command: %d\n", ret);
+ ret = -EIO;
+ goto out;
+ }
+ msleep(SHT21_WAIT_T);
+
+ /* Read temperature data */
+ ret = i2c_master_recv(client, buf, sizeof(buf));
+ if (ret != SHT21_RESPONSE_LENGTH) {
+ dev_err(dev, "failed to read values: %d\n", ret);
+ ret = -EIO;
+ goto out;
+ }
+ sht21->temperature = sht21_temp_ticks_to_millicelsius(
+ (buf[0] << 8) | buf[1]
+ );
+
+ /* Trigger humidity measurement */
+ ret = i2c_master_send(client,
+ sht21_trig_rh_measurement_nh,
+ SHT21_CMD_LENGTH);
+ if (ret != SHT21_CMD_LENGTH) {
+ dev_err(dev, "failed to send command: %d\n", ret);
+ ret = -EIO;
goto out;
- sht21->temperature = sht21_temp_ticks_to_millicelsius(ret);
- ret = i2c_smbus_read_word_swapped(client,
- SHT21_TRIG_RH_MEASUREMENT_HM);
- if (ret < 0)
+ }
+ msleep(SHT21_WAIT_RH);
+
+ /* Read humidity data */
+ ret = i2c_master_recv(client, buf, sizeof(buf));
+ if (ret != SHT21_RESPONSE_LENGTH) {
+ dev_err(dev, "failed to read values: %d\n", ret);
+ ret = -EIO;
goto out;
- sht21->humidity = sht21_rh_ticks_to_per_cent_mille(ret);
+ }
+ sht21->humidity = sht21_rh_ticks_to_per_cent_mille(
+ (buf[0] << 8) | buf[1]
+ );
+
sht21->last_update = jiffies;
sht21->valid = 1;
}
out:
mutex_unlock(&sht21->lock);
-
return ret >= 0 ? 0 : ret;
}
@@ -269,13 +317,6 @@ static int sht21_probe(struct i2c_client *client,
struct device *hwmon_dev;
struct sht21 *sht21;
- if (!i2c_check_functionality(client->adapter,
- I2C_FUNC_SMBUS_WORD_DATA)) {
- dev_err(&client->dev,
- "adapter does not support SMBus word transactions\n");
- return -ENODEV;
- }
-
sht21 = devm_kzalloc(dev, sizeof(*sht21), GFP_KERNEL);
if (!sht21)
return -ENOMEM;
--
2.15.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] hwmon: (sht21) Don't use clock stretching
2018-01-26 23:37 [PATCH] hwmon: (sht21) Don't use clock stretching Danilo Bargen
@ 2018-01-27 16:26 ` Guenter Roeck
2018-01-31 14:39 ` Danilo Bargen
0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2018-01-27 16:26 UTC (permalink / raw
To: Danilo Bargen, linux-hwmon; +Cc: jdelvare, andreas.brauchli
On 01/26/2018 03:37 PM, Danilo Bargen wrote:
> The current driver uses clock stretching (hold master) mode to retrieve
> the measurements. This blocks the bus and may cause problems on some
> systems like the BCM2835 (Raspberry Pi) that have buggy I2C hardware¹.
>
> ¹ http://www.advamation.com/knowhow/raspberrypi/rpi-i2c-bug.html
>
> Signed-off-by: Danilo Bargen <mail@dbrgn.ch>
> Reviewed-by: Andreas Brauchli <andreas.brauchli@sensirion.com>
> ---
> drivers/hwmon/sht21.c | 79 ++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 60 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/hwmon/sht21.c b/drivers/hwmon/sht21.c
> index 984f14b3e89a..56aa13b15682 100644
> --- a/drivers/hwmon/sht21.c
> +++ b/drivers/hwmon/sht21.c
> @@ -25,15 +25,24 @@
> #include <linux/mutex.h>
> #include <linux/device.h>
> #include <linux/jiffies.h>
> +#include <linux/delay.h>
>
> -/* I2C command bytes */
> -#define SHT21_TRIG_T_MEASUREMENT_HM 0xe3
> -#define SHT21_TRIG_RH_MEASUREMENT_HM 0xe5
> +/* I2C commands */
> +static const char sht21_trig_t_measurement_nh[] = { 0xf3 };
> +static const char sht21_trig_rh_measurement_nh[] = { 0xf5 };
> #define SHT21_READ_SNB_CMD1 0xFA
> #define SHT21_READ_SNB_CMD2 0x0F
> #define SHT21_READ_SNAC_CMD1 0xFC
> #define SHT21_READ_SNAC_CMD2 0xC9
>
> +/* I2C lengths */
> +#define SHT21_CMD_LENGTH 1
> +#define SHT21_RESPONSE_LENGTH 3
> +
> +/* Max waiting times in ms at max resolution, datasheet table 7 */
> +#define SHT21_WAIT_T 85
> +#define SHT21_WAIT_RH 29
> +
> /**
> * struct sht21 - SHT21 device specific data
> * @hwmon_dev: device registered with hwmon
> @@ -93,6 +102,7 @@ static inline int sht21_rh_ticks_to_per_cent_mille(int ticks)
> static int sht21_update_measurements(struct device *dev)
> {
> int ret = 0;
> + unsigned char buf[SHT21_RESPONSE_LENGTH];
> struct sht21 *sht21 = dev_get_drvdata(dev);
> struct i2c_client *client = sht21->client;
>
> @@ -103,22 +113,60 @@ static int sht21_update_measurements(struct device *dev)
> * maximum two measurements per second at 12bit accuracy shall be made.
> */
> if (time_after(jiffies, sht21->last_update + HZ / 2) || !sht21->valid) {
> - ret = i2c_smbus_read_word_swapped(client,
> - SHT21_TRIG_T_MEASUREMENT_HM);
> - if (ret < 0)
> + /*
> + * Read from the SHT21 in non blocking mode
> + * (no clock stretching).
> + */
> +
> + /* Trigger temperature measurement */
> + ret = i2c_master_send(client,
> + sht21_trig_t_measurement_nh,
> + SHT21_CMD_LENGTH);
> + if (ret != SHT21_CMD_LENGTH) {
> + dev_err(dev, "failed to send command: %d\n", ret);
> + ret = -EIO;
Both i2c_master_send() and i2c_master_recv() return either the expected length
or an error. Overriding the error is neither necessary nor desirable.
> + goto out;
> + }
> + msleep(SHT21_WAIT_T);
> +
> + /* Read temperature data */
> + ret = i2c_master_recv(client, buf, sizeof(buf));
> + if (ret != SHT21_RESPONSE_LENGTH) {
> + dev_err(dev, "failed to read values: %d\n", ret);
> + ret = -EIO;
> + goto out;
> + }
> + sht21->temperature = sht21_temp_ticks_to_millicelsius(
> + (buf[0] << 8) | buf[1]
> + );
Please find a better alignment than that.
> +
> + /* Trigger humidity measurement */
> + ret = i2c_master_send(client,
> + sht21_trig_rh_measurement_nh,
> + SHT21_CMD_LENGTH);
> + if (ret != SHT21_CMD_LENGTH) {
> + dev_err(dev, "failed to send command: %d\n", ret);
> + ret = -EIO;
> goto out;
> - sht21->temperature = sht21_temp_ticks_to_millicelsius(ret);
> - ret = i2c_smbus_read_word_swapped(client,
> - SHT21_TRIG_RH_MEASUREMENT_HM);
> - if (ret < 0)
> + }
> + msleep(SHT21_WAIT_RH);
> +
> + /* Read humidity data */
> + ret = i2c_master_recv(client, buf, sizeof(buf));
> + if (ret != SHT21_RESPONSE_LENGTH) {
> + dev_err(dev, "failed to read values: %d\n", ret);
> + ret = -EIO;
> goto out;
> - sht21->humidity = sht21_rh_ticks_to_per_cent_mille(ret);
> + }
> + sht21->humidity = sht21_rh_ticks_to_per_cent_mille(
> + (buf[0] << 8) | buf[1]
> + );
> +
> sht21->last_update = jiffies;
> sht21->valid = 1;
> }
> out:
> mutex_unlock(&sht21->lock);
> -
> return ret >= 0 ? 0 : ret;
> }
>
> @@ -269,13 +317,6 @@ static int sht21_probe(struct i2c_client *client,
> struct device *hwmon_dev;
> struct sht21 *sht21;
>
> - if (!i2c_check_functionality(client->adapter,
> - I2C_FUNC_SMBUS_WORD_DATA)) {
> - dev_err(&client->dev,
> - "adapter does not support SMBus word transactions\n");
> - return -ENODEV;
> - }
> -
The driver must now support I2C_FUNC_I2C which is a significant change in functionality
since it will no longer work on SMBus-only adapters. On top of that, it severely affects
access speed (or at least that is most likely the case). You'll have to find a better
solution, one that retains the old functionality but also works on the Raspberry Pi.
FWIW, this should really be solved in the infrastructure. Many devices support and
depend on clock stretching, and we can not mess up the drivers for all those devices
to deal with a broken adapter. Has this been discussed on the i2c mailing list ?
Guenter
> sht21 = devm_kzalloc(dev, sizeof(*sht21), GFP_KERNEL);
> if (!sht21)
> return -ENOMEM;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hwmon: (sht21) Don't use clock stretching
2018-01-27 16:26 ` Guenter Roeck
@ 2018-01-31 14:39 ` Danilo Bargen
2018-01-31 15:57 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Danilo Bargen @ 2018-01-31 14:39 UTC (permalink / raw
To: Guenter Roeck; +Cc: linux-hwmon, jdelvare, andreas.brauchli
Hello Guenter
Thank you for your fast review!
> The driver must now support I2C_FUNC_I2C which is a significant change in functionality
> since it will no longer work on SMBus-only adapters.
You are right of course, I wasn't aware of that.
> You'll have to find a better
> solution, one that retains the old functionality but also works on the Raspberry Pi.
What are my options? A separate driver (sht21nhm) would probably not be accepted, right?
Or should I instead try to submit it to the Raspberry Pi kernel tree directly?
> FWIW, this should really be solved in the infrastructure. Many devices support and
> depend on clock stretching, and we can not mess up the drivers for all those devices
> to deal with a broken adapter.
Could you elaborate what you mean with "in the infrastructure"? Could there be a
way to solve this on the i2c / adapter driver level?
An interesting sidenote is that the driver used to work on the Raspberry Pi with
older kernel versions, but not on current ones. (I'd have to look up the exact
versions, but I think it changed sometime last year.) Maybe something about the
timing was changed that broke clock stretching.
> Has this been discussed on the i2c mailing list ?
I did not submit a discussion so far, and the mailing list search did not
turn up anything related. Should I start a discussion there?
Cheers and thanks for your time,
Danilo Bargen
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hwmon: (sht21) Don't use clock stretching
2018-01-31 14:39 ` Danilo Bargen
@ 2018-01-31 15:57 ` Guenter Roeck
0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2018-01-31 15:57 UTC (permalink / raw
To: Danilo Bargen; +Cc: linux-hwmon, jdelvare, andreas.brauchli
On Wed, Jan 31, 2018 at 03:39:23PM +0100, Danilo Bargen wrote:
> Hello Guenter
>
> Thank you for your fast review!
>
> > The driver must now support I2C_FUNC_I2C which is a significant change in functionality
> > since it will no longer work on SMBus-only adapters.
>
> You are right of course, I wasn't aware of that.
>
> > You'll have to find a better
> > solution, one that retains the old functionality but also works on the Raspberry Pi.
>
> What are my options? A separate driver (sht21nhm) would probably not be accepted, right?
>
One option would be to implement access functions differently if clock
stretching is known to not work. That would, however, require infrastructure
support since the controller driver should tell the chip driver that it doesnot
support clock stretching.
> Or should I instead try to submit it to the Raspberry Pi kernel tree directly?
>
> > FWIW, this should really be solved in the infrastructure. Many devices support and
> > depend on clock stretching, and we can not mess up the drivers for all those devices
> > to deal with a broken adapter.
>
> Could you elaborate what you mean with "in the infrastructure"? Could there be a
> way to solve this on the i2c / adapter driver level?
>
As mentioned above, this is really a i2c controller driver issue. If the
controller driver does not support clock stretching, it should tell chip drivers
about it. There should also be infrastructure support to support "simulated"
clock stretching; otherwise each and every chip driver who needs it would have
to re-implement the same set of hacks.
> An interesting sidenote is that the driver used to work on the Raspberry Pi with
> older kernel versions, but not on current ones. (I'd have to look up the exact
> versions, but I think it changed sometime last year.) Maybe something about the
> timing was changed that broke clock stretching.
>
Wouldn't it make sense then to track down the patch which introduced the problem
and fix the underlying issue instead of working around it ?
Guenter
> > Has this been discussed on the i2c mailing list ?
>
> I did not submit a discussion so far, and the mailing list search did not
> turn up anything related. Should I start a discussion there?
>
> Cheers and thanks for your time,
> Danilo Bargen
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-01-31 15:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-26 23:37 [PATCH] hwmon: (sht21) Don't use clock stretching Danilo Bargen
2018-01-27 16:26 ` Guenter Roeck
2018-01-31 14:39 ` Danilo Bargen
2018-01-31 15:57 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.