[PATCH 1/4] i2c: added i2c quirk flag for unsupported clock stretching

[PATCH 1/4] i2c: added i2c quirk flag for unsupported clock stretching

[Nicola Corna]

Added I2C_AQ_NO_CLK_STRETCH, to be used when clock stretching is not
supported.

Signed-off-by: Nicola Corna <nicola-V0pQiMyom5mh6J55Ss3d3w@public.gmane.org>
---
 include/linux/i2c.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 768063b..9697002 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -493,6 +493,8 @@ struct i2c_adapter_quirks {
 /* convenience macro for typical write-then read case */
 #define I2C_AQ_COMB_WRITE_THEN_READ	(I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | \
 					 I2C_AQ_COMB_READ_SECOND | I2C_AQ_COMB_SAME_ADDR)
+/* clock stretching is not supported */
+#define I2C_AQ_NO_CLK_STRETCH		BIT(4)
 
 /*
  * i2c_adapter is the structure used to identify a physical i2c bus along
-- 
2.6.1

[PATCH 2/4] i2c: added I2C_AQ_NO_CLK_STRETCH to i2c-algo-bit.c

[Nicola Corna]

Added I2C_AQ_NO_CLK_STRETCH to drivers/i2c/algos/i2c-algo-bit.c when
getscl is not available.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
 drivers/i2c/algos/i2c-algo-bit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index 899bede..9d233bb 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -617,6 +617,10 @@ const struct i2c_algorithm i2c_bit_algo = {
 };
 EXPORT_SYMBOL(i2c_bit_algo);
 
+const struct i2c_adapter_quirks i2c_bit_quirk_no_clk_stretch = {
+	.flags = I2C_AQ_NO_CLK_STRETCH,
+};
+
 /*
  * registering functions to load algorithms at runtime
  */
@@ -635,6 +639,8 @@ static int __i2c_bit_add_bus(struct i2c_adapter *adap,
 	/* register new adapter to i2c module... */
 	adap->algo = &i2c_bit_algo;
 	adap->retries = 3;
+	if (bit_adap->getscl == NULL)
+		adap->quirks = &i2c_bit_quirk_no_clk_stretch;
 
 	ret = add_adapter(adap);
 	if (ret < 0)
-- 
2.6.1

[PATCH 3/4] i2c: added I2C_AQ_NO_CLK_STRETCH to i2c-bcm2835.c

[Nicola Corna]

As reported here
http://www.advamation.com/knowhow/raspberrypi/rpi-i2c-bug.html
the BCM2835 has a bug in its i2c implementation which prevents a correct
clock stretching.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
 drivers/i2c/busses/i2c-bcm2835.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 3032b89..38d7102 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -222,6 +222,10 @@ static const struct i2c_algorithm bcm2835_i2c_algo = {
 	.functionality	= bcm2835_i2c_func,
 };
 
+static const struct i2c_adapter_quirks bcm2835_i2c_quirks = {
+	.flags = I2C_AQ_NO_CLK_STRETCH,
+};
+
 static int bcm2835_i2c_probe(struct platform_device *pdev)
 {
 	struct bcm2835_i2c_dev *i2c_dev;
@@ -293,6 +297,7 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 	adap->algo = &bcm2835_i2c_algo;
 	adap->dev.parent = &pdev->dev;
 	adap->dev.of_node = pdev->dev.of_node;
+	adap->quirks = &bcm2835_i2c_quirks;
 
 	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, 0);
 
-- 
2.6.1

Re: [PATCH 3/4] i2c: added I2C_AQ_NO_CLK_STRETCH to i2c-bcm2835.c

[Eric Anholt]

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

Nicola Corna <nicola-V0pQiMyom5mh6J55Ss3d3w@public.gmane.org> writes:
> As reported here
> http://www.advamation.com/knowhow/raspberrypi/rpi-i2c-bug.html
> the BCM2835 has a bug in its i2c implementation which prevents a correct
> clock stretching.
>
> Signed-off-by: Nicola Corna <nicola-V0pQiMyom5mh6J55Ss3d3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-bcm2835.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> index 3032b89..38d7102 100644
> --- a/drivers/i2c/busses/i2c-bcm2835.c
> +++ b/drivers/i2c/busses/i2c-bcm2835.c
> @@ -222,6 +222,10 @@ static const struct i2c_algorithm bcm2835_i2c_algo = {
>  	.functionality	= bcm2835_i2c_func,
>  };
>  
> +static const struct i2c_adapter_quirks bcm2835_i2c_quirks = {
> +	.flags = I2C_AQ_NO_CLK_STRETCH,
> +};
> +
>  static int bcm2835_i2c_probe(struct platform_device *pdev)
>  {
>  	struct bcm2835_i2c_dev *i2c_dev;
> @@ -293,6 +297,7 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
>  	adap->algo = &bcm2835_i2c_algo;
>  	adap->dev.parent = &pdev->dev;
>  	adap->dev.of_node = pdev->dev.of_node;
> +	adap->quirks = &bcm2835_i2c_quirks;
>  
>  	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, 0);

Looking at the driver, we're not setting the REDL/FEDL fields, which are
involved in the delays following clock stretching.  Looks like I2C_DEL's
low 16 bits (rising edge delay) should be I2C_DIV / 4 (wait half the
period for the slave to update its data before we sample it), and the
high 16 bits (falling edge delay) should be I2C_DIV / 16 (put our data
out before we expet the slave to sample the value).

Note: this is not a detailed analysis, nor am I asserting that this is
the bug, just something that we seem to be missing in the Linux driver.

(Added cc to linux-rpi-kernel, in case anyone on that list is interested
in I2C issues on the platform)

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

Re: [PATCH 3/4] i2c: added I2C_AQ_NO_CLK_STRETCH to i2c-bcm2835.c

[Stephen Warren]

On 10/27/2015 02:11 PM, Nicola Corna wrote:
> As reported here
> http://www.advamation.com/knowhow/raspberrypi/rpi-i2c-bug.html
> the BCM2835 has a bug in its i2c implementation which prevents a correct
> clock stretching.

I was going to ask for some more official confirmation of this bug (e.g.
update to BCM2835_Peripherals.pdf), but it looks like it's already been
confirmed; check Gert van Loo's responses at:

https://www.raspberrypi.org/forums/viewtopic.php?p=146272

It might be useful to add this link into the patch description.

[PATCH v3 4/4] iio: humidity: si7020: added No Hold read mode

[Nicola Corna]

The Si7013/20/21 modules support 2 read modes:
 * Hold mode (blocking), where the device stretches the clock until the end
of the measurement
 * No Hold mode (non-blocking), where the device replies NACK for every I2C
call during the measurement
Here the No Hold mode is implemented, selectable with the blocking_io
variable within si7020_platform_data. The default mode is Hold, unless the
adapter does not support clock stretching, in which case the No Hold mode
is used.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
This patch depends on patch "[PATCH v4 1/2] iio: humidity: si7020: replaced
bitmask on humidity values with range check"
 drivers/iio/humidity/si7020.c        | 78 ++++++++++++++++++++++++++++++++----
 include/linux/platform_data/si7020.h | 21 ++++++++++
 2 files changed, 92 insertions(+), 7 deletions(-)
 create mode 100644 include/linux/platform_data/si7020.h

diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
index 12128d1..cb65f34 100644
--- a/drivers/iio/humidity/si7020.c
+++ b/drivers/iio/humidity/si7020.c
@@ -2,6 +2,7 @@
  * si7020.c - Silicon Labs Si7013/20/21 Relative Humidity and Temp Sensors
  * Copyright (c) 2013,2014  Uplogix, Inc.
  * David Barksdale <dbarksdale@uplogix.com>
+ * Copyright (c) 2015 Nicola Corna <nicola@corna.info>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -30,33 +31,79 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
+#include <linux/jiffies.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/platform_data/si7020.h>
 
 /* Measure Relative Humidity, Hold Master Mode */
 #define SI7020CMD_RH_HOLD	0xE5
+/* Measure Relative Humidity, No Hold Master Mode */
+#define SI7020CMD_RH_NO_HOLD	0xF5
 /* Measure Temperature, Hold Master Mode */
 #define SI7020CMD_TEMP_HOLD	0xE3
+/* Measure Temperature, No Hold Master Mode */
+#define SI7020CMD_TEMP_NO_HOLD	0xF3
 /* Software Reset */
 #define SI7020CMD_RESET		0xFE
+/* Relative humidity measurement timeout (us) */
+#define SI7020_RH_TIMEOUT	22800
+/* Temperature measurement timeout (us) */
+#define SI7020_TEMP_TIMEOUT	10800
+/* Minimum delay between retries (No Hold Mode) in us */
+#define SI7020_NOHOLD_SLEEP_MIN	2000
+/* Maximum delay between retries (No Hold Mode) in us */
+#define SI7020_NOHOLD_SLEEP_MAX	6000
 
 static int si7020_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
 {
 	struct i2c_client **client = iio_priv(indio_dev);
+	struct si7020_platform_data *pdata;
 	int ret;
+	bool holdmode;
+	unsigned char buf[2];
+	unsigned long start;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_data(*client,
-					       chan->type == IIO_TEMP ?
-					       SI7020CMD_TEMP_HOLD :
-					       SI7020CMD_RH_HOLD);
-		if (ret < 0)
-			return ret;
-		*val = ret >> 2;
+		pdata = dev_get_platdata(&(*client)->dev);
+		if (pdata)
+			holdmode = pdata->blocking_io;
+		else
+			holdmode = !((*client)->adapter->quirks &&
+					(*client)->adapter->quirks->flags |
+					I2C_AQ_NO_CLK_STRETCH);
+		if (holdmode) {
+			ret = i2c_smbus_read_word_data(*client,
+						       chan->type == IIO_TEMP ?
+						       SI7020CMD_TEMP_HOLD :
+						       SI7020CMD_RH_HOLD);
+			if (ret < 0)
+				return ret;
+			*val = ret >> 2;
+		} else {
+			ret = i2c_smbus_write_byte(*client,
+						   chan->type == IIO_TEMP ?
+						   SI7020CMD_TEMP_NO_HOLD :
+						   SI7020CMD_RH_NO_HOLD);
+			if (ret < 0)
+				return ret;
+			start = jiffies;
+			while ((ret = i2c_master_recv(*client, buf, 2)) < 0) {
+				if (time_after(jiffies, start +
+					       usecs_to_jiffies(
+							chan->type == IIO_TEMP ?
+							SI7020_TEMP_TIMEOUT :
+							SI7020_RH_TIMEOUT)))
+					return ret;
+				usleep_range(SI7020_NOHOLD_SLEEP_MIN,
+					     SI7020_NOHOLD_SLEEP_MAX);
+			}
+			*val = ((buf[0] << 8) | buf[1]) >> 2;
+		}
 		/*
 		 * Humidity values can slightly exceed the 0-100%RH
 		 * range and should be corrected by software
@@ -116,6 +163,7 @@ static int si7020_probe(struct i2c_client *client,
 {
 	struct iio_dev *indio_dev;
 	struct i2c_client **data;
+	struct si7020_platform_data *pdata;
 	int ret;
 
 	if (!i2c_check_functionality(client->adapter,
@@ -123,6 +171,22 @@ static int si7020_probe(struct i2c_client *client,
 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
 		return -ENODEV;
 
+	pdata = dev_get_platdata(&client->dev);
+	if (pdata) {
+		if (pdata->blocking_io) {
+			if (client->adapter->quirks &&
+			   client->adapter->quirks->flags |
+			   I2C_AQ_NO_CLK_STRETCH)
+				return -ENODEV;
+		} else if (!i2c_check_functionality(client->adapter,
+			  I2C_FUNC_I2C))
+			return -ENODEV;
+	} else
+		if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
+		   client->adapter->quirks &&
+		   client->adapter->quirks->flags | I2C_AQ_NO_CLK_STRETCH)
+			return -ENODEV;
+
 	/* Reset device, loads default settings. */
 	ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
 	if (ret < 0)
diff --git a/include/linux/platform_data/si7020.h b/include/linux/platform_data/si7020.h
new file mode 100644
index 0000000..8bb5848
--- /dev/null
+++ b/include/linux/platform_data/si7020.h
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2015 Nicola Corna <nicola@corna.info>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __SI7020_H_
+#define __SI7020_H_
+
+struct si7020_platform_data {
+	bool blocking_io;
+};
+#endif /* __SI7020_H_ */
-- 
2.6.1

Re: [PATCH v3 4/4] iio: humidity: si7020: added No Hold read mode

[Wolfram Sang]

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


Not a complete review, I just stumbled over this one:

> +			if (client->adapter->quirks &&
> +			   client->adapter->quirks->flags |

'|' should be '&', or?

> +			   I2C_AQ_NO_CLK_STRETCH)
> +				return -ENODEV;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 4/4] iio: humidity: si7020: added No Hold read mode

[Nicola Corna]

October 28 2015 12:30 AM, "Wolfram Sang" <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> Not a complete review, I just stumbled over this one:
> 
>> + if (client->adapter->quirks &&
>> + client->adapter->quirks->flags |
> 
> '|' should be '&', or?
> 
Yes, sorry. I'll update the patch.

Nicola Corna
>> + I2C_AQ_NO_CLK_STRETCH)
>> + return -ENODEV;

[PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode

[Nicola Corna]

The Si7013/20/21 modules support 2 read modes:
 * Hold mode (blocking), where the device stretches the clock until the end
of the measurement
 * No Hold mode (non-blocking), where the device replies NACK for every I2C
call during the measurement
Here the No Hold mode is implemented, selectable with the blocking_io
variable within si7020_platform_data. The default mode is Hold, unless the
adapter does not support clock stretching, in which case the No Hold mode
is used.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
This patch depends on patch "[PATCH v4 1/2] iio: humidity: si7020: replaced
bitmask on humidity values with range check"
 drivers/iio/humidity/si7020.c        | 78 ++++++++++++++++++++++++++++++++----
 include/linux/platform_data/si7020.h | 21 ++++++++++
 2 files changed, 92 insertions(+), 7 deletions(-)
 create mode 100644 include/linux/platform_data/si7020.h

diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
index 12128d1..c728681 100644
--- a/drivers/iio/humidity/si7020.c
+++ b/drivers/iio/humidity/si7020.c
@@ -2,6 +2,7 @@
  * si7020.c - Silicon Labs Si7013/20/21 Relative Humidity and Temp Sensors
  * Copyright (c) 2013,2014  Uplogix, Inc.
  * David Barksdale <dbarksdale@uplogix.com>
+ * Copyright (c) 2015 Nicola Corna <nicola@corna.info>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -30,33 +31,79 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
+#include <linux/jiffies.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/platform_data/si7020.h>
 
 /* Measure Relative Humidity, Hold Master Mode */
 #define SI7020CMD_RH_HOLD	0xE5
+/* Measure Relative Humidity, No Hold Master Mode */
+#define SI7020CMD_RH_NO_HOLD	0xF5
 /* Measure Temperature, Hold Master Mode */
 #define SI7020CMD_TEMP_HOLD	0xE3
+/* Measure Temperature, No Hold Master Mode */
+#define SI7020CMD_TEMP_NO_HOLD	0xF3
 /* Software Reset */
 #define SI7020CMD_RESET		0xFE
+/* Relative humidity measurement timeout (us) */
+#define SI7020_RH_TIMEOUT	22800
+/* Temperature measurement timeout (us) */
+#define SI7020_TEMP_TIMEOUT	10800
+/* Minimum delay between retries (No Hold Mode) in us */
+#define SI7020_NOHOLD_SLEEP_MIN	2000
+/* Maximum delay between retries (No Hold Mode) in us */
+#define SI7020_NOHOLD_SLEEP_MAX	6000
 
 static int si7020_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
 {
 	struct i2c_client **client = iio_priv(indio_dev);
+	struct si7020_platform_data *pdata;
 	int ret;
+	bool holdmode;
+	unsigned char buf[2];
+	unsigned long start;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_data(*client,
-					       chan->type == IIO_TEMP ?
-					       SI7020CMD_TEMP_HOLD :
-					       SI7020CMD_RH_HOLD);
-		if (ret < 0)
-			return ret;
-		*val = ret >> 2;
+		pdata = dev_get_platdata(&(*client)->dev);
+		if (pdata)
+			holdmode = pdata->blocking_io;
+		else
+			holdmode = !((*client)->adapter->quirks &&
+					(*client)->adapter->quirks->flags &
+					I2C_AQ_NO_CLK_STRETCH);
+		if (holdmode) {
+			ret = i2c_smbus_read_word_data(*client,
+						       chan->type == IIO_TEMP ?
+						       SI7020CMD_TEMP_HOLD :
+						       SI7020CMD_RH_HOLD);
+			if (ret < 0)
+				return ret;
+			*val = ret >> 2;
+		} else {
+			ret = i2c_smbus_write_byte(*client,
+						   chan->type == IIO_TEMP ?
+						   SI7020CMD_TEMP_NO_HOLD :
+						   SI7020CMD_RH_NO_HOLD);
+			if (ret < 0)
+				return ret;
+			start = jiffies;
+			while ((ret = i2c_master_recv(*client, buf, 2)) < 0) {
+				if (time_after(jiffies, start +
+					       usecs_to_jiffies(
+							chan->type == IIO_TEMP ?
+							SI7020_TEMP_TIMEOUT :
+							SI7020_RH_TIMEOUT)))
+					return ret;
+				usleep_range(SI7020_NOHOLD_SLEEP_MIN,
+					     SI7020_NOHOLD_SLEEP_MAX);
+			}
+			*val = ((buf[0] << 8) | buf[1]) >> 2;
+		}
 		/*
 		 * Humidity values can slightly exceed the 0-100%RH
 		 * range and should be corrected by software
@@ -116,6 +163,7 @@ static int si7020_probe(struct i2c_client *client,
 {
 	struct iio_dev *indio_dev;
 	struct i2c_client **data;
+	struct si7020_platform_data *pdata;
 	int ret;
 
 	if (!i2c_check_functionality(client->adapter,
@@ -123,6 +171,22 @@ static int si7020_probe(struct i2c_client *client,
 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
 		return -ENODEV;
 
+	pdata = dev_get_platdata(&client->dev);
+	if (pdata) {
+		if (pdata->blocking_io) {
+			if (client->adapter->quirks &&
+			   client->adapter->quirks->flags &
+			   I2C_AQ_NO_CLK_STRETCH)
+				return -ENODEV;
+		} else if (!i2c_check_functionality(client->adapter,
+			  I2C_FUNC_I2C))
+			return -ENODEV;
+	} else
+		if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
+		   client->adapter->quirks &&
+		   client->adapter->quirks->flags & I2C_AQ_NO_CLK_STRETCH)
+			return -ENODEV;
+
 	/* Reset device, loads default settings. */
 	ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
 	if (ret < 0)
diff --git a/include/linux/platform_data/si7020.h b/include/linux/platform_data/si7020.h
new file mode 100644
index 0000000..8bb5848
--- /dev/null
+++ b/include/linux/platform_data/si7020.h
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2015 Nicola Corna <nicola@corna.info>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __SI7020_H_
+#define __SI7020_H_
+
+struct si7020_platform_data {
+	bool blocking_io;
+};
+#endif /* __SI7020_H_ */
-- 
2.6.1

Re: [PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode

[Lars-Peter Clausen]

On 10/28/2015 07:58 AM, Nicola Corna wrote:
[...]
> +			holdmode = !((*client)->adapter->quirks &&
> +					(*client)->adapter->quirks->flags &
[...]
> +		   client->adapter->quirks &&
> +		   client->adapter->quirks->flags & I2C_AQ_NO_CLK_STRETCH)

This is rather ugly, can we get a helper in the I2C core something along the
lines of

i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH)

- Lars

Re: [PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode

[Nicola Corna]

October 28 2015 10:38 AM, "Lars-Peter Clausen" <lars@metafoo.de> wrote:

> On 10/28/2015 07:58 AM, Nicola Corna wrote:
> [...]
> 
>> + holdmode = !((*client)->adapter->quirks &&
>> + (*client)->adapter->quirks->flags &
> 
> [...]
> 
>> + client->adapter->quirks &&
>> + client->adapter->quirks->flags & I2C_AQ_NO_CLK_STRETCH)
> 
> This is rather ugly, can we get a helper in the I2C core something along the
> lines of
> 
> i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH)
> 
> - Lars

Something like this?

---
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index a69a9a0..a06ffc0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -613,6 +613,12 @@ static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
return (func & i2c_get_functionality(adap)) == func;
}

+/* Return 1 if adapter has the specified quirks, 0 if not. */
+static inline int i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
+{
+ return (quirks & (adap->quirks ? adap->quirks->flags : 0)) == quirks;
+}
+
/* Return the adapter number for a specific adapter */
static inline int i2c_adapter_id(struct i2c_adapter *adap)
{

Re: [PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode

[Lars-Peter Clausen]

On 10/28/2015 07:35 PM, Nicola Corna wrote:
> October 28 2015 10:38 AM, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
> 
>> On 10/28/2015 07:58 AM, Nicola Corna wrote:
>> [...]
>>
>>> + holdmode = !((*client)->adapter->quirks &&
>>> + (*client)->adapter->quirks->flags &
>>
>> [...]
>>
>>> + client->adapter->quirks &&
>>> + client->adapter->quirks->flags & I2C_AQ_NO_CLK_STRETCH)
>>
>> This is rather ugly, can we get a helper in the I2C core something along the
>> lines of
>>
>> i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH)
>>
>> - Lars
> 
> Something like this?
> 
> ---
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index a69a9a0..a06ffc0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -613,6 +613,12 @@ static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
> return (func & i2c_get_functionality(adap)) == func;
> }
> 
> +/* Return 1 if adapter has the specified quirks, 0 if not. */
> +static inline int i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
> +{
> + return (quirks & (adap->quirks ? adap->quirks->flags : 0)) == quirks;
> +}

This is not a code obfuscation contest ;)

So maybe more like this:

static inline bool i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
{
	if (!adap->quirks)
		return false;
	return (adap->quirks->flags & quirks) == quirks;
}

And please use kernel-doc for the documentation.

Re: [PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode

[Nicola Corna]

October 28 2015 7:46 PM, "Lars-Peter Clausen" <lars@metafoo.de> wrote:

> On 10/28/2015 07:35 PM, Nicola Corna wrote:
> 
>> October 28 2015 10:38 AM, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
>> 
>>> On 10/28/2015 07:58 AM, Nicola Corna wrote:
>>> [...]
>>> 
>>>> + holdmode = !((*client)->adapter->quirks &&
>>>> + (*client)->adapter->quirks->flags &
>>> 
>>> [...]
>>> 
>>>> + client->adapter->quirks &&
>>>> + client->adapter->quirks->flags & I2C_AQ_NO_CLK_STRETCH)
>>> 
>>> This is rather ugly, can we get a helper in the I2C core something along the
>>> lines of
>>> 
>>> i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH)
>>> 
>>> - Lars
>> 
>> Something like this?
>> 
>> ---
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index a69a9a0..a06ffc0 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -613,6 +613,12 @@ static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
>> return (func & i2c_get_functionality(adap)) == func;
>> }
>> 
>> +/* Return 1 if adapter has the specified quirks, 0 if not. */
>> +static inline int i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
>> +{
>> + return (quirks & (adap->quirks ? adap->quirks->flags : 0)) == quirks;
>> +}
> 
> This is not a code obfuscation contest ;)

I love one-liners ;)

> So maybe more like this:
> 
> static inline bool i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
> {
> if (!adap->quirks)
> return false;
> return (adap->quirks->flags & quirks) == quirks;
> }

Should I use bool (like in your snippet) or int (like i2c_check_functionality) as return type?

> And please use kernel-doc for the documentation.

Re: [PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode

[Nicola Corna]

October 28 2015 7:46 PM, "Lars-Peter Clausen" <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:

> On 10/28/2015 07:35 PM, Nicola Corna wrote:
> 
>> October 28 2015 10:38 AM, "Lars-Peter Clausen" <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
>> 
>>> On 10/28/2015 07:58 AM, Nicola Corna wrote:
>>> [...]
>>> 
>>>> + holdmode = !((*client)->adapter->quirks &&
>>>> + (*client)->adapter->quirks->flags &
>>> 
>>> [...]
>>> 
>>>> + client->adapter->quirks &&
>>>> + client->adapter->quirks->flags & I2C_AQ_NO_CLK_STRETCH)
>>> 
>>> This is rather ugly, can we get a helper in the I2C core something along the
>>> lines of
>>> 
>>> i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH)
>>> 
>>> - Lars
>> 
>> Something like this?
>> 
>> ---
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index a69a9a0..a06ffc0 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -613,6 +613,12 @@ static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
>> return (func & i2c_get_functionality(adap)) == func;
>> }
>> 
>> +/* Return 1 if adapter has the specified quirks, 0 if not. */
>> +static inline int i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
>> +{
>> + return (quirks & (adap->quirks ? adap->quirks->flags : 0)) == quirks;
>> +}
> 
> This is not a code obfuscation contest ;)

I love one-liners ;)

> So maybe more like this:
> 
> static inline bool i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
> {
> if (!adap->quirks)
> return false;
> return (adap->quirks->flags & quirks) == quirks;
> }

Should I use bool (like in your snippet) or int (like i2c_check_functionality) as return type?

> And please use kernel-doc for the documentation.

Re: [PATCH v4 4/4] iio: humidity: si7020: added No Hold read mode

[Lars-Peter Clausen]

>> So maybe more like this:
>>
>> static inline bool i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
>> {
>> if (!adap->quirks)
>> return false;
>> return (adap->quirks->flags & quirks) == quirks;
>> }
> 
> Should I use bool (like in your snippet) or int (like i2c_check_functionality) as return type?

I'd use bool, given that the result is a boolean value. It's semantically
more clear this way.

[PATCH v2 1/5] i2c: add i2c quirk flag for unsupported clock stretching

[Nicola Corna]

Add I2C_AQ_NO_CLK_STRETCH quirk flag, to be used when clock stretching is
not supported.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
 include/linux/i2c.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 768063b..9697002 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -493,6 +493,8 @@ struct i2c_adapter_quirks {
 /* convenience macro for typical write-then read case */
 #define I2C_AQ_COMB_WRITE_THEN_READ	(I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | \
 					 I2C_AQ_COMB_READ_SECOND | I2C_AQ_COMB_SAME_ADDR)
+/* clock stretching is not supported */
+#define I2C_AQ_NO_CLK_STRETCH		BIT(4)
 
 /*
  * i2c_adapter is the structure used to identify a physical i2c bus along
-- 
2.6.1

[PATCH v2 2/5] i2c: add I2C_AQ_NO_CLK_STRETCH to i2c-algo-bit.c

[Nicola Corna]

Add I2C_AQ_NO_CLK_STRETCH to drivers/i2c/algos/i2c-algo-bit.c when getscl
is not available.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
 drivers/i2c/algos/i2c-algo-bit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index 899bede..9d233bb 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -617,6 +617,10 @@ const struct i2c_algorithm i2c_bit_algo = {
 };
 EXPORT_SYMBOL(i2c_bit_algo);
 
+const struct i2c_adapter_quirks i2c_bit_quirk_no_clk_stretch = {
+	.flags = I2C_AQ_NO_CLK_STRETCH,
+};
+
 /*
  * registering functions to load algorithms at runtime
  */
@@ -635,6 +639,8 @@ static int __i2c_bit_add_bus(struct i2c_adapter *adap,
 	/* register new adapter to i2c module... */
 	adap->algo = &i2c_bit_algo;
 	adap->retries = 3;
+	if (bit_adap->getscl == NULL)
+		adap->quirks = &i2c_bit_quirk_no_clk_stretch;
 
 	ret = add_adapter(adap);
 	if (ret < 0)
-- 
2.6.1

Re: [PATCH v2 2/5] i2c: add I2C_AQ_NO_CLK_STRETCH to i2c-algo-bit.c

[Wolfram Sang]

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

On Thu, Oct 29, 2015 at 12:34:24PM +0100, Nicola Corna wrote:
> Add I2C_AQ_NO_CLK_STRETCH to drivers/i2c/algos/i2c-algo-bit.c when getscl
> is not available.
> 
> Signed-off-by: Nicola Corna <nicola-V0pQiMyom5mh6J55Ss3d3w@public.gmane.org>

Applied to for-next, thanks! Please check git-log next time to see how
the subsystem likes its prefixes, here "i2c: algo-bit: ...".


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 3/5] i2c: add I2C_AQ_NO_CLK_STRETCH to i2c-bcm2835.c

[Nicola Corna]

As reported here
http://www.advamation.com/knowhow/raspberrypi/rpi-i2c-bug.html
and confirmed here
https://www.raspberrypi.org/forums/viewtopic.php?p=146272
the BCM2835 has a hardware bug in its i2c module which prevents a correct
clock stretching. This patch adds the I2C_AQ_NO_CLK_STRETCH quirk flag
to i2c-bcm2835.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
 drivers/i2c/busses/i2c-bcm2835.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 3032b89..38d7102 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -222,6 +222,10 @@ static const struct i2c_algorithm bcm2835_i2c_algo = {
 	.functionality	= bcm2835_i2c_func,
 };
 
+static const struct i2c_adapter_quirks bcm2835_i2c_quirks = {
+	.flags = I2C_AQ_NO_CLK_STRETCH,
+};
+
 static int bcm2835_i2c_probe(struct platform_device *pdev)
 {
 	struct bcm2835_i2c_dev *i2c_dev;
@@ -293,6 +297,7 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 	adap->algo = &bcm2835_i2c_algo;
 	adap->dev.parent = &pdev->dev;
 	adap->dev.of_node = pdev->dev.of_node;
+	adap->quirks = &bcm2835_i2c_quirks;
 
 	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, 0);
 
-- 
2.6.1

Re: [PATCH v2 3/5] i2c: add I2C_AQ_NO_CLK_STRETCH to i2c-bcm2835.c

[Wolfram Sang]

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

On Thu, Oct 29, 2015 at 12:34:25PM +0100, Nicola Corna wrote:
> As reported here
> http://www.advamation.com/knowhow/raspberrypi/rpi-i2c-bug.html
> and confirmed here
> https://www.raspberrypi.org/forums/viewtopic.php?p=146272
> the BCM2835 has a hardware bug in its i2c module which prevents a correct
> clock stretching. This patch adds the I2C_AQ_NO_CLK_STRETCH quirk flag
> to i2c-bcm2835.
> 
> Signed-off-by: Nicola Corna <nicola@corna.info>

I put the above links into the code as comments, reworded the commit
message a little and applied it to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 4/5] i2c: add i2c_check_quirks helper function

[Nicola Corna]

This patch adds a i2c_check_quirks helper function to check the quirk flags
of an i2c adapter, in a similar way to i2c_check_functionality.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
 include/linux/i2c.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 9697002..51028f3 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -617,6 +617,20 @@ static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
 	return (func & i2c_get_functionality(adap)) == func;
 }
 
+/**
+ * i2c_check_quirks() - Function for checking the quirk flags in an i2c adapter
+ * @adap: i2c adapter
+ * @quirks: quirk flags
+ *
+ * Return: true if the adapter has all the specified quirk flags, false if not
+ */
+static inline bool i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
+{
+	if (!adap->quirks)
+		return false;
+	return (adap->quirks->flags & quirks) == quirks;
+}
+
 /* Return the adapter number for a specific adapter */
 static inline int i2c_adapter_id(struct i2c_adapter *adap)
 {
-- 
2.6.1

Re: [PATCH 4/5] i2c: add i2c_check_quirks helper function

[Jonathan Cameron]

On 29/10/15 11:34, Nicola Corna wrote:
> This patch adds a i2c_check_quirks helper function to check the quirk flags
> of an i2c adapter, in a similar way to i2c_check_functionality.
> 
> Signed-off-by: Nicola Corna <nicola-V0pQiMyom5mh6J55Ss3d3w@public.gmane.org>
I don't know about everyone else, but I'm finding this particular style
of email thread very difficult to follow.
Perhaps either:
* Start a new thread for each new series version.
* label the whole series with a rolling version number rather than doing it
  on a per patch basis.

Would just make it easier to keep track when one reads the thread a while
after it started!

Jonathan
> ---
>  include/linux/i2c.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 9697002..51028f3 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -617,6 +617,20 @@ static inline int i2c_check_functionality(struct i2c_adapter *adap, u32 func)
>  	return (func & i2c_get_functionality(adap)) == func;
>  }
>  
> +/**
> + * i2c_check_quirks() - Function for checking the quirk flags in an i2c adapter
> + * @adap: i2c adapter
> + * @quirks: quirk flags
> + *
> + * Return: true if the adapter has all the specified quirk flags, false if not
> + */
> +static inline bool i2c_check_quirks(struct i2c_adapter *adap, u64 quirks)
> +{
> +	if (!adap->quirks)
> +		return false;
> +	return (adap->quirks->flags & quirks) == quirks;
> +}
> +
>  /* Return the adapter number for a specific adapter */
>  static inline int i2c_adapter_id(struct i2c_adapter *adap)
>  {
> 

Re: [PATCH 4/5] i2c: add i2c_check_quirks helper function

[Wolfram Sang]

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

> I don't know about everyone else, but I'm finding this particular style
> of email thread very difficult to follow.

Same here.

> Perhaps either:
> * Start a new thread for each new series version.

+1

> * label the whole series with a rolling version number rather than doing it
>   on a per patch basis.

+1

> 
> Would just make it easier to keep track when one reads the thread a while
> after it started!

+1 :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] i2c: add i2c_check_quirks helper function

[Wolfram Sang]

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

On Thu, Oct 29, 2015 at 12:34:26PM +0100, Nicola Corna wrote:
> This patch adds a i2c_check_quirks helper function to check the quirk flags
> of an i2c adapter, in a similar way to i2c_check_functionality.
> 
> Signed-off-by: Nicola Corna <nicola@corna.info>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v5 5/5] iio: humidity: si7020: add No Hold read mode

[Nicola Corna]

The Si7013/20/21 modules support 2 read modes:
 * Hold mode (blocking), where the device stretches the clock until the end
of the measurement
 * No Hold mode (non-blocking), where the device replies NACK for every I2C
call during the measurement
Here the No Hold mode is implemented, selectable with the blocking_io
variable within si7020_platform_data. The default mode is Hold, unless the
adapter does not support clock stretching, in which case the No Hold mode
is used.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
This patch depends on patch "[PATCH v4 1/2] iio: humidity: si7020: replaced
bitmask on humidity values with range check"
 drivers/iio/humidity/si7020.c        | 75 ++++++++++++++++++++++++++++++++----
 include/linux/platform_data/si7020.h | 21 ++++++++++
 2 files changed, 89 insertions(+), 7 deletions(-)
 create mode 100644 include/linux/platform_data/si7020.h

diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
index 12128d1..674a61a 100644
--- a/drivers/iio/humidity/si7020.c
+++ b/drivers/iio/humidity/si7020.c
@@ -2,6 +2,7 @@
  * si7020.c - Silicon Labs Si7013/20/21 Relative Humidity and Temp Sensors
  * Copyright (c) 2013,2014  Uplogix, Inc.
  * David Barksdale <dbarksdale@uplogix.com>
+ * Copyright (c) 2015 Nicola Corna <nicola@corna.info>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -30,33 +31,78 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
+#include <linux/jiffies.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/platform_data/si7020.h>
 
 /* Measure Relative Humidity, Hold Master Mode */
 #define SI7020CMD_RH_HOLD	0xE5
+/* Measure Relative Humidity, No Hold Master Mode */
+#define SI7020CMD_RH_NO_HOLD	0xF5
 /* Measure Temperature, Hold Master Mode */
 #define SI7020CMD_TEMP_HOLD	0xE3
+/* Measure Temperature, No Hold Master Mode */
+#define SI7020CMD_TEMP_NO_HOLD	0xF3
 /* Software Reset */
 #define SI7020CMD_RESET		0xFE
+/* Relative humidity measurement timeout (us) */
+#define SI7020_RH_TIMEOUT	22800
+/* Temperature measurement timeout (us) */
+#define SI7020_TEMP_TIMEOUT	10800
+/* Minimum delay between retries (No Hold Mode) in us */
+#define SI7020_NOHOLD_SLEEP_MIN	2000
+/* Maximum delay between retries (No Hold Mode) in us */
+#define SI7020_NOHOLD_SLEEP_MAX	6000
 
 static int si7020_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
 {
 	struct i2c_client **client = iio_priv(indio_dev);
+	struct si7020_platform_data *pdata;
 	int ret;
+	bool holdmode;
+	unsigned char buf[2];
+	unsigned long start;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_data(*client,
-					       chan->type == IIO_TEMP ?
-					       SI7020CMD_TEMP_HOLD :
-					       SI7020CMD_RH_HOLD);
-		if (ret < 0)
-			return ret;
-		*val = ret >> 2;
+		pdata = dev_get_platdata(&(*client)->dev);
+		if (pdata)
+			holdmode = pdata->blocking_io;
+		else
+			holdmode = !i2c_check_quirks((*client)->adapter,
+				   I2C_AQ_NO_CLK_STRETCH);
+		if (holdmode) {
+			ret = i2c_smbus_read_word_data(*client,
+						       chan->type == IIO_TEMP ?
+						       SI7020CMD_TEMP_HOLD :
+						       SI7020CMD_RH_HOLD);
+			if (ret < 0)
+				return ret;
+			*val = ret >> 2;
+		} else {
+			ret = i2c_smbus_write_byte(*client,
+						   chan->type == IIO_TEMP ?
+						   SI7020CMD_TEMP_NO_HOLD :
+						   SI7020CMD_RH_NO_HOLD);
+			if (ret < 0)
+				return ret;
+			start = jiffies;
+			while ((ret = i2c_master_recv(*client, buf, 2)) < 0) {
+				if (time_after(jiffies, start +
+					       usecs_to_jiffies(
+							chan->type == IIO_TEMP ?
+							SI7020_TEMP_TIMEOUT :
+							SI7020_RH_TIMEOUT)))
+					return ret;
+				usleep_range(SI7020_NOHOLD_SLEEP_MIN,
+					     SI7020_NOHOLD_SLEEP_MAX);
+			}
+			*val = ((buf[0] << 8) | buf[1]) >> 2;
+		}
 		/*
 		 * Humidity values can slightly exceed the 0-100%RH
 		 * range and should be corrected by software
@@ -116,6 +162,7 @@ static int si7020_probe(struct i2c_client *client,
 {
 	struct iio_dev *indio_dev;
 	struct i2c_client **data;
+	struct si7020_platform_data *pdata;
 	int ret;
 
 	if (!i2c_check_functionality(client->adapter,
@@ -123,6 +170,20 @@ static int si7020_probe(struct i2c_client *client,
 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
 		return -ENODEV;
 
+	pdata = dev_get_platdata(&client->dev);
+	if (pdata) {
+		if (pdata->blocking_io) {
+			if (i2c_check_quirks(client->adapter,
+			   I2C_AQ_NO_CLK_STRETCH))
+				return -ENODEV;
+		} else if (!i2c_check_functionality(client->adapter,
+			  I2C_FUNC_I2C))
+			return -ENODEV;
+	} else
+		if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
+		   i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH))
+			return -ENODEV;
+
 	/* Reset device, loads default settings. */
 	ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
 	if (ret < 0)
diff --git a/include/linux/platform_data/si7020.h b/include/linux/platform_data/si7020.h
new file mode 100644
index 0000000..8bb5848
--- /dev/null
+++ b/include/linux/platform_data/si7020.h
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2015 Nicola Corna <nicola@corna.info>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __SI7020_H_
+#define __SI7020_H_
+
+struct si7020_platform_data {
+	bool blocking_io;
+};
+#endif /* __SI7020_H_ */
-- 
2.6.1

Re: [PATCH v5 5/5] iio: humidity: si7020: add No Hold read mode

[Jonathan Cameron]

On 29/10/15 11:34, Nicola Corna wrote:
> The Si7013/20/21 modules support 2 read modes:
>  * Hold mode (blocking), where the device stretches the clock until the end
> of the measurement
>  * No Hold mode (non-blocking), where the device replies NACK for every I2C
> call during the measurement
> Here the No Hold mode is implemented, selectable with the blocking_io
> variable within si7020_platform_data. The default mode is Hold, unless the
> adapter does not support clock stretching, in which case the No Hold mode
> is used.
> 
> Signed-off-by: Nicola Corna <nicola@corna.info>
I'm fine with this.  The dependency below should I think show up in the
coming merge window, so this lot might as well go via Wolfram.

Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
> This patch depends on patch "[PATCH v4 1/2] iio: humidity: si7020: replaced
> bitmask on humidity values with range check"
>  drivers/iio/humidity/si7020.c        | 75 ++++++++++++++++++++++++++++++++----
>  include/linux/platform_data/si7020.h | 21 ++++++++++
>  2 files changed, 89 insertions(+), 7 deletions(-)
>  create mode 100644 include/linux/platform_data/si7020.h
> 
> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
> index 12128d1..674a61a 100644
> --- a/drivers/iio/humidity/si7020.c
> +++ b/drivers/iio/humidity/si7020.c
> @@ -2,6 +2,7 @@
>   * si7020.c - Silicon Labs Si7013/20/21 Relative Humidity and Temp Sensors
>   * Copyright (c) 2013,2014  Uplogix, Inc.
>   * David Barksdale <dbarksdale@uplogix.com>
> + * Copyright (c) 2015 Nicola Corna <nicola@corna.info>
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms and conditions of the GNU General Public License,
> @@ -30,33 +31,78 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> +#include <linux/jiffies.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/platform_data/si7020.h>
>  
>  /* Measure Relative Humidity, Hold Master Mode */
>  #define SI7020CMD_RH_HOLD	0xE5
> +/* Measure Relative Humidity, No Hold Master Mode */
> +#define SI7020CMD_RH_NO_HOLD	0xF5
>  /* Measure Temperature, Hold Master Mode */
>  #define SI7020CMD_TEMP_HOLD	0xE3
> +/* Measure Temperature, No Hold Master Mode */
> +#define SI7020CMD_TEMP_NO_HOLD	0xF3
>  /* Software Reset */
>  #define SI7020CMD_RESET		0xFE
> +/* Relative humidity measurement timeout (us) */
> +#define SI7020_RH_TIMEOUT	22800
> +/* Temperature measurement timeout (us) */
> +#define SI7020_TEMP_TIMEOUT	10800
> +/* Minimum delay between retries (No Hold Mode) in us */
> +#define SI7020_NOHOLD_SLEEP_MIN	2000
> +/* Maximum delay between retries (No Hold Mode) in us */
> +#define SI7020_NOHOLD_SLEEP_MAX	6000
>  
>  static int si7020_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan, int *val,
>  			   int *val2, long mask)
>  {
>  	struct i2c_client **client = iio_priv(indio_dev);
> +	struct si7020_platform_data *pdata;
>  	int ret;
> +	bool holdmode;
> +	unsigned char buf[2];
> +	unsigned long start;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		ret = i2c_smbus_read_word_data(*client,
> -					       chan->type == IIO_TEMP ?
> -					       SI7020CMD_TEMP_HOLD :
> -					       SI7020CMD_RH_HOLD);
> -		if (ret < 0)
> -			return ret;
> -		*val = ret >> 2;
> +		pdata = dev_get_platdata(&(*client)->dev);
> +		if (pdata)
> +			holdmode = pdata->blocking_io;
> +		else
> +			holdmode = !i2c_check_quirks((*client)->adapter,
> +				   I2C_AQ_NO_CLK_STRETCH);
> +		if (holdmode) {
> +			ret = i2c_smbus_read_word_data(*client,
> +						       chan->type == IIO_TEMP ?
> +						       SI7020CMD_TEMP_HOLD :
> +						       SI7020CMD_RH_HOLD);
> +			if (ret < 0)
> +				return ret;
> +			*val = ret >> 2;
> +		} else {
> +			ret = i2c_smbus_write_byte(*client,
> +						   chan->type == IIO_TEMP ?
> +						   SI7020CMD_TEMP_NO_HOLD :
> +						   SI7020CMD_RH_NO_HOLD);
> +			if (ret < 0)
> +				return ret;
> +			start = jiffies;
> +			while ((ret = i2c_master_recv(*client, buf, 2)) < 0) {
> +				if (time_after(jiffies, start +
> +					       usecs_to_jiffies(
> +							chan->type == IIO_TEMP ?
> +							SI7020_TEMP_TIMEOUT :
> +							SI7020_RH_TIMEOUT)))
> +					return ret;
> +				usleep_range(SI7020_NOHOLD_SLEEP_MIN,
> +					     SI7020_NOHOLD_SLEEP_MAX);
> +			}
> +			*val = ((buf[0] << 8) | buf[1]) >> 2;
> +		}
>  		/*
>  		 * Humidity values can slightly exceed the 0-100%RH
>  		 * range and should be corrected by software
> @@ -116,6 +162,7 @@ static int si7020_probe(struct i2c_client *client,
>  {
>  	struct iio_dev *indio_dev;
>  	struct i2c_client **data;
> +	struct si7020_platform_data *pdata;
>  	int ret;
>  
>  	if (!i2c_check_functionality(client->adapter,
> @@ -123,6 +170,20 @@ static int si7020_probe(struct i2c_client *client,
>  				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>  		return -ENODEV;
>  
> +	pdata = dev_get_platdata(&client->dev);
> +	if (pdata) {
> +		if (pdata->blocking_io) {
> +			if (i2c_check_quirks(client->adapter,
> +			   I2C_AQ_NO_CLK_STRETCH))
> +				return -ENODEV;
> +		} else if (!i2c_check_functionality(client->adapter,
> +			  I2C_FUNC_I2C))
> +			return -ENODEV;
> +	} else
> +		if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
> +		   i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH))
> +			return -ENODEV;
> +
>  	/* Reset device, loads default settings. */
>  	ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
>  	if (ret < 0)
> diff --git a/include/linux/platform_data/si7020.h b/include/linux/platform_data/si7020.h
> new file mode 100644
> index 0000000..8bb5848
> --- /dev/null
> +++ b/include/linux/platform_data/si7020.h
> @@ -0,0 +1,21 @@
> +/*
> + * Copyright (C) 2015 Nicola Corna <nicola@corna.info>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __SI7020_H_
> +#define __SI7020_H_
> +
> +struct si7020_platform_data {
> +	bool blocking_io;
> +};
> +#endif /* __SI7020_H_ */
> 

Re: [PATCH v5 5/5] iio: humidity: si7020: add No Hold read mode

[Wolfram Sang]

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

On Sat, Oct 31, 2015 at 10:21:56AM +0000, Jonathan Cameron wrote:
> On 29/10/15 11:34, Nicola Corna wrote:
> > The Si7013/20/21 modules support 2 read modes:
> >  * Hold mode (blocking), where the device stretches the clock until the end
> > of the measurement
> >  * No Hold mode (non-blocking), where the device replies NACK for every I2C
> > call during the measurement
> > Here the No Hold mode is implemented, selectable with the blocking_io
> > variable within si7020_platform_data. The default mode is Hold, unless the
> > adapter does not support clock stretching, in which case the No Hold mode
> > is used.
> > 
> > Signed-off-by: Nicola Corna <nicola-V0pQiMyom5mh6J55Ss3d3w@public.gmane.org>
> I'm fine with this.  The dependency below should I think show up in the
> coming merge window, so this lot might as well go via Wolfram.
> 
> Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Not sure, will think about it...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 5/5] iio: humidity: si7020: add No Hold read mode

[Jonathan Cameron]

On 30/11/15 17:33, Wolfram Sang wrote:
> On Sat, Oct 31, 2015 at 10:21:56AM +0000, Jonathan Cameron wrote:
>> On 29/10/15 11:34, Nicola Corna wrote:
>>> The Si7013/20/21 modules support 2 read modes:
>>>  * Hold mode (blocking), where the device stretches the clock until the end
>>> of the measurement
>>>  * No Hold mode (non-blocking), where the device replies NACK for every I2C
>>> call during the measurement
>>> Here the No Hold mode is implemented, selectable with the blocking_io
>>> variable within si7020_platform_data. The default mode is Hold, unless the
>>> adapter does not support clock stretching, in which case the No Hold mode
>>> is used.
>>>
>>> Signed-off-by: Nicola Corna <nicola@corna.info>
>> I'm fine with this.  The dependency below should I think show up in the
>> coming merge window, so this lot might as well go via Wolfram.
>>
>> Acked-by: Jonathan Cameron <jic23@kernel.org>
> 
> Not sure, will think about it...
> 
Nicola,

If Wolfram doesn't pick this up, please ping me after the next merge window.
Whilst I'll still have it in my queue, it'll be so long ago by then I'll probably
miss it!

Jonathan

Re: [PATCH v5 5/5] iio: humidity: si7020: add No Hold read mode

[Wolfram Sang]

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


> If Wolfram doesn't pick this up, please ping me after the next merge window.

I tried to pick it a second ago, but it could not be applied (my branch
is based on 4.4-rc3). So, I really think it's better you pick it and
handle the conflicts.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/5] i2c: add i2c quirk flag for unsupported clock stretching

[Wolfram Sang]

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

On Thu, Oct 29, 2015 at 12:34:23PM +0100, Nicola Corna wrote:
> Add I2C_AQ_NO_CLK_STRETCH quirk flag, to be used when clock stretching is
> not supported.
> 
> Signed-off-by: Nicola Corna <nicola@corna.info>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]