Re: [PATCH] hwmon: (sht21) Don't use clock stretching

2018-01-31 Thread Guenter Roeck
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
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] hwmon: (sht21) Don't use clock stretching

2018-01-31 Thread Danilo Bargen
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
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] hwmon: (sht21) Don't use clock stretching

2018-01-27 Thread Guenter Roeck

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 
Reviewed-by: Andreas Brauchli 
---
  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 
  #include 
  #include 
+#include 
  
-/* 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(>lock);
-
return ret >= 0 ? 0 : ret;
  }
  
@@ -269,13 +317,6 @@ static int sht21_probe(struct i2c_client 

[PATCH] hwmon: (sht21) Don't use clock stretching

2018-01-26 Thread 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 
Reviewed-by: Andreas Brauchli 
---
 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 
 #include 
 #include 
+#include 
 
-/* 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(>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(>dev,
-   "adapter does not