[PATCH 1/2] iio: dht11: Add locking

[PATCH 1/2] iio: dht11: Add locking

[Richard Weinberger]

Protect the read function from concurrent reads.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/iio/humidity/dht11.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 623c145..7636e66 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -57,6 +57,7 @@ struct dht11 {
 	int				irq;
 
 	struct completion		completion;
+	struct mutex			lock;
 
 	s64				timestamp;
 	int				temperature;
@@ -146,16 +147,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 	int ret;
 
 	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
+		mutex_lock(&dht11->lock);
 		reinit_completion(&dht11->completion);
 
 		dht11->num_edges = 0;
 		ret = gpio_direction_output(dht11->gpio, 0);
 		if (ret)
-			goto err;
+			goto err_unlock;
 		msleep(DHT11_START_TRANSMISSION);
 		ret = gpio_direction_input(dht11->gpio);
 		if (ret)
-			goto err;
+			goto err_unlock;
 
 		ret = wait_for_completion_killable_timeout(&dht11->completion,
 								 HZ);
@@ -166,14 +168,16 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 			ret = -ETIMEDOUT;
 		}
 		if (ret < 0)
-			goto err;
+			goto err_unlock;
 
 		ret = dht11_decode(dht11,
 				dht11->num_edges == DHT11_EDGES_PER_READ ?
 					DHT11_EDGES_PREAMBLE :
 					DHT11_EDGES_PREAMBLE - 2);
+
+		mutex_unlock(&dht11->lock);
 		if (ret)
-			goto err;
+			goto out;
 	}
 
 	ret = IIO_VAL_INT;
@@ -183,9 +187,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		*val = dht11->humidity;
 	else
 		ret = -EINVAL;
-err:
+out:
 	dht11->num_edges = -1;
 	return ret;
+
+err_unlock:
+	mutex_unlock(&dht11->lock);
+	goto out;
 }
 
 static const struct iio_info dht11_iio_info = {
@@ -268,6 +276,7 @@ static int dht11_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, iio);
 
 	init_completion(&dht11->completion);
+	mutex_init(&dht11->lock);
 	iio->name = pdev->name;
 	iio->dev.parent = &pdev->dev;
 	iio->info = &dht11_iio_info;
-- 
2.1.0

[PATCH 2/2] iio: dht11: IRQ fixes

[Richard Weinberger]

We must not set an IRQ pin into output mode.
Set the data GPIO only into IRQ mode if needed.
This is a bit hacky but the only way to communicate in a sane
way with the device.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/iio/humidity/dht11.c | 56 ++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 7636e66..b6c44ad 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -139,6 +139,27 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	return 0;
 }
 
+/*
+ * IRQ handler called on GPIO edges
+ */
+static irqreturn_t dht11_handle_irq(int irq, void *data)
+{
+	struct iio_dev *iio = data;
+	struct dht11 *dht11 = iio_priv(iio);
+
+	/* TODO: Consider making the handler safe for IRQ sharing */
+	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
+		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
+		dht11->edges[dht11->num_edges++].value =
+						gpio_get_value(dht11->gpio);
+
+		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
+			complete(&dht11->completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int dht11_read_raw(struct iio_dev *iio_dev,
 			const struct iio_chan_spec *chan,
 			int *val, int *val2, long m)
@@ -159,8 +180,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		if (ret)
 			goto err_unlock;
 
+		ret = request_irq(dht11->irq, dht11_handle_irq,
+				  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				  iio_dev->name, iio_dev);
+		if (ret)
+			goto err_unlock;
+
 		ret = wait_for_completion_killable_timeout(&dht11->completion,
 								 HZ);
+
+		free_irq(dht11->irq, iio_dev);
+
 		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
 			dev_err(&iio_dev->dev,
 					"Only %d signal edges detected\n",
@@ -201,27 +231,6 @@ static const struct iio_info dht11_iio_info = {
 	.read_raw		= dht11_read_raw,
 };
 
-/*
- * IRQ handler called on GPIO edges
-*/
-static irqreturn_t dht11_handle_irq(int irq, void *data)
-{
-	struct iio_dev *iio = data;
-	struct dht11 *dht11 = iio_priv(iio);
-
-	/* TODO: Consider making the handler safe for IRQ sharing */
-	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
-		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
-		dht11->edges[dht11->num_edges++].value =
-						gpio_get_value(dht11->gpio);
-
-		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
-			complete(&dht11->completion);
-	}
-
-	return IRQ_HANDLED;
-}
-
 static const struct iio_chan_spec dht11_chan_spec[] = {
 	{ .type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
@@ -264,11 +273,6 @@ static int dht11_probe(struct platform_device *pdev)
 		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
 		return -EINVAL;
 	}
-	ret = devm_request_irq(dev, dht11->irq, dht11_handle_irq,
-				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-				pdev->name, iio);
-	if (ret)
-		return ret;
 
 	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
 	dht11->num_edges = -1;
-- 
2.1.0

Re: [PATCH 1/2] iio: dht11: Add locking

[Harald Geyer]

Hi Richard,

thanks for the patch. Comments inline.

Richard Weinberger writes:
> Protect the read function from concurrent reads.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/iio/humidity/dht11.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 623c145..7636e66 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c

#include <linux/mutex.h>

> @@ -57,6 +57,7 @@ struct dht11 {
>  	int				irq;
>  
>  	struct completion		completion;
> +	struct mutex			lock;
>  
>  	s64				timestamp;
>  	int				temperature;
> @@ -146,16 +147,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  	int ret;
>  
>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
> +		mutex_lock(&dht11->lock);

Move the locking out of the if statement.

BTW, it seems that there is already locking around read_raw() in the
in-kernel consumer interface but not in the sysfs interface. Is there
any reason for this difference?

Thanks,
Harald

>  		reinit_completion(&dht11->completion);
>  
>  		dht11->num_edges = 0;
>  		ret = gpio_direction_output(dht11->gpio, 0);
>  		if (ret)
> -			goto err;
> +			goto err_unlock;
>  		msleep(DHT11_START_TRANSMISSION);
>  		ret = gpio_direction_input(dht11->gpio);
>  		if (ret)
> -			goto err;
> +			goto err_unlock;
>  
>  		ret = wait_for_completion_killable_timeout(&dht11->completion,
>  								 HZ);
> @@ -166,14 +168,16 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  			ret = -ETIMEDOUT;
>  		}
>  		if (ret < 0)
> -			goto err;
> +			goto err_unlock;
>  
>  		ret = dht11_decode(dht11,
>  				dht11->num_edges == DHT11_EDGES_PER_READ ?
>  					DHT11_EDGES_PREAMBLE :
>  					DHT11_EDGES_PREAMBLE - 2);
> +
> +		mutex_unlock(&dht11->lock);
>  		if (ret)
> -			goto err;
> +			goto out;
>  	}
>  
>  	ret = IIO_VAL_INT;
> @@ -183,9 +187,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		*val = dht11->humidity;
>  	else
>  		ret = -EINVAL;
> -err:
> +out:
>  	dht11->num_edges = -1;
>  	return ret;
> +
> +err_unlock:
> +	mutex_unlock(&dht11->lock);
> +	goto out;
>  }
>  
>  static const struct iio_info dht11_iio_info = {
> @@ -268,6 +276,7 @@ static int dht11_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, iio);
>  
>  	init_completion(&dht11->completion);
> +	mutex_init(&dht11->lock);
>  	iio->name = pdev->name;
>  	iio->dev.parent = &pdev->dev;
>  	iio->info = &dht11_iio_info;
> -- 
> 2.1.0
> 

Re: [PATCH 2/2] iio: dht11: IRQ fixes

[Harald Geyer]

Hi Richard,

thanks for the patch.

I think (haven't tried yet) that your patch changes the number of
edges recorded per transmission. So probably the decoding function
needs to be adapted too...

I won't be able to ACK this before testing on real HW. Of course
confirmation that your changes work reliably on both DHT11 and DHT22
will do as well. The debuging code present in the initial submission
of the driver might be helpful to anybody trying to verify the
timing.

I'm doing kernel work in my free time and am quite busy with my day job
right now, so it will take a few days before I can test your code myself.
Sorry about that.

Thanks,
Harald

Richard Weinberger writes:
> We must not set an IRQ pin into output mode.
> Set the data GPIO only into IRQ mode if needed.
> This is a bit hacky but the only way to communicate in a sane
> way with the device.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/iio/humidity/dht11.c | 56 ++++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 7636e66..b6c44ad 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -139,6 +139,27 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  	return 0;
>  }
>  
> +/*
> + * IRQ handler called on GPIO edges
> + */
> +static irqreturn_t dht11_handle_irq(int irq, void *data)
> +{
> +	struct iio_dev *iio = data;
> +	struct dht11 *dht11 = iio_priv(iio);
> +
> +	/* TODO: Consider making the handler safe for IRQ sharing */
> +	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
> +		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
> +		dht11->edges[dht11->num_edges++].value =
> +						gpio_get_value(dht11->gpio);
> +
> +		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
> +			complete(&dht11->completion);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int dht11_read_raw(struct iio_dev *iio_dev,
>  			const struct iio_chan_spec *chan,
>  			int *val, int *val2, long m)
> @@ -159,8 +180,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		if (ret)
>  			goto err_unlock;
>  
> +		ret = request_irq(dht11->irq, dht11_handle_irq,
> +				  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				  iio_dev->name, iio_dev);
> +		if (ret)
> +			goto err_unlock;
> +
>  		ret = wait_for_completion_killable_timeout(&dht11->completion,
>  								 HZ);
> +
> +		free_irq(dht11->irq, iio_dev);
> +
>  		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
>  			dev_err(&iio_dev->dev,
>  					"Only %d signal edges detected\n",
> @@ -201,27 +231,6 @@ static const struct iio_info dht11_iio_info = {
>  	.read_raw		= dht11_read_raw,
>  };
>  
> -/*
> - * IRQ handler called on GPIO edges
> -*/
> -static irqreturn_t dht11_handle_irq(int irq, void *data)
> -{
> -	struct iio_dev *iio = data;
> -	struct dht11 *dht11 = iio_priv(iio);
> -
> -	/* TODO: Consider making the handler safe for IRQ sharing */
> -	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
> -		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
> -		dht11->edges[dht11->num_edges++].value =
> -						gpio_get_value(dht11->gpio);
> -
> -		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
> -			complete(&dht11->completion);
> -	}
> -
> -	return IRQ_HANDLED;
> -}
> -
>  static const struct iio_chan_spec dht11_chan_spec[] = {
>  	{ .type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
> @@ -264,11 +273,6 @@ static int dht11_probe(struct platform_device *pdev)
>  		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
>  		return -EINVAL;
>  	}
> -	ret = devm_request_irq(dev, dht11->irq, dht11_handle_irq,
> -				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> -				pdev->name, iio);
> -	if (ret)
> -		return ret;
>  
>  	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
>  	dht11->num_edges = -1;
> -- 
> 2.1.0
> 

Re: [PATCH 1/2] iio: dht11: Add locking

[Richard Weinberger]

Harald,

Am 02.12.2014 um 11:07 schrieb Harald Geyer:
> Hi Richard,
> 
> thanks for the patch. Comments inline.
> 
> Richard Weinberger writes:
>> Protect the read function from concurrent reads.
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  drivers/iio/humidity/dht11.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>> index 623c145..7636e66 100644
>> --- a/drivers/iio/humidity/dht11.c
>> +++ b/drivers/iio/humidity/dht11.c
> 
> #include <linux/mutex.h>
> 
>> @@ -57,6 +57,7 @@ struct dht11 {
>>  	int				irq;
>>  
>>  	struct completion		completion;
>> +	struct mutex			lock;
>>  
>>  	s64				timestamp;
>>  	int				temperature;
>> @@ -146,16 +147,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>>  	int ret;
>>  
>>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>> +		mutex_lock(&dht11->lock);
> 
> Move the locking out of the if statement.

Care to explain why?

But I found another issue in my patch.
The "dht11->num_edges = -1;" before "return ret" needs to go into the locked area.
Will send an updated version soon.

> BTW, it seems that there is already locking around read_raw() in the
> in-kernel consumer interface but not in the sysfs interface. Is there
> any reason for this difference?

Dunno. :-)

Thanks,
//richard

Re: [PATCH 2/2] iio: dht11: IRQ fixes

[Richard Weinberger]

Harald,

Am 02.12.2014 um 11:19 schrieb Harald Geyer:
> Hi Richard,
> 
> thanks for the patch.
> 
> I think (haven't tried yet) that your patch changes the number of
> edges recorded per transmission. So probably the decoding function
> needs to be adapted too...

Can you explain your thought?
I'll happily dig into that.

> I won't be able to ACK this before testing on real HW. Of course
> confirmation that your changes work reliably on both DHT11 and DHT22
> will do as well. The debuging code present in the initial submission
> of the driver might be helpful to anybody trying to verify the
> timing.

I have only DHT22 sensors. Of course I've tested the driver with these.

> I'm doing kernel work in my free time and am quite busy with my day job
> right now, so it will take a few days before I can test your code myself.
> Sorry about that.

No need to worry!

Thanks,
//richard

Re: [PATCH 1/2] iio: dht11: Add locking

[Harald Geyer]

Richard Weinberger writes:
> Harald,
> 
> Am 02.12.2014 um 11:07 schrieb Harald Geyer:
> > Hi Richard,
> > 
> > thanks for the patch. Comments inline.
> > 
> > Richard Weinberger writes:
> >> Protect the read function from concurrent reads.
> >>
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >> ---
> >>  drivers/iio/humidity/dht11.c | 19 ++++++++++++++-----
> >>  1 file changed, 14 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> >> index 623c145..7636e66 100644
> >> --- a/drivers/iio/humidity/dht11.c
> >> +++ b/drivers/iio/humidity/dht11.c
> > 
> > #include <linux/mutex.h>
> > 
> >> @@ -57,6 +57,7 @@ struct dht11 {
> >>  	int				irq;
> >>  
> >>  	struct completion		completion;
> >> +	struct mutex			lock;
> >>  
> >>  	s64				timestamp;
> >>  	int				temperature;
> >> @@ -146,16 +147,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> >>  	int ret;
> >>  
> >>  	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
> >> +		mutex_lock(&dht11->lock);
> > 
> > Move the locking out of the if statement.
> 
> Care to explain why?

The purpose of the if statement is to limit the number of data
transmissions if values are requested multiple times in short
succession. (Ie an application reading both sensor values.)

If we have concurrent reads, then the later one will wait in the
mutex_lock() while the former will update the timestamp. If the
later one resumes, it won't check the timestamp and cause an
unnecessary data transmission.
 
> But I found another issue in my patch.
> The "dht11->num_edges = -1;" before "return ret" needs to go into the locked area.
> Will send an updated version soon.
> 
> > BTW, it seems that there is already locking around read_raw() in the
> > in-kernel consumer interface but not in the sysfs interface. Is there
> > any reason for this difference?
> 
> Dunno. :-)

If locking is actually necessary, then this would be a bug in the
current version of the driver, which wasn't caught by at least three
people doing reviews, so maybe let's find out if it really is necessary
or if I'm missing something ...

Harald

Re: [PATCH 2/2] iio: dht11: IRQ fixes

[Harald Geyer]

Richard Weinberger writes:
> Harald,
> 
> Am 02.12.2014 um 11:19 schrieb Harald Geyer:
> > Hi Richard,
> > 
> > thanks for the patch.
> > 
> > I think (haven't tried yet) that your patch changes the number of
> > edges recorded per transmission. So probably the decoding function
> > needs to be adapted too...
> 
> Can you explain your thought?
> I'll happily dig into that.

Part of the reason to have the IRQ enabled during output was to
get consistent timestamps (all timestamps would be taken via the
same codepath). While this isn't essential, the current code expects
that the start command we generate is in the list of edges. I think
your changes will cause that start command not to be in the list of
edges.

Note: There have been issues with some sensors (DHT11s?) not to
send proper start bits, so the driver tries to be liberal about
what it expects and you probably haven't seen any decoding errors
if your sample sensor works to specification. (Now that I come to
think about it: Undefined behaviour of IRQs on output lines could
have been involved too...)

This is a bit of a mess and I'd rather clean this up than add more
to it. Maybe I'm overly fussy about this, but it has bitten me in
the past.

Since it seems you have some interesst in working on these parts,
let me mention an unrelated issue: The DHT22 stops sending data
after a random time (think of days here) which AFAIK only can be
worked around by power-cycling the sensor. I mean to add something
for this to the driver but couln't make up my mind about what the
proper ABI for this would be, so right now I'm using some userspace
hack for this. (The issue was already discussed on the linux-iio
mailing list a few month ago, if you want to look into this.
Anyway: You have been warned ... ;)  
 
> > I won't be able to ACK this before testing on real HW. Of course
> > confirmation that your changes work reliably on both DHT11 and DHT22
> > will do as well. The debuging code present in the initial submission
> > of the driver might be helpful to anybody trying to verify the
> > timing.
> 
> I have only DHT22 sensors. Of course I've tested the driver with these.

Ok, so I'll test DHT11 first. It would still be nice to confirm how
many edges are recorded from DHT22 though.

Thanks,
Harald

Re: [PATCH 1/2] iio: dht11: Add locking

[Richard Weinberger]

Harald,

Am 02.12.2014 um 13:14 schrieb Harald Geyer:
>>> Move the locking out of the if statement.
>>
>> Care to explain why?
> 
> The purpose of the if statement is to limit the number of data
> transmissions if values are requested multiple times in short
> succession. (Ie an application reading both sensor values.)
> 
> If we have concurrent reads, then the later one will wait in the
> mutex_lock() while the former will update the timestamp. If the
> later one resumes, it won't check the timestamp and cause an
> unnecessary data transmission.

Okay, makes sense.
I'll update my patch!

>  
>> But I found another issue in my patch.
>> The "dht11->num_edges = -1;" before "return ret" needs to go into the locked area.
>> Will send an updated version soon.
>>
>>> BTW, it seems that there is already locking around read_raw() in the
>>> in-kernel consumer interface but not in the sysfs interface. Is there
>>> any reason for this difference?
>>
>> Dunno. :-)
> 
> If locking is actually necessary, then this would be a bug in the
> current version of the driver, which wasn't caught by at least three
> people doing reviews, so maybe let's find out if it really is necessary
> or if I'm missing something ...

Maybe IIO folks can tell us more.
What I see in other IIO drivers is that they all have locking in the read functions
and so far I see no global serialization in IIO itself.

Thanks,
//richard

Re: [PATCH 2/2] iio: dht11: IRQ fixes

[Richard Weinberger]

Harald,

Am 02.12.2014 um 13:58 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Harald,
>>
>> Am 02.12.2014 um 11:19 schrieb Harald Geyer:
>>> Hi Richard,
>>>
>>> thanks for the patch.
>>>
>>> I think (haven't tried yet) that your patch changes the number of
>>> edges recorded per transmission. So probably the decoding function
>>> needs to be adapted too...
>>
>> Can you explain your thought?
>> I'll happily dig into that.
> 
> Part of the reason to have the IRQ enabled during output was to
> get consistent timestamps (all timestamps would be taken via the
> same codepath). While this isn't essential, the current code expects
> that the start command we generate is in the list of edges. I think
> your changes will cause that start command not to be in the list of
> edges.

Yeah, if the sensor starts transmitting data before we've setup
the IRQ we'll lose edges.
But AFAICT the DHT is much slower than we are with setting up the IRQ.
The DHT is a rather stupid device so we cannot use proper interrupting.
But I can think of adding also polling support to the driver.
Such that one can select whether she wants to use IRQ or polling...

> Note: There have been issues with some sensors (DHT11s?) not to
> send proper start bits, so the driver tries to be liberal about
> what it expects and you probably haven't seen any decoding errors
> if your sample sensor works to specification. (Now that I come to
> think about it: Undefined behaviour of IRQs on output lines could
> have been involved too...)
> 
> This is a bit of a mess and I'd rather clean this up than add more
> to it. Maybe I'm overly fussy about this, but it has bitten me in
> the past.

I can feel your pain. I've wasted some hours with half baked
user space drivers for the DHT22.

> Since it seems you have some interesst in working on these parts,
> let me mention an unrelated issue: The DHT22 stops sending data
> after a random time (think of days here) which AFAIK only can be
> worked around by power-cycling the sensor. I mean to add something
> for this to the driver but couln't make up my mind about what the
> proper ABI for this would be, so right now I'm using some userspace
> hack for this. (The issue was already discussed on the linux-iio
> mailing list a few month ago, if you want to look into this.
> Anyway: You have been warned ... ;)  

Oh, that's a very valuable information!
Currently I'm evaluating some sensors for a private project.
You can recommend a better temp/humidity sensor?

>>> I won't be able to ACK this before testing on real HW. Of course
>>> confirmation that your changes work reliably on both DHT11 and DHT22
>>> will do as well. The debuging code present in the initial submission
>>> of the driver might be helpful to anybody trying to verify the
>>> timing.
>>
>> I have only DHT22 sensors. Of course I've tested the driver with these.
> 
> Ok, so I'll test DHT11 first. It would still be nice to confirm how
> many edges are recorded from DHT22 though.

Of course, I'll run some tests later.

Thanks,
//richard

Re: [PATCH 2/2] iio: dht11: IRQ fixes

[Harald Geyer]

Richard Weinberger writes:
> Harald,
> 
> Am 02.12.2014 um 13:58 schrieb Harald Geyer:
> > Richard Weinberger writes:
> >> Harald,
> >>
> >> Am 02.12.2014 um 11:19 schrieb Harald Geyer:
> >>> Hi Richard,
> >>>
> >>> thanks for the patch.
> >>>
> >>> I think (haven't tried yet) that your patch changes the number of
> >>> edges recorded per transmission. So probably the decoding function
> >>> needs to be adapted too...
> >>
> >> Can you explain your thought?
> >> I'll happily dig into that.
> > 
> > Part of the reason to have the IRQ enabled during output was to
> > get consistent timestamps (all timestamps would be taken via the
> > same codepath). While this isn't essential, the current code expects
> > that the start command we generate is in the list of edges. I think
> > your changes will cause that start command not to be in the list of
> > edges.
> 
> Yeah, if the sensor starts transmitting data before we've setup
> the IRQ we'll lose edges.

Yes, but you missunderstood me. That's not the point.

The point is that now the edges that the driver generates while
the gpio is configured as output are part of the preamble of the
data transmission. I think with your patch applied this is no longer
the case (we don't get interrupts for these edges anymore). So the
decoding needs to be changed to work with a shorter preamble.

> But AFAICT the DHT is much slower than we are with setting up the IRQ.
> The DHT is a rather stupid device so we cannot use proper interrupting.
> But I can think of adding also polling support to the driver.
> Such that one can select whether she wants to use IRQ or polling...

I don't like the idea of a polling driver. Also I don't think it will
be necessary.

> > Since it seems you have some interesst in working on these parts,
> > let me mention an unrelated issue: The DHT22 stops sending data
> > after a random time (think of days here) which AFAIK only can be
> > worked around by power-cycling the sensor. I mean to add something
> > for this to the driver but couln't make up my mind about what the
> > proper ABI for this would be, so right now I'm using some userspace
> > hack for this. (The issue was already discussed on the linux-iio
> > mailing list a few month ago, if you want to look into this.
> > Anyway: You have been warned ... ;)  
> 
> Oh, that's a very valuable information!
> Currently I'm evaluating some sensors for a private project.
> You can recommend a better temp/humidity sensor?

No really. When I noticed these cheap little parts are actually
*cheap* parts, I looked which sensors are already supported by
hwmon, but none appealed to me, so I decided to try and work
around this in software as far as possible. It's not hard to do, just
hard to do right. ;)

The best I can recommend you is to have a look at the list of
humidity sensors supported by hwmon yourself.
 
Thanks,
Harald

Re: [PATCH 2/2] iio: dht11: IRQ fixes

[Richard Weinberger]

Harald,

Am 02.12.2014 um 20:49 schrieb Harald Geyer:
>> Yeah, if the sensor starts transmitting data before we've setup
>> the IRQ we'll lose edges.
> 
> Yes, but you missunderstood me. That's not the point.
> 
> The point is that now the edges that the driver generates while
> the gpio is configured as output are part of the preamble of the
> data transmission. I think with your patch applied this is no longer
> the case (we don't get interrupts for these edges anymore). So the
> decoding needs to be changed to work with a shorter preamble.

Hmm, I get always 84 edges.
This value makes sense to me, 40 bits (80 edges) plus 4 preamble edges.

>> But AFAICT the DHT is much slower than we are with setting up the IRQ.
>> The DHT is a rather stupid device so we cannot use proper interrupting.
>> But I can think of adding also polling support to the driver.
>> Such that one can select whether she wants to use IRQ or polling...
> 
> I don't like the idea of a polling driver. Also I don't think it will
> be necessary.

Of course polling is ugly, if IRQ works let's keep it as is. :)

>>> Since it seems you have some interesst in working on these parts,
>>> let me mention an unrelated issue: The DHT22 stops sending data
>>> after a random time (think of days here) which AFAIK only can be
>>> worked around by power-cycling the sensor. I mean to add something
>>> for this to the driver but couln't make up my mind about what the
>>> proper ABI for this would be, so right now I'm using some userspace
>>> hack for this. (The issue was already discussed on the linux-iio
>>> mailing list a few month ago, if you want to look into this.
>>> Anyway: You have been warned ... ;)  
>>
>> Oh, that's a very valuable information!
>> Currently I'm evaluating some sensors for a private project.
>> You can recommend a better temp/humidity sensor?
> 
> No really. When I noticed these cheap little parts are actually
> *cheap* parts, I looked which sensors are already supported by
> hwmon, but none appealed to me, so I decided to try and work
> around this in software as far as possible. It's not hard to do, just
> hard to do right. ;)
> 
> The best I can recommend you is to have a look at the list of
> humidity sensors supported by hwmon yourself.

Ok!

Thanks,
//richard

Re: [PATCH 2/2] iio: dht11: IRQ fixes

[Hartmut Knaack]

Richard Weinberger schrieb am 02.12.2014 um 19:12:
> Oh, that's a very valuable information! Currently I'm evaluating some sensors for a private project. You can recommend a better temp/humidity sensor? 
Si7013 and Si7021 look very promising to me, regarding accuracy. Both are supported by driver si7020.