[PATCH 1/3] iio:humidity:si7020: replaced bitmask on humidity values with range check

[PATCH 1/3] iio:humidity:si7020: replaced bitmask on humidity values with range check

[Nicola Corna]

The maximum possible value for the relative humidity is 55575 (100%RH).
This value, if shifted right by 2 bits, uses 14 bits and masking it with
a 12 bit mask removes 2 meaningful bits.
The masking has been replaced with a range check that sets the minimum
value at 3194 (0%RH) and the maximum at 55575 (100%RH).

Signed-off-by: Nicola Corna <nicola@corna.info>
---
 drivers/iio/humidity/si7020.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
index fa3b809..62fdbcf 100644
--- a/drivers/iio/humidity/si7020.c
+++ b/drivers/iio/humidity/si7020.c
@@ -57,8 +57,16 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 		*val = ret >> 2;
-		if (chan->type == IIO_HUMIDITYRELATIVE)
-			*val &= GENMASK(11, 0);
+		/*
+		 * Humidity values can sligthly exceed the 0-100%RH
+		 * range and should be corrected by software
+		 */
+		if (chan->type == IIO_HUMIDITYRELATIVE) {
+			if (*val < 3194) /* 0%RH */
+				*val = 3194;
+			else if (*val > 55575) /* 100%RH */
+				*val = 55575;
+		}
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_TEMP)
-- 
2.5.0

[PATCH 2/3] iio:humidity:si7020: added No Hold read mode

[Nicola Corna]

The Si7013/20/21 modules support 2 read modes:
 * Hold mode, where the device stretches the clock until the end of the
measurement
 * No Hold mode, where the device replies NACK for every I2C call during
the measurement
Here the No Hold mode is implemented, selectable with the boolean
parameter holdmode=N. The No Hold mode is less efficient, since it
requires multiple calls to the device, but it can be used as a fallback if
the clock stretching is not supported.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
I've tested this on a Raspberry Pi 2, where the clock stretching is
currently bugged.
 drivers/iio/humidity/si7020.c | 55 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
index 62fdbcf..a8bad04 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,16 +31,33 @@
 #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>
 
 /* 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 bool holdmode = 1;
+module_param(holdmode, bool, 0644);
+MODULE_PARM_DESC(holdmode, "Select whether the measurement has to be done with Hold Mode (clock stretching) or No Hold Mode (repeated calls)");
 
 static int si7020_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
@@ -47,16 +65,39 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
 {
 	struct i2c_client **client = iio_priv(indio_dev);
 	int ret;
+	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;
+		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 sligthly exceed the 0-100%RH
 		 * range and should be corrected by software
-- 
2.5.0

Re: [PATCH 2/3] iio:humidity:si7020: added No Hold read mode

[Jonathan Cameron]

On 20/08/15 15:11, Nicola Corna wrote:
> The Si7013/20/21 modules support 2 read modes:
>  * Hold mode, where the device stretches the clock until the end of the
> measurement
>  * No Hold mode, where the device replies NACK for every I2C call during
> the measurement
> Here the No Hold mode is implemented, selectable with the boolean
> parameter holdmode=N. The No Hold mode is less efficient, since it
> requires multiple calls to the device, but it can be used as a fallback if
> the clock stretching is not supported.
Interesting. Strikes me as something that should really be handled via the i2c
core (and device tree or similar bindings) rather than inside a driver as
a module parameter.  Perhaps info provided to the i2c client driver
via a check on whether the device supports clock stretching?

I'd like input from Jean on this.
> 
> Signed-off-by: Nicola Corna <nicola@corna.info>
> ---
> I've tested this on a Raspberry Pi 2, where the clock stretching is
> currently bugged.
>  drivers/iio/humidity/si7020.c | 55 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
> index 62fdbcf..a8bad04 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,16 +31,33 @@
>  #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>
>  
>  /* 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 bool holdmode = 1;
> +module_param(holdmode, bool, 0644);
> +MODULE_PARM_DESC(holdmode, "Select whether the measurement has to be done with Hold Mode (clock stretching) or No Hold Mode (repeated calls)");


>  
>  static int si7020_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan, int *val,
> @@ -47,16 +65,39 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct i2c_client **client = iio_priv(indio_dev);
>  	int ret;
> +	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;
> +		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 sligthly exceed the 0-100%RH
>  		 * range and should be corrected by software
> 

Re: [PATCH 2/3] iio:humidity:si7020: added No Hold read mode

[Nicola Corna]

August 22 2015 4:00 PM, "Jonathan Cameron" <jic23@kernel.org> wrote:=0A> =
On 20/08/15 15:11, Nicola Corna wrote:=0A> =0A>> The Si7013/20/21 modules=
 support 2 read modes:=0A>> * Hold mode, where the device stretches the c=
lock until the end of the=0A>> measurement=0A>> * No Hold mode, where the=
 device replies NACK for every I2C call during=0A>> the measurement=0A>> =
Here the No Hold mode is implemented, selectable with the boolean=0A>> pa=
rameter holdmode=3DN. The No Hold mode is less efficient, since it=0A>> r=
equires multiple calls to the device, but it can be used as a fallback if=
=0A>> the clock stretching is not supported.=0A> =0A> Interesting. Strike=
s me as something that should really be handled via the i2c=0A> core (and=
 device tree or similar bindings) rather than inside a driver as=0A> a mo=
dule parameter. Perhaps info provided to the i2c client driver=0A> via a =
check on whether the device supports clock stretching?=0A> =0A=0AReasonab=
le, but we also have to consider that:=0A * it can happen that the device=
 supports clock stretching but it is bugged=0A(like the Raspberry Pi)=0A =
* with the clock stretching the i2c bus is completely locked until the en=
d of=0Athe measurement (which can take up to 22.8 ms), while with the No =
Hold mode the=0Abus is used every 2-6 ms for very short periods (with a i=
2c clock at 100 KHz,=0Aeach call takes 0.1 ms)=0AIn some cases the No Hol=
d mode is preferable, even if the clock stretching is=0Asupported and wor=
king.=0A=0ANicola Corna=0A=0A> I'd like input from Jean on this.=0A> =0A>=
> Signed-off-by: Nicola Corna <nicola@corna.info>=0A>> ---=0A>> I've test=
ed this on a Raspberry Pi 2, where the clock stretching is=0A>> currently=
 bugged.=0A>> drivers/iio/humidity/si7020.c | 55 ++++++++++++++++++++++++=
+++++++++++++------=0A>> 1 file changed, 48 insertions(+), 7 deletions(-)=
=0A>> =0A>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humi=
dity/si7020.c=0A>> index 62fdbcf..a8bad04 100644=0A>> --- a/drivers/iio/h=
umidity/si7020.c=0A>> +++ b/drivers/iio/humidity/si7020.c=0A>> @@ -2,6 +2=
,7 @@=0A>> * si7020.c - Silicon Labs Si7013/20/21 Relative Humidity and T=
emp Sensors=0A>> * Copyright (c) 2013,2014 Uplogix, Inc.=0A>> * David Bar=
ksdale <dbarksdale@uplogix.com>=0A>> + * Copyright (c) 2015 Nicola Corna =
<nicola@corna.info>=0A>> *=0A>> * This program is free software; you can =
redistribute it and/or modify it=0A>> * under the terms and conditions of=
 the GNU General Public License,=0A>> @@ -30,16 +31,33 @@=0A>> #include <=
linux/module.h>=0A>> #include <linux/slab.h>=0A>> #include <linux/sysfs.h=
>=0A>> +#include <linux/jiffies.h>=0A>> =0A>> #include <linux/iio/iio.h>=
=0A>> #include <linux/iio/sysfs.h>=0A>> =0A>> /* Measure Relative Humidit=
y, Hold Master Mode */=0A>> #define SI7020CMD_RH_HOLD 0xE5=0A>> +/* Measu=
re Relative Humidity, No Hold Master Mode */=0A>> +#define SI7020CMD_RH_N=
O_HOLD 0xF5=0A>> /* Measure Temperature, Hold Master Mode */=0A>> #define=
 SI7020CMD_TEMP_HOLD 0xE3=0A>> +/* Measure Temperature, No Hold Master Mo=
de */=0A>> +#define SI7020CMD_TEMP_NO_HOLD 0xF3=0A>> /* Software Reset */=
=0A>> #define SI7020CMD_RESET 0xFE=0A>> +/* Relative humidity measurement=
 timeout (us) */=0A>> +#define SI7020_RH_TIMEOUT 22800=0A>> +/* Temperatu=
re measurement timeout (us) */=0A>> +#define SI7020_TEMP_TIMEOUT 10800=0A=
>> +/* Minimum delay between retries (No Hold Mode) in us */=0A>> +#defin=
e SI7020_NOHOLD_SLEEP_MIN 2000=0A>> +/* Maximum delay between retries (No=
 Hold Mode) in us */=0A>> +#define SI7020_NOHOLD_SLEEP_MAX 6000=0A>> +=0A=
>> +static bool holdmode =3D 1;=0A>> +module_param(holdmode, bool, 0644);=
=0A>> +MODULE_PARM_DESC(holdmode, "Select whether the measurement has to =
be done with Hold Mode (clock=0A>> stretching) or No Hold Mode (repeated =
calls)");=0A>> =0A>> static int si7020_read_raw(struct iio_dev *indio_dev=
,=0A>> struct iio_chan_spec const *chan, int *val,=0A>> @@ -47,16 +65,39 =
@@ static int si7020_read_raw(struct iio_dev *indio_dev,=0A>> {=0A>> stru=
ct i2c_client **client =3D iio_priv(indio_dev);=0A>> int ret;=0A>> + unsi=
gned char buf[2];=0A>> + unsigned long start;=0A>> =0A>> switch (mask) {=
=0A>> case IIO_CHAN_INFO_RAW:=0A>> - ret =3D i2c_smbus_read_word_data(*cl=
ient,=0A>> - chan->type =3D=3D IIO_TEMP ?=0A>> - SI7020CMD_TEMP_HOLD :=0A=
>> - SI7020CMD_RH_HOLD);=0A>> - if (ret < 0)=0A>> - return ret;=0A>> - *v=
al =3D ret >> 2;=0A>> + if (holdmode) {=0A>> + ret =3D i2c_smbus_read_wor=
d_data(*client,=0A>> + chan->type =3D=3D IIO_TEMP ?=0A>> + SI7020CMD_TEMP=
_HOLD :=0A>> + SI7020CMD_RH_HOLD);=0A>> + if (ret < 0)=0A>> + return ret;=
=0A>> + *val =3D ret >> 2;=0A>> + } else {=0A>> + ret =3D i2c_smbus_write=
_byte(*client,=0A>> + chan->type =3D=3D IIO_TEMP ?=0A>> + SI7020CMD_TEMP_=
NO_HOLD :=0A>> + SI7020CMD_RH_NO_HOLD);=0A>> + if (ret < 0)=0A>> + return=
 ret;=0A>> + start =3D jiffies;=0A>> + while ((ret =3D i2c_master_recv(*c=
lient, buf, 2)) < 0) {=0A>> + if (time_after(jiffies, start +=0A>> + usec=
s_to_jiffies(=0A>> + chan->type =3D=3D IIO_TEMP ?=0A>> + SI7020_TEMP_TIME=
OUT :=0A>> + SI7020_RH_TIMEOUT)))=0A>> + return ret;=0A>> + usleep_range(=
SI7020_NOHOLD_SLEEP_MIN,=0A>> + SI7020_NOHOLD_SLEEP_MAX);=0A>> + }=0A>> +=
 *val =3D ((buf[0] << 8) | buf[1]) >> 2;=0A>> + }=0A>> /*=0A>> * Humidity=
 values can sligthly exceed the 0-100%RH=0A>> * range and should be corre=
cted by software

Re: [PATCH 2/3] iio:humidity:si7020: added No Hold read mode

[Jean Delvare]

Le Sunday 23 August 2015 à 09:50 +0000, Nicola Corna a écrit :
> August 22 2015 4:00 PM, "Jonathan Cameron" <jic23@kernel.org> wrote:
> > On 20/08/15 15:11, Nicola Corna wrote:
> > 
> >> The Si7013/20/21 modules support 2 read modes:
> >> * Hold mode, where the device stretches the clock until the end of the
> >> measurement
> >> * No Hold mode, where the device replies NACK for every I2C call during
> >> the measurement
> >> Here the No Hold mode is implemented, selectable with the boolean
> >> parameter holdmode=N. The No Hold mode is less efficient, since it
> >> requires multiple calls to the device, but it can be used as a fallback if
> >> the clock stretching is not supported.
> > 
> > Interesting. Strikes me as something that should really be handled via the i2c
> > core (and device tree or similar bindings) rather than inside a driver as
> > a module parameter. Perhaps info provided to the i2c client driver
> > via a check on whether the device supports clock stretching?

There currently is no way for bus drivers to report whether they support
clock stretching or not. We could add a functionality flag for this,
like:

#define I2C_FUNC_NO_CLK_STRETCH          0x00000040 /* No check for SCL low */

For example i2c-algo-bit would set this flag if no getscl callback is
provided.

> Reasonable, but we also have to consider that:
>  * it can happen that the device supports clock stretching but it is bugged
> (like the Raspberry Pi)

I can't really see the difference between "supports clock stretching but
it is bugged" and "does not support clock stretching.

>  * with the clock stretching the i2c bus is completely locked until the end of
> the measurement (which can take up to 22.8 ms), while with the No Hold mode the
> bus is used every 2-6 ms for very short periods (with a i2c clock at 100 KHz,
> each call takes 0.1 ms)

I2C puts no limit on clock stretching and SMBus allows for up to 50 ms,
so hopefully 22.8 ms should be non-fatal in most cases. But I understand
there may be latency concerns.

> In some cases the No Hold mode is preferable, even if the clock stretching is
> supported and working.

Got it. But something like I2C_FUNC_NO_CLK_STRETCH would let drivers
pick a sane default at least.

> > I'd like input from Jean on this.

In fact you wanted input from Wolfram (Cc'd) as the new (you know, like
for 3 years now) maintainer of the i2c subsystem ;-)

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/3] iio:humidity:si7020: added No Hold read mode

[Jonathan Cameron]

On 27 August 2015 15:40:36 BST, Jean Delvare <jdelvare@suse.de> wrote:
>Le Sunday 23 August 2015 à 09:50 +0000, Nicola Corna a écrit :
>> August 22 2015 4:00 PM, "Jonathan Cameron" <jic23@kernel.org> wrote:
>> > On 20/08/15 15:11, Nicola Corna wrote:
>> > 
>> >> The Si7013/20/21 modules support 2 read modes:
>> >> * Hold mode, where the device stretches the clock until the end of
>the
>> >> measurement
>> >> * No Hold mode, where the device replies NACK for every I2C call
>during
>> >> the measurement
>> >> Here the No Hold mode is implemented, selectable with the boolean
>> >> parameter holdmode=N. The No Hold mode is less efficient, since it
>> >> requires multiple calls to the device, but it can be used as a
>fallback if
>> >> the clock stretching is not supported.
>> > 
>> > Interesting. Strikes me as something that should really be handled
>via the i2c
>> > core (and device tree or similar bindings) rather than inside a
>driver as
>> > a module parameter. Perhaps info provided to the i2c client driver
>> > via a check on whether the device supports clock stretching?
>
>There currently is no way for bus drivers to report whether they
>support
>clock stretching or not. We could add a functionality flag for this,
>like:
>
>#define I2C_FUNC_NO_CLK_STRETCH          0x00000040 /* No check for SCL
>low */
>
>For example i2c-algo-bit would set this flag if no getscl callback is
>provided.
>
>> Reasonable, but we also have to consider that:
>>  * it can happen that the device supports clock stretching but it is
>bugged
>> (like the Raspberry Pi)
>
>I can't really see the difference between "supports clock stretching
>but
>it is bugged" and "does not support clock stretching.
>
>>  * with the clock stretching the i2c bus is completely locked until
>the end of
>> the measurement (which can take up to 22.8 ms), while with the No
>Hold mode the
>> bus is used every 2-6 ms for very short periods (with a i2c clock at
>100 KHz,
>> each call takes 0.1 ms)
>
>I2C puts no limit on clock stretching and SMBus allows for up to 50 ms,
>so hopefully 22.8 ms should be non-fatal in most cases. But I
>understand
>there may be latency concerns.
>
>> In some cases the No Hold mode is preferable, even if the clock
>stretching is
>> supported and working.
>
>Got it. But something like I2C_FUNC_NO_CLK_STRETCH would let drivers
>pick a sane default at least.
>
>> > I'd like input from Jean on this.
>
>In fact you wanted input from Wolfram (Cc'd) as the new (you know, like
>for 3 years now) maintainer of the i2c subsystem ;-)
Doh! Must have been half asleep... Sorry Jean and Wolfram!

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 2/3] iio:humidity:si7020: added No Hold read mode

[Nicola Corna]

August 27 2015 4:40 PM, "Jean Delvare" <jdelvare@suse.de> wrote:

> Le Sunday 23 August 2015 à 09:50 +0000, Nicola Corna a écrit :
> 
>> August 22 2015 4:00 PM, "Jonathan Cameron" <jic23@kernel.org> wrote:
>>> On 20/08/15 15:11, Nicola Corna wrote:
>>> 
>>>> The Si7013/20/21 modules support 2 read modes:
>>>> * Hold mode, where the device stretches the clock until the end of the
>>>> measurement
>>>> * No Hold mode, where the device replies NACK for every I2C call during
>>>> the measurement
>>>> Here the No Hold mode is implemented, selectable with the boolean
>>>> parameter holdmode=N. The No Hold mode is less efficient, since it
>>>> requires multiple calls to the device, but it can be used as a fallback if
>>>> the clock stretching is not supported.
>>> 
>>> Interesting. Strikes me as something that should really be handled via the i2c
>>> core (and device tree or similar bindings) rather than inside a driver as
>>> a module parameter. Perhaps info provided to the i2c client driver
>>> via a check on whether the device supports clock stretching?
> 
> There currently is no way for bus drivers to report whether they support
> clock stretching or not. We could add a functionality flag for this,
> like:
> 
> #define I2C_FUNC_NO_CLK_STRETCH 0x00000040 /* No check for SCL low */
> 
> For example i2c-algo-bit would set this flag if no getscl callback is
> provided.

Something like this?

---
 drivers/i2c/algos/i2c-algo-bit.c | 5 ++++-
 include/uapi/linux/i2c.h         | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index 899bede..618deb3 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -605,7 +605,10 @@ static u32 bit_func(struct i2c_adapter *adap)
 	return I2C_FUNC_I2C | I2C_FUNC_NOSTART | I2C_FUNC_SMBUS_EMUL |
 	       I2C_FUNC_SMBUS_READ_BLOCK_DATA |
 	       I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
-	       I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
+	       I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING |
+	       (((struct i2c_algo_bit_data *)adap->algo_data)->getscl ?
+	       0 : I2C_FUNC_NO_CLK_STRETCH);
+
 }
 
 
diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index b0a7dd6..59e4b43 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -88,6 +88,7 @@ struct i2c_msg {
 #define I2C_FUNC_SMBUS_PEC		0x00000008
 #define I2C_FUNC_NOSTART		0x00000010 /* I2C_M_NOSTART */
 #define I2C_FUNC_SLAVE			0x00000020
+#define I2C_FUNC_NO_CLK_STRETCH		0x00000040 /* No check for SCL low */
 #define I2C_FUNC_SMBUS_BLOCK_PROC_CALL	0x00008000 /* SMBus 2.0 */
 #define I2C_FUNC_SMBUS_QUICK		0x00010000
 #define I2C_FUNC_SMBUS_READ_BYTE	0x00020000
-- 

Why do we have to use a negative flag? Doesn't it make more sense to use a
I2C_FUNC_CLK_STRETCH flag and add it to every device that supports it?

>> Reasonable, but we also have to consider that:
>> * it can happen that the device supports clock stretching but it is bugged
>> (like the Raspberry Pi)
> 
> I can't really see the difference between "supports clock stretching but
> it is bugged" and "does not support clock stretching.
> 
>> * with the clock stretching the i2c bus is completely locked until the end of
>> the measurement (which can take up to 22.8 ms), while with the No Hold mode the
>> bus is used every 2-6 ms for very short periods (with a i2c clock at 100 KHz,
>> each call takes 0.1 ms)
> 
> I2C puts no limit on clock stretching and SMBus allows for up to 50 ms,
> so hopefully 22.8 ms should be non-fatal in most cases. But I understand
> there may be latency concerns.
> 
>> In some cases the No Hold mode is preferable, even if the clock stretching is
>> supported and working.
> 
> Got it. But something like I2C_FUNC_NO_CLK_STRETCH would let drivers
> pick a sane default at least.

I've looked for similar situations in the kernel code and I've found this module
https://github.com/torvalds/linux/blob/master/Documentation/hwmon/shtc1
The situation is identical, but here the developer used shtc1_platform_data to
pass the parameter. This solution seems better to me (multiple instances can
have different parameters), but can a parameter in a platform_data be passed
with a userspace instantiation?

>>> I'd like input from Jean on this.
> 
> In fact you wanted input from Wolfram (Cc'd) as the new (you know, like
> for 3 years now) maintainer of the i2c subsystem ;-)
> 
> --
> Jean Delvare
> SUSE L3 Support

Re: [PATCH 2/3] iio:humidity:si7020: added No Hold read mode

[Jean Delvare]

Hi Nicola,

On Fri, 28 Aug 2015 07:32:02 +0000, Nicola Corna wrote:
> August 27 2015 4:40 PM, "Jean Delvare" <jdelvare@suse.de> wrote:
> > There currently is no way for bus drivers to report whether they support
> > clock stretching or not. We could add a functionality flag for this,
> > like:
> > 
> > #define I2C_FUNC_NO_CLK_STRETCH 0x00000040 /* No check for SCL low */
> > 
> > For example i2c-algo-bit would set this flag if no getscl callback is
> > provided.
> 
> Something like this?

Yes.

> ---
>  drivers/i2c/algos/i2c-algo-bit.c | 5 ++++-
>  include/uapi/linux/i2c.h         | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
> index 899bede..618deb3 100644
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -605,7 +605,10 @@ static u32 bit_func(struct i2c_adapter *adap)
>  	return I2C_FUNC_I2C | I2C_FUNC_NOSTART | I2C_FUNC_SMBUS_EMUL |
>  	       I2C_FUNC_SMBUS_READ_BLOCK_DATA |
>  	       I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
> -	       I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
> +	       I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING |
> +	       (((struct i2c_algo_bit_data *)adap->algo_data)->getscl ?
> +	       0 : I2C_FUNC_NO_CLK_STRETCH);
> +

No blank line at end of function please.

Also I think this would all be more readable if adap->algo_data was
stored in a local variable.

>  }
>  
>  
> diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> index b0a7dd6..59e4b43 100644
> --- a/include/uapi/linux/i2c.h
> +++ b/include/uapi/linux/i2c.h
> @@ -88,6 +88,7 @@ struct i2c_msg {
>  #define I2C_FUNC_SMBUS_PEC		0x00000008
>  #define I2C_FUNC_NOSTART		0x00000010 /* I2C_M_NOSTART */
>  #define I2C_FUNC_SLAVE			0x00000020
> +#define I2C_FUNC_NO_CLK_STRETCH		0x00000040 /* No check for SCL low */
>  #define I2C_FUNC_SMBUS_BLOCK_PROC_CALL	0x00008000 /* SMBus 2.0 */
>  #define I2C_FUNC_SMBUS_QUICK		0x00010000
>  #define I2C_FUNC_SMBUS_READ_BYTE	0x00020000

This should be documented in Documentation/i2c/functionality too.

> -- 

Please do not include a signature separator ("-- ") in the middle of
messages, that makes replying to them more difficult.

> Why do we have to use a negative flag? Doesn't it make more sense to use a
> I2C_FUNC_CLK_STRETCH flag and add it to every device that supports it?

Two reasons:

1* Clock stretching support is a mandatory part of the I2C
specification, devices not supporting that are not compliant. It makes
sense to make this non-compliance visible.

2* Thankfully a vast majority of the ~150 i2c bus drivers in the kernel do
support clock stretching, and I'd rather add a flag to 5 drivers than
to 145.

> >> In some cases the No Hold mode is preferable, even if the clock stretching is
> >> supported and working.  
> > 
> > Got it. But something like I2C_FUNC_NO_CLK_STRETCH would let drivers
> > pick a sane default at least.  
> 
> I've looked for similar situations in the kernel code and I've found this module
> https://github.com/torvalds/linux/blob/master/Documentation/hwmon/shtc1
> The situation is identical, but here the developer used shtc1_platform_data to
> pass the parameter. This solution seems better to me (multiple instances can
> have different parameters),

Indeed. I wanted to comment on that previously but forgot. Module
parameters are theoretically inadequate for per-device settings, even
though in practice this tends to work out in most cases.

> but can a parameter in a platform_data be passed
> with a userspace instantiation?

No, it can't. You'd need a sysfs attribute, but non-standard sysfs
attributes need to be documented properly. Is that a big problem in
practice though? User-space instantiation of i2c devices was originally
meant for developers or for working around bugs, I did not expect this
interface to be heavily used (which is why it is so basic.)

-- 
Jean Delvare
SUSE L3 Support

[PATCH 3/3] iio:humidity:si7020: added processed data

[Nicola Corna]

The value given by the Si7013/20/21 modules needs a conversion using a
small expression in order to obtain the measurement in °C or %RH. This
patch adds the computation inside the si7020 module, allowing the user to
obtain the measurement in m°C and 10E-3%RH without any additional
calculation.

Signed-off-by: Nicola Corna <nicola@corna.info>
---
 drivers/iio/humidity/si7020.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
index a8bad04..c534706 100644
--- a/drivers/iio/humidity/si7020.c
+++ b/drivers/iio/humidity/si7020.c
@@ -70,6 +70,7 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+	case IIO_CHAN_INFO_PROCESSED:
 		if (holdmode) {
 			ret = i2c_smbus_read_word_data(*client,
 						       chan->type == IIO_TEMP ?
@@ -108,6 +109,17 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
 			else if (*val > 55575) /* 100%RH */
 				*val = 55575;
 		}
+		if (mask == IIO_CHAN_INFO_PROCESSED) {
+			/*
+			 * To avoid overflows the fractions can be simplified:
+			 * temperature --> 175720 / (65536 >> 2) = 21965 / 2048
+			 * humidity    --> 125000 / (65536 >> 2) = 15625 / 2048
+			 */
+			if (chan->type == IIO_TEMP)
+				*val = (*val - 4368) * 21965 / 2048;
+			else
+				*val = (*val - 768) * 15625 / 2048;
+		}
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_TEMP)
@@ -142,12 +154,14 @@ static const struct iio_chan_spec si7020_channels[] = {
 	{
 		.type = IIO_HUMIDITYRELATIVE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
+			BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
+			BIT(IIO_CHAN_INFO_PROCESSED),
 	},
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
+			BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
+			BIT(IIO_CHAN_INFO_PROCESSED),
 	}
 };
 
-- 
2.5.0

Re: [PATCH 3/3] iio:humidity:si7020: added processed data

[Crt Mori]

One comment inline.
On 20 August 2015 at 16:11, Nicola Corna <nicola@corna.info> wrote:
>
> The value given by the
> Si7013/20/21 modules needs a conversion using a
> small expression in order to obtain the measurement in °C or %RH. This
> patch adds the computation inside the si7020 module, allowing the user to
> obtain the measurement in m°C and 10E-3%RH without any additional
> calculation.
>
> Signed-off-by: Nicola Corna <nicola@corna.info>
Acked-by: Crt Mori <cmo@melexis.com>
>
> ---
>  drivers/iio/humidity/si7020.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
> index a8bad04..c534706 100644
> --- a/drivers/iio/humidity/si7020.c
> +++ b/drivers/iio/humidity/si7020.c
> @@ -70,6 +70,7 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
>
>         switch (mask) {
>         case IIO_CHAN_INFO_RAW:
> +       case IIO_CHAN_INFO_PROCESSED:
>                 if (holdmode) {
>                         ret = i2c_smbus_read_word_data(*client,
>                                                        chan->type == IIO_TEMP ?
> @@ -108,6 +109,17 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
>                         else if (*val > 55575) /* 100%RH */
>                                 *val = 55575;
>                 }
> +               if (mask == IIO_CHAN_INFO_PROCESSED) {
> +                       /*
> +                        * To avoid overflows the fractions can be simplified:
> +                        * temperature --> 175720 / (65536 >> 2) = 21965 / 2048
> +                        * humidity    --> 125000 / (65536 >> 2) = 15625 / 2048
> +                        */
> +                       if (chan->type == IIO_TEMP)
> +                               *val = (*val - 4368) * 21965 / 2048;
I was suggested by Peter Meerwald to use parenthesis in case like this
(and below).
>
> +                       else
> +                               *val = (*val - 768) * 15625 / 2048;
> +               }
>                 return IIO_VAL_INT;
>         case IIO_CHAN_INFO_SCALE:
>                 if (chan->type == IIO_TEMP)
> @@ -142,12 +154,14 @@ static const struct iio_chan_spec si7020_channels[] = {
>         {
>                 .type = IIO_HUMIDITYRELATIVE,
>                 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -                       BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
> +                       BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
> +                       BIT(IIO_CHAN_INFO_PROCESSED),
>         },
>         {
>                 .type = IIO_TEMP,
>                 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -                       BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
> +                       BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
> +                       BIT(IIO_CHAN_INFO_PROCESSED),
>         }
>  };
>
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] iio:humidity:si7020: added processed data

[Jonathan Cameron]

On 20/08/15 15:11, Nicola Corna wrote:
> The value given by the Si7013/20/21 modules needs a conversion using a
> small expression in order to obtain the measurement in °C or %RH. This
> patch adds the computation inside the si7020 module, allowing the user to
> obtain the measurement in m°C and 10E-3%RH without any additional
> calculation.
> 
> Signed-off-by: Nicola Corna <nicola@corna.info>
What is the benefit in doing this?  Userspace already has all the numbers
to calculate the value?  The calculation is trivial to do in userspace.
The basic principle of IIO is to do as little as possible in the driver
and to leave such calculations to userspace which has the benefit of
easy access to floating point calculations if desired.

So sorry, not taking this one.

Jonathan
> ---
>  drivers/iio/humidity/si7020.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
> index a8bad04..c534706 100644
> --- a/drivers/iio/humidity/si7020.c
> +++ b/drivers/iio/humidity/si7020.c
> @@ -70,6 +70,7 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +	case IIO_CHAN_INFO_PROCESSED:
>  		if (holdmode) {
>  			ret = i2c_smbus_read_word_data(*client,
>  						       chan->type == IIO_TEMP ?
> @@ -108,6 +109,17 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
>  			else if (*val > 55575) /* 100%RH */
>  				*val = 55575;
>  		}
> +		if (mask == IIO_CHAN_INFO_PROCESSED) {
> +			/*
> +			 * To avoid overflows the fractions can be simplified:
> +			 * temperature --> 175720 / (65536 >> 2) = 21965 / 2048
> +			 * humidity    --> 125000 / (65536 >> 2) = 15625 / 2048
> +			 */
> +			if (chan->type == IIO_TEMP)
> +				*val = (*val - 4368) * 21965 / 2048;
> +			else
> +				*val = (*val - 768) * 15625 / 2048;
> +		}
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		if (chan->type == IIO_TEMP)
> @@ -142,12 +154,14 @@ static const struct iio_chan_spec si7020_channels[] = {
>  	{
>  		.type = IIO_HUMIDITYRELATIVE,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -			BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
> +			BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
> +			BIT(IIO_CHAN_INFO_PROCESSED),
>  	},
>  	{
>  		.type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -			BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
> +			BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
> +			BIT(IIO_CHAN_INFO_PROCESSED),
>  	}
>  };
>  
> 

Re: [PATCH 3/3] iio:humidity:si7020: added processed data

[Nicola Corna]

August 22 2015 7:21 PM, "Jonathan Cameron" <jic23@kernel.org> wrote:=0A> =
On 20/08/15 15:11, Nicola Corna wrote:=0A> =0A>> The value given by the S=
i7013/20/21 modules needs a conversion using a=0A>> small expression in o=
rder to obtain the measurement in =C2=B0C or %RH. This=0A>> patch adds th=
e computation inside the si7020 module, allowing the user to=0A>> obtain =
the measurement in m=C2=B0C and 10E-3%RH without any additional=0A>> calc=
ulation.=0A>> =0A>> Signed-off-by: Nicola Corna <nicola@corna.info>=0A> =
=0A> What is the benefit in doing this? Userspace already has all the num=
bers=0A> to calculate the value? The calculation is trivial to do in user=
space.=0A> The basic principle of IIO is to do as little as possible in t=
he driver=0A> and to leave such calculations to userspace which has the b=
enefit of=0A> easy access to floating point calculations if desired.=0A> =
=0A> So sorry, not taking this one.=0A> =0A> Jonathan=0A> =0A=0AOk, thank=
s anyway.=0A=0A>> ---=0A>> drivers/iio/humidity/si7020.c | 18 +++++++++++=
+++++--=0A>> 1 file changed, 16 insertions(+), 2 deletions(-)=0A>> =0A>> =
diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.=
c=0A>> index a8bad04..c534706 100644=0A>> --- a/drivers/iio/humidity/si70=
20.c=0A>> +++ b/drivers/iio/humidity/si7020.c=0A>> @@ -70,6 +70,7 @@ stat=
ic int si7020_read_raw(struct iio_dev *indio_dev,=0A>> =0A>> switch (mask=
) {=0A>> case IIO_CHAN_INFO_RAW:=0A>> + case IIO_CHAN_INFO_PROCESSED:=0A>=
> if (holdmode) {=0A>> ret =3D i2c_smbus_read_word_data(*client,=0A>> cha=
n->type =3D=3D IIO_TEMP ?=0A>> @@ -108,6 +109,17 @@ static int si7020_rea=
d_raw(struct iio_dev *indio_dev,=0A>> else if (*val > 55575) /* 100%RH */=
=0A>> *val =3D 55575;=0A>> }=0A>> + if (mask =3D=3D IIO_CHAN_INFO_PROCESS=
ED) {=0A>> + /*=0A>> + * To avoid overflows the fractions can be simplifi=
ed:=0A>> + * temperature --> 175720 / (65536 >> 2) =3D 21965 / 2048=0A>> =
+ * humidity --> 125000 / (65536 >> 2) =3D 15625 / 2048=0A>> + */=0A>> + =
if (chan->type =3D=3D IIO_TEMP)=0A>> + *val =3D (*val - 4368) * 21965 / 2=
048;=0A>> + else=0A>> + *val =3D (*val - 768) * 15625 / 2048;=0A>> + }=0A=
>> return IIO_VAL_INT;=0A>> case IIO_CHAN_INFO_SCALE:=0A>> if (chan->type=
 =3D=3D IIO_TEMP)=0A>> @@ -142,12 +154,14 @@ static const struct iio_chan=
_spec si7020_channels[] =3D {=0A>> {=0A>> .type =3D IIO_HUMIDITYRELATIVE,=
=0A>> .info_mask_separate =3D BIT(IIO_CHAN_INFO_RAW) |=0A>> - BIT(IIO_CHA=
N_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),=0A>> + BIT(IIO_CHAN_INFO_SCALE=
) | BIT(IIO_CHAN_INFO_OFFSET) |=0A>> + BIT(IIO_CHAN_INFO_PROCESSED),=0A>>=
 },=0A>> {=0A>> .type =3D IIO_TEMP,=0A>> .info_mask_separate =3D BIT(IIO_=
CHAN_INFO_RAW) |=0A>> - BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFS=
ET),=0A>> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |=0A>> +=
 BIT(IIO_CHAN_INFO_PROCESSED),=0A>> }=0A>> };

Re: [PATCH 1/3] iio:humidity:si7020: replaced bitmask on humidity values with range check

[Hartmut Knaack]

Nicola Corna schrieb am 20.08.2015 um 16:11:
> The maximum possible value for the relative humidity is 55575 (100%RH).
> This value, if shifted right by 2 bits, uses 14 bits and masking it with
> a 12 bit mask removes 2 meaningful bits.
> The masking has been replaced with a range check that sets the minimum
> value at 3194 (0%RH) and the maximum at 55575 (100%RH).

Hmm, I guess the mask should have been applied for bits 13:2, but your
solution confirms better to the data sheet. Just one essential detail:
The raw sensor data has already been divided by 4 when you do the boundary
check. So, you need to divide the boundaries, as well.
Btw, you can make this a one-liner using clamp_val, check it out: [1]

[1]http://lxr.free-electrons.com/source/include/linux/kernel.h#L795

> 
> Signed-off-by: Nicola Corna <nicola@corna.info>
> ---
>  drivers/iio/humidity/si7020.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
> index fa3b809..62fdbcf 100644
> --- a/drivers/iio/humidity/si7020.c
> +++ b/drivers/iio/humidity/si7020.c
> @@ -57,8 +57,16 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  		*val = ret >> 2;
> -		if (chan->type == IIO_HUMIDITYRELATIVE)
> -			*val &= GENMASK(11, 0);
> +		/*
> +		 * Humidity values can sligthly exceed the 0-100%RH
> +		 * range and should be corrected by software
> +		 */
> +		if (chan->type == IIO_HUMIDITYRELATIVE) {
> +			if (*val < 3194) /* 0%RH */
> +				*val = 3194;
> +			else if (*val > 55575) /* 100%RH */
> +				*val = 55575;
> +		}
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		if (chan->type == IIO_TEMP)
> 

Re: [PATCH 1/3] iio:humidity:si7020: replaced bitmask on humidity values with range check

[Nicola Corna]

August 20 2015 10:49 PM, "Hartmut Knaack" <knaack.h@gmx.de> wrote:
> Nicola Corna schrieb am 20.08.2015 um 16:11:
> 
>> The maximum possible value for the relative humidity is 55575 (100%RH).
>> This value, if shifted right by 2 bits, uses 14 bits and masking it with
>> a 12 bit mask removes 2 meaningful bits.
>> The masking has been replaced with a range check that sets the minimum
>> value at 3194 (0%RH) and the maximum at 55575 (100%RH).
> 
> Hmm, I guess the mask should have been applied for bits 13:2, but your
> solution confirms better to the data sheet. Just one essential detail:
> The raw sensor data has already been divided by 4 when you do the boundary
> check. So, you need to divide the boundaries, as well.
> Btw, you can make this a one-liner using clamp_val, check it out: [1]
> 
> [1]http://lxr.free-electrons.com/source/include/linux/kernel.h#L795
> 

Yes, you're right. Thanks.
Forgive me for my ignorance, but do I have now to upload the [PATCH v2] for
all the three patches?

>> Signed-off-by: Nicola Corna <nicola@corna.info>
>> ---
>> drivers/iio/humidity/si7020.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
>> index fa3b809..62fdbcf 100644
>> --- a/drivers/iio/humidity/si7020.c
>> +++ b/drivers/iio/humidity/si7020.c
>> @@ -57,8 +57,16 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
>> if (ret < 0)
>> return ret;
>> *val = ret >> 2;
>> - if (chan->type == IIO_HUMIDITYRELATIVE)
>> - *val &= GENMASK(11, 0);
>> + /*
>> + * Humidity values can sligthly exceed the 0-100%RH
>> + * range and should be corrected by software
>> + */
>> + if (chan->type == IIO_HUMIDITYRELATIVE) {
>> + if (*val < 3194) /* 0%RH */
>> + *val = 3194;
>> + else if (*val > 55575) /* 100%RH */
>> + *val = 55575;
>> + }
>> return IIO_VAL_INT;
>> case IIO_CHAN_INFO_SCALE:
>> if (chan->type == IIO_TEMP)

Re: [PATCH 1/3] iio:humidity:si7020: replaced bitmask on humidity values with range check

[Hartmut Knaack]

Nicola Corna schrieb am 20.08.2015 um 23:57:
> August 20 2015 10:49 PM, "Hartmut Knaack" <knaack.h@gmx.de> wrote:
>> Nicola Corna schrieb am 20.08.2015 um 16:11:
>>
>>> The maximum possible value for the relative humidity is 55575 (100%RH).
>>> This value, if shifted right by 2 bits, uses 14 bits and masking it with
>>> a 12 bit mask removes 2 meaningful bits.
>>> The masking has been replaced with a range check that sets the minimum
>>> value at 3194 (0%RH) and the maximum at 55575 (100%RH).
>>
>> Hmm, I guess the mask should have been applied for bits 13:2, but your
>> solution confirms better to the data sheet. Just one essential detail:
>> The raw sensor data has already been divided by 4 when you do the boundary
>> check. So, you need to divide the boundaries, as well.
>> Btw, you can make this a one-liner using clamp_val, check it out: [1]
>>
>> [1]http://lxr.free-electrons.com/source/include/linux/kernel.h#L795
>>
> 
> Yes, you're right. Thanks.
> Forgive me for my ignorance, but do I have now to upload the [PATCH v2] for
> all the three patches?
> 

That's what we usually do. And since your third patch depends on this one,
it needs to be updated as well.

>>> Signed-off-by: Nicola Corna <nicola@corna.info>
>>> ---
>>> drivers/iio/humidity/si7020.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
>>> index fa3b809..62fdbcf 100644
>>> --- a/drivers/iio/humidity/si7020.c
>>> +++ b/drivers/iio/humidity/si7020.c
>>> @@ -57,8 +57,16 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
>>> if (ret < 0)
>>> return ret;
>>> *val = ret >> 2;
>>> - if (chan->type == IIO_HUMIDITYRELATIVE)
>>> - *val &= GENMASK(11, 0);
>>> + /*
>>> + * Humidity values can sligthly exceed the 0-100%RH
>>> + * range and should be corrected by software
>>> + */
>>> + if (chan->type == IIO_HUMIDITYRELATIVE) {
>>> + if (*val < 3194) /* 0%RH */
>>> + *val = 3194;
>>> + else if (*val > 55575) /* 100%RH */
>>> + *val = 55575;
>>> + }
>>> return IIO_VAL_INT;
>>> case IIO_CHAN_INFO_SCALE:
>>> if (chan->type == IIO_TEMP)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>