Please see my current patches for your driver.
As discussed in an earlier mail I'm testing with the DHT22 sensor only.
With the IRQ changes I see 84 edges.
I have also a question on your driver. Why you increment
DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
threshold = DHT11_DATA_BIT_HIGH / timeres;
if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
pr_err("dht11: WARNING: decoding ambiguous\n");
Thanks,
//richard
[PATCH 1/4] iio: dht11: Add locking
[PATCH 2/4] iio: dht11: IRQ fixes
[PATCH 3/4] iio: dht11: Logging updates
[PATCH 4/4] iio: dht11: Fix out-of-bounds read
Make sure that the read function is not interrupted...
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/iio/humidity/dht11.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index d8771f5..168ebc4 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -29,6 +29,7 @@
#include <linux/wait.h>
#include <linux/bitops.h>
#include <linux/completion.h>
+#include <linux/mutex.h>
#include <linux/delay.h>
#include <linux/gpio.h>
#include <linux/of_gpio.h>
@@ -57,6 +58,7 @@ struct dht11 {
int irq;
struct completion completion;
+ struct mutex lock;
s64 timestamp;
int temperature;
@@ -145,6 +147,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
struct dht11 *dht11 = iio_priv(iio_dev);
int ret;
+ mutex_lock(&dht11->lock);
if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
reinit_completion(&dht11->completion);
@@ -185,6 +188,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
ret = -EINVAL;
err:
dht11->num_edges = -1;
+ mutex_unlock(&dht11->lock);
return ret;
}
@@ -268,6 +272,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;
--
1.8.4.5
As we access i-1 we must not start with i=0.
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/iio/humidity/dht11.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index fbcd7cb..5dfe71b 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -90,7 +90,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
/* Calculate timestamp resolution */
- for (i = 0; i < dht11->num_edges; ++i) {
+ for (i = 1; i < dht11->num_edges; ++i) {
t = dht11->edges[i].ts - dht11->edges[i-1].ts;
if (t > 0 && t < timeres)
timeres = t;
--
1.8.4.5
Currently the driver uses pr_* and dev_* functions.
Change all logging functions to dev_* style to be consistent
and have the correct device prefix in all messages.
This change set also adds new messages to diagnose issues.
Signed-off-by: Richard Weinberger <[email protected]>
---
drivers/iio/humidity/dht11.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 0023699..fbcd7cb 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
timeres = t;
}
if (2*timeres > DHT11_DATA_BIT_HIGH) {
- pr_err("dht11: timeresolution %d too bad for decoding\n",
+ dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
timeres);
return -EIO;
}
threshold = DHT11_DATA_BIT_HIGH / timeres;
if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
- pr_err("dht11: WARNING: decoding ambiguous\n");
+ dev_err(dht11->dev, "decoding ambiguous\n");
/* scale down with timeres and check validity */
for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
t = dht11->edges[offset + 2*i + 2].ts -
dht11->edges[offset + 2*i + 1].ts;
- if (!dht11->edges[offset + 2*i + 1].value)
- return -EIO; /* lost synchronisation */
+ if (!dht11->edges[offset + 2*i + 1].value) {
+ dev_err(dht11->dev, "lost synchronisation\n");
+ return -EIO;
+ }
timing[i] = t / timeres;
}
@@ -119,8 +121,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
temp_dec = dht11_decode_byte(&timing[24], threshold);
checksum = dht11_decode_byte(&timing[32], threshold);
- if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
+ if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
+ dev_err(dht11->dev, "invalid checksum\n");
return -EIO;
+ }
dht11->timestamp = iio_get_time_ns();
if (hum_int < 20) { /* DHT22 */
@@ -193,7 +197,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
free_irq(dht11->irq, iio_dev);
if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
- dev_err(&iio_dev->dev,
+ dev_err(dht11->dev,
"Only %d signal edges detected\n",
dht11->num_edges);
ret = -ETIMEDOUT;
--
1.8.4.5
Signed-off-by: Richard Weinberger <[email protected]>
---
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 168ebc4..0023699 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -140,6 +140,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)
@@ -160,8 +181,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
if (ret)
goto err;
+ ret = request_irq(dht11->irq, dht11_handle_irq,
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+ iio_dev->name, iio_dev);
+ if (ret)
+ goto err;
+
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",
@@ -197,27 +227,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), },
@@ -260,11 +269,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;
--
1.8.4.5
Am 03.12.2014 um 00:32 schrieb Richard Weinberger:
<insert commit message here> :-\
> Signed-off-by: Richard Weinberger <[email protected]>
Hi Richard,
thanks for all the work you put into this!
Richard Weinberger writes:
> Please see my current patches for your driver.
> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
> With the IRQ changes I see 84 edges.
This still surprises me. With the IRQ changes I would expect the
preamble to be 2 edges only. I must be missing something. Can you
explain this to me?
I'll look into this as soon as I've time.
> I have also a question on your driver. Why you increment
> DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
>
> threshold = DHT11_DATA_BIT_HIGH / timeres;
> if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
> pr_err("dht11: WARNING: decoding ambiguous\n");
This is to take ambiguity of when the bit started relativ to the
clock ticks into account. For example with common 32kHz clocks:
DHT11_DATA_BIT_LOW / timeres = 0
DHT11_DATA_BIT_HIGH / timeres = 2
but since the bit might not start at a clock tick the actual t of
a low bit can be either 0 or 1 while the actual t of a high bit
can be either 2 or 3.
This case is fine.
But if we had a 38kHz clock:
DHT11_DATA_BIT_LOW / timeres = 1 t can be 1 or 2
DHT11_DATA_BIT_HIGH / timeres = 2 t can be 2 or 3
so we have an ambiguity. The ambiguity could be removed by a smarter
decoder, that looks at the t of other bits, but I'm not going to do
that unless somebody is promising to test it on affected hardware.
Feel free to add some comment about this to the code.
> [PATCH 1/4] iio: dht11: Add locking
> [PATCH 2/4] iio: dht11: IRQ fixes
> [PATCH 3/4] iio: dht11: Logging updates
> [PATCH 4/4] iio: dht11: Fix out-of-bounds read
You can add my Acked-by or Reviewed-by to 1/4 and 4/4 if you want to.
Maybe Jonathan wants the patches reordered so that fixes come first, but
then 4/4 should not cause any problems in practice, so perhaps it doesn't
matter.
I really have to understand this preamble length thing on 2/4 and I will
reply to 3/4 separately.
Thanks,
Harald
Richard Weinberger writes:
> Currently the driver uses pr_* and dev_* functions.
> Change all logging functions to dev_* style to be consistent
> and have the correct device prefix in all messages.
Yes, actually this was on purpose:
The dev_ messages are really about something wrong with the device.
Ie if something goes wrong with one device but could perfectly work
with some other device.
The pr_ messages OTOH are about something wrong with clock resolution,
etc that would affect any DHT11 sensor on the system. Ideally we would
notice these things during probe() and just return with an error there.
Right now we aren't as clever as that, so we just log an error message
about the driver, when actually we are reading the device.
That said, I don't have strong feelings about this. If you want to
change this, I won't object. However if you really want to fix this,
then the proper thing would be to check for this conditions in
probe().
> This change set also adds new messages to diagnose issues.
Comment below.
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> drivers/iio/humidity/dht11.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 0023699..fbcd7cb 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> timeres = t;
> }
> if (2*timeres > DHT11_DATA_BIT_HIGH) {
> - pr_err("dht11: timeresolution %d too bad for decoding\n",
> + dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
> timeres);
> return -EIO;
> }
> threshold = DHT11_DATA_BIT_HIGH / timeres;
> if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
> - pr_err("dht11: WARNING: decoding ambiguous\n");
> + dev_err(dht11->dev, "decoding ambiguous\n");
>
> /* scale down with timeres and check validity */
> for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
> t = dht11->edges[offset + 2*i + 2].ts -
> dht11->edges[offset + 2*i + 1].ts;
> - if (!dht11->edges[offset + 2*i + 1].value)
> - return -EIO; /* lost synchronisation */
> + if (!dht11->edges[offset + 2*i + 1].value) {
> + dev_err(dht11->dev, "lost synchronisation\n");
> + return -EIO;
> + }
Are you sure this warrants a log message? I don't think this provides
much information. The userspace application should just try reading
the sensor again.
We could do someting smart and try to recover from such errors, but
ultimately userspace will need to deal with failed sensor communication
anyway, so I don't see the point.
> timing[i] = t / timeres;
> }
>
> @@ -119,8 +121,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> temp_dec = dht11_decode_byte(&timing[24], threshold);
> checksum = dht11_decode_byte(&timing[32], threshold);
>
> - if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
> + if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
> + dev_err(dht11->dev, "invalid checksum\n");
> return -EIO;
> + }
Same thing here.
> dht11->timestamp = iio_get_time_ns();
> if (hum_int < 20) { /* DHT22 */
> @@ -193,7 +197,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> free_irq(dht11->irq, iio_dev);
>
> if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
> - dev_err(&iio_dev->dev,
> + dev_err(dht11->dev,
Ack.
Thanks,
Harald
> "Only %d signal edges detected\n",
> dht11->num_edges);
> ret = -ETIMEDOUT;
> --
> 1.8.4.5
>
Am 03.12.2014 um 13:58 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Currently the driver uses pr_* and dev_* functions.
>> Change all logging functions to dev_* style to be consistent
>> and have the correct device prefix in all messages.
>
> Yes, actually this was on purpose:
> The dev_ messages are really about something wrong with the device.
> Ie if something goes wrong with one device but could perfectly work
> with some other device.
> The pr_ messages OTOH are about something wrong with clock resolution,
> etc that would affect any DHT11 sensor on the system. Ideally we would
> notice these things during probe() and just return with an error there.
> Right now we aren't as clever as that, so we just log an error message
> about the driver, when actually we are reading the device.
>
> That said, I don't have strong feelings about this. If you want to
> change this, I won't object. However if you really want to fix this,
> then the proper thing would be to check for this conditions in
> probe().
Currently we get log messages of style:
"iio iio:deviceX: foo bar"
and:
"dht11 <name in DT>: foo bar"
I really favorite "dht11 <name in DT".
In my device tree every senor has a sane name and log messages look like:
"dht11 toiletten sensor: invalid checksum"
>> This change set also adds new messages to diagnose issues.
>
> Comment below.
>
>> Signed-off-by: Richard Weinberger <[email protected]>
>> ---
>> drivers/iio/humidity/dht11.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>> index 0023699..fbcd7cb 100644
>> --- a/drivers/iio/humidity/dht11.c
>> +++ b/drivers/iio/humidity/dht11.c
>> @@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>> timeres = t;
>> }
>> if (2*timeres > DHT11_DATA_BIT_HIGH) {
>> - pr_err("dht11: timeresolution %d too bad for decoding\n",
>> + dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
>> timeres);
>> return -EIO;
>> }
>> threshold = DHT11_DATA_BIT_HIGH / timeres;
>> if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>> - pr_err("dht11: WARNING: decoding ambiguous\n");
>> + dev_err(dht11->dev, "decoding ambiguous\n");
>>
>> /* scale down with timeres and check validity */
>> for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
>> t = dht11->edges[offset + 2*i + 2].ts -
>> dht11->edges[offset + 2*i + 1].ts;
>> - if (!dht11->edges[offset + 2*i + 1].value)
>> - return -EIO; /* lost synchronisation */
>> + if (!dht11->edges[offset + 2*i + 1].value) {
>> + dev_err(dht11->dev, "lost synchronisation\n");
>> + return -EIO;
>> + }
>
> Are you sure this warrants a log message? I don't think this provides
> much information. The userspace application should just try reading
> the sensor again.
>
> We could do someting smart and try to recover from such errors, but
> ultimately userspace will need to deal with failed sensor communication
> anyway, so I don't see the point.
The sensors are rather flaky and if they go nuts the user/developer maybe wants
to know why.
>From a plain EIO she has no chance to find out. He'll have to add printk()s by hand
to find out.
With a log message one can start digging into the issue.
>> timing[i] = t / timeres;
>> }
>>
>> @@ -119,8 +121,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>> temp_dec = dht11_decode_byte(&timing[24], threshold);
>> checksum = dht11_decode_byte(&timing[32], threshold);
>>
>> - if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>> + if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
>> + dev_err(dht11->dev, "invalid checksum\n");
>> return -EIO;
>> + }
>
> Same thing here.
Same here.
I had some wiring issues with my sensors and wanted to know from where the EIO comes.
Thanks,
//richard
Harald,
Am 03.12.2014 um 13:18 schrieb Harald Geyer:
> Hi Richard,
>
> thanks for all the work you put into this!
>
> Richard Weinberger writes:
>> Please see my current patches for your driver.
>> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
>> With the IRQ changes I see 84 edges.
>
> This still surprises me. With the IRQ changes I would expect the
> preamble to be 2 edges only. I must be missing something. Can you
> explain this to me?
I'll try to find out!
> I'll look into this as soon as I've time.
>
>> I have also a question on your driver. Why you increment
>> DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
>>
>> threshold = DHT11_DATA_BIT_HIGH / timeres;
>> if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>> pr_err("dht11: WARNING: decoding ambiguous\n");
>
> This is to take ambiguity of when the bit started relativ to the
> clock ticks into account. For example with common 32kHz clocks:
> DHT11_DATA_BIT_LOW / timeres = 0
> DHT11_DATA_BIT_HIGH / timeres = 2
> but since the bit might not start at a clock tick the actual t of
> a low bit can be either 0 or 1 while the actual t of a high bit
> can be either 2 or 3.
>
> This case is fine.
>
> But if we had a 38kHz clock:
> DHT11_DATA_BIT_LOW / timeres = 1 t can be 1 or 2
> DHT11_DATA_BIT_HIGH / timeres = 2 t can be 2 or 3
> so we have an ambiguity. The ambiguity could be removed by a smarter
> decoder, that looks at the t of other bits, but I'm not going to do
> that unless somebody is promising to test it on affected hardware.
>
> Feel free to add some comment about this to the code.
Will do, thanks a lot for the explanation.
I was asking because I see the "dht11: WARNING: decoding ambiguous"
very often. (with and without my patches)
>> [PATCH 1/4] iio: dht11: Add locking
>> [PATCH 2/4] iio: dht11: IRQ fixes
>> [PATCH 3/4] iio: dht11: Logging updates
>> [PATCH 4/4] iio: dht11: Fix out-of-bounds read
>
> You can add my Acked-by or Reviewed-by to 1/4 and 4/4 if you want to.
> Maybe Jonathan wants the patches reordered so that fixes come first, but
> then 4/4 should not cause any problems in practice, so perhaps it doesn't
> matter.
>
> I really have to understand this preamble length thing on 2/4 and I will
> reply to 3/4 separately.
Thanks a lot!
//richard
Richard Weinberger writes:
> Am 03.12.2014 um 13:58 schrieb Harald Geyer:
> > Richard Weinberger writes:
> >> Currently the driver uses pr_* and dev_* functions.
> >> Change all logging functions to dev_* style to be consistent
> >> and have the correct device prefix in all messages.
> >
> > Yes, actually this was on purpose:
> > The dev_ messages are really about something wrong with the device.
> > Ie if something goes wrong with one device but could perfectly work
> > with some other device.
> > The pr_ messages OTOH are about something wrong with clock resolution,
> > etc that would affect any DHT11 sensor on the system. Ideally we would
> > notice these things during probe() and just return with an error there.
> > Right now we aren't as clever as that, so we just log an error message
> > about the driver, when actually we are reading the device.
> >
> > That said, I don't have strong feelings about this. If you want to
> > change this, I won't object. However if you really want to fix this,
> > then the proper thing would be to check for this conditions in
> > probe().
>
> Currently we get log messages of style:
> "iio iio:deviceX: foo bar"
> and:
> "dht11 <name in DT>: foo bar"
>
> I really favorite "dht11 <name in DT".
> In my device tree every senor has a sane name and log messages look like:
> "dht11 toiletten sensor: invalid checksum"
I think I ACKed the one with "iio iio:deviceX: foo bar"?
> >> This change set also adds new messages to diagnose issues.
> >
> > Comment below.
> >
> >> Signed-off-by: Richard Weinberger <[email protected]>
> >> ---
> >> drivers/iio/humidity/dht11.c | 16 ++++++++++------
> >> 1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> >> index 0023699..fbcd7cb 100644
> >> --- a/drivers/iio/humidity/dht11.c
> >> +++ b/drivers/iio/humidity/dht11.c
> >> @@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> >> timeres = t;
> >> }
> >> if (2*timeres > DHT11_DATA_BIT_HIGH) {
> >> - pr_err("dht11: timeresolution %d too bad for decoding\n",
> >> + dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
> >> timeres);
> >> return -EIO;
> >> }
> >> threshold = DHT11_DATA_BIT_HIGH / timeres;
> >> if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
> >> - pr_err("dht11: WARNING: decoding ambiguous\n");
> >> + dev_err(dht11->dev, "decoding ambiguous\n");
> >>
> >> /* scale down with timeres and check validity */
> >> for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
> >> t = dht11->edges[offset + 2*i + 2].ts -
> >> dht11->edges[offset + 2*i + 1].ts;
> >> - if (!dht11->edges[offset + 2*i + 1].value)
> >> - return -EIO; /* lost synchronisation */
> >> + if (!dht11->edges[offset + 2*i + 1].value) {
> >> + dev_err(dht11->dev, "lost synchronisation\n");
> >> + return -EIO;
> >> + }
> >
> > Are you sure this warrants a log message? I don't think this provides
> > much information. The userspace application should just try reading
> > the sensor again.
> >
> > We could do someting smart and try to recover from such errors, but
> > ultimately userspace will need to deal with failed sensor communication
> > anyway, so I don't see the point.
>
> The sensors are rather flaky and if they go nuts the user/developer
> maybe wants to know why.
> From a plain EIO she has no chance to find out. He'll have to add
> printk()s by hand to find out.
> With a log message one can start digging into the issue.
I think any log messages that are caused by wiring problems are
in place already. Also you should get an ETIMEDOUT not an EIO in
these cases. In these cases the number of edges received is reported,
which actually tells you a lot about the type of wiring problem.
BTW when I originally submitted the driver, it contained the following
code that got rejected:
/*
* dht11_edges_print: show the data as actually received by the
* driver.
* This is dead code, keeping it for now just in case somebody needs
* it for porting the driver to new sensor HW, etc.
*
static void dht11_edges_print(struct dht11_gpio *dht11)
{
int i;
pr_err("dht11: transfer timed out:\n");
for (i = 1; i < dht11->num_edges; ++i) {
pr_err("dht11: %d: %lld ns %s\n", i,
dht11->edges[i].ts - dht11->edges[i-1].ts,
dht11->edges[i-1].value ? "high" : "low");
}
}
*/
which I used to call on errors to debug the decoder and the quirks
necessary to support different sensors. Maybe someting like this would
get accepted if it integrated properly with the debugging frameworks
of the kernel instead of flooding the log?
I think the EIO errors are all really of the type:
"Some noise injection into the data line caused an hiccup, just try again."
I wouldn't object to changing some of the EIOs to EAGAINs or something,
but I doubt we get this through review.
JFTR: I don't object strongly to additional logging. Just telling you
1) This has been considered when writing the driver.
2) I probably won't ACK it, so Jonathan will have to decide wether this
is a valuable addition.
Thanks,
Harald
> >> timing[i] = t / timeres;
> >> }
> >>
> >> @@ -119,8 +121,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> >> temp_dec = dht11_decode_byte(&timing[24], threshold);
> >> checksum = dht11_decode_byte(&timing[32], threshold);
> >>
> >> - if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
> >> + if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
> >> + dev_err(dht11->dev, "invalid checksum\n");
> >> return -EIO;
> >> + }
> >
> > Same thing here.
>
> Same here.
> I had some wiring issues with my sensors and wanted to know from where the EIO comes.
>
> Thanks,
> //richard
Richard Weinberger writes:
> Harald,
>
> Am 03.12.2014 um 13:18 schrieb Harald Geyer:
> > Hi Richard,
> >
> > thanks for all the work you put into this!
> >
> > Richard Weinberger writes:
> >> I have also a question on your driver. Why you increment
> >> DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
> >>
> >> threshold = DHT11_DATA_BIT_HIGH / timeres;
> >> if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
> >> pr_err("dht11: WARNING: decoding ambiguous\n");
> >
> > This is to take ambiguity of when the bit started relativ to the
> > clock ticks into account. For example with common 32kHz clocks:
> > DHT11_DATA_BIT_LOW / timeres = 0
> > DHT11_DATA_BIT_HIGH / timeres = 2
> > but since the bit might not start at a clock tick the actual t of
> > a low bit can be either 0 or 1 while the actual t of a high bit
> > can be either 2 or 3.
> >
> > This case is fine.
> >
> > But if we had a 38kHz clock:
> > DHT11_DATA_BIT_LOW / timeres = 1 t can be 1 or 2
> > DHT11_DATA_BIT_HIGH / timeres = 2 t can be 2 or 3
> > so we have an ambiguity. The ambiguity could be removed by a smarter
> > decoder, that looks at the t of other bits, but I'm not going to do
> > that unless somebody is promising to test it on affected hardware.
> >
> > Feel free to add some comment about this to the code.
>
> Will do, thanks a lot for the explanation.
>
> I was asking because I see the "dht11: WARNING: decoding ambiguous"
> very often. (with and without my patches)
Yes, your patches shouldn't have any effect on this.
"very often" in the sense of "not always"? This would be very surprising,
because this would involve variable length clock ticks, i think.
I guess we should include timeres into the warning message.
Also I guess now is the time to think about a smarter decoder.
Thanks a lot for your effort.
Harald
Am 03.12.2014 um 15:08 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Harald,
>>
>> Am 03.12.2014 um 13:18 schrieb Harald Geyer:
>>> Hi Richard,
>>>
>>> thanks for all the work you put into this!
>>>
>>> Richard Weinberger writes:
>>>> I have also a question on your driver. Why you increment
>>>> DHT11_DATA_BIT_LOW/timeres by one in the ambiguity check?
>>>>
>>>> threshold = DHT11_DATA_BIT_HIGH / timeres;
>>>> if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>>>> pr_err("dht11: WARNING: decoding ambiguous\n");
>>>
>>> This is to take ambiguity of when the bit started relativ to the
>>> clock ticks into account. For example with common 32kHz clocks:
>>> DHT11_DATA_BIT_LOW / timeres = 0
>>> DHT11_DATA_BIT_HIGH / timeres = 2
>>> but since the bit might not start at a clock tick the actual t of
>>> a low bit can be either 0 or 1 while the actual t of a high bit
>>> can be either 2 or 3.
>>>
>>> This case is fine.
>>>
>>> But if we had a 38kHz clock:
>>> DHT11_DATA_BIT_LOW / timeres = 1 t can be 1 or 2
>>> DHT11_DATA_BIT_HIGH / timeres = 2 t can be 2 or 3
>>> so we have an ambiguity. The ambiguity could be removed by a smarter
>>> decoder, that looks at the t of other bits, but I'm not going to do
>>> that unless somebody is promising to test it on affected hardware.
>>>
>>> Feel free to add some comment about this to the code.
>>
>> Will do, thanks a lot for the explanation.
>>
>> I was asking because I see the "dht11: WARNING: decoding ambiguous"
>> very often. (with and without my patches)
>
> Yes, your patches shouldn't have any effect on this.
> "very often" in the sense of "not always"? This would be very surprising,
> because this would involve variable length clock ticks, i think.
It happens around 33% of all reads.
BTW: I'm using the DHT22's on my Beaglebone Black.
So the board should be "sane".
But I'll test them on another board too, just to be sure...
> I guess we should include timeres into the warning message.
I'll add some diagnose printk()s to find out.
Thanks,
//richard
Harald,
Am 03.12.2014 um 13:18 schrieb Harald Geyer:
> Hi Richard,
>
> thanks for all the work you put into this!
>
> Richard Weinberger writes:
>> Please see my current patches for your driver.
>> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
>> With the IRQ changes I see 84 edges.
>
> This still surprises me. With the IRQ changes I would expect the
> preamble to be 2 edges only. I must be missing something. Can you
> explain this to me?
Did some more tests.
With my IRQ changes applied I get as stated 84 edges,
without I get 85. So we're only losing one edge.
Of course if we're too slow with registering the IRQ we'll lose
more edges and the results renders unusable.
Thanks,
//richard
Am 03.12.2014 um 14:56 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Am 03.12.2014 um 13:58 schrieb Harald Geyer:
>>> Richard Weinberger writes:
>>>> Currently the driver uses pr_* and dev_* functions.
>>>> Change all logging functions to dev_* style to be consistent
>>>> and have the correct device prefix in all messages.
>>>
>>> Yes, actually this was on purpose:
>>> The dev_ messages are really about something wrong with the device.
>>> Ie if something goes wrong with one device but could perfectly work
>>> with some other device.
>>> The pr_ messages OTOH are about something wrong with clock resolution,
>>> etc that would affect any DHT11 sensor on the system. Ideally we would
>>> notice these things during probe() and just return with an error there.
>>> Right now we aren't as clever as that, so we just log an error message
>>> about the driver, when actually we are reading the device.
>>>
>>> That said, I don't have strong feelings about this. If you want to
>>> change this, I won't object. However if you really want to fix this,
>>> then the proper thing would be to check for this conditions in
>>> probe().
>>
>> Currently we get log messages of style:
>> "iio iio:deviceX: foo bar"
>> and:
>> "dht11 <name in DT>: foo bar"
>>
>> I really favorite "dht11 <name in DT".
>> In my device tree every senor has a sane name and log messages look like:
>> "dht11 toiletten sensor: invalid checksum"
>
> I think I ACKed the one with "iio iio:deviceX: foo bar"?
No, my patch turns all logs to "dht11 <name in DT>: foo bar".
>>>> This change set also adds new messages to diagnose issues.
>>>
>>> Comment below.
>>>
>>>> Signed-off-by: Richard Weinberger <[email protected]>
>>>> ---
>>>> drivers/iio/humidity/dht11.c | 16 ++++++++++------
>>>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>>>> index 0023699..fbcd7cb 100644
>>>> --- a/drivers/iio/humidity/dht11.c
>>>> +++ b/drivers/iio/humidity/dht11.c
>>>> @@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>>>> timeres = t;
>>>> }
>>>> if (2*timeres > DHT11_DATA_BIT_HIGH) {
>>>> - pr_err("dht11: timeresolution %d too bad for decoding\n",
>>>> + dev_err(dht11->dev, "timeresolution %d too bad for decoding\n",
>>>> timeres);
>>>> return -EIO;
>>>> }
>>>> threshold = DHT11_DATA_BIT_HIGH / timeres;
>>>> if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>>>> - pr_err("dht11: WARNING: decoding ambiguous\n");
>>>> + dev_err(dht11->dev, "decoding ambiguous\n");
>>>>
>>>> /* scale down with timeres and check validity */
>>>> for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
>>>> t = dht11->edges[offset + 2*i + 2].ts -
>>>> dht11->edges[offset + 2*i + 1].ts;
>>>> - if (!dht11->edges[offset + 2*i + 1].value)
>>>> - return -EIO; /* lost synchronisation */
>>>> + if (!dht11->edges[offset + 2*i + 1].value) {
>>>> + dev_err(dht11->dev, "lost synchronisation\n");
>>>> + return -EIO;
>>>> + }
>>>
>>> Are you sure this warrants a log message? I don't think this provides
>>> much information. The userspace application should just try reading
>>> the sensor again.
>>>
>>> We could do someting smart and try to recover from such errors, but
>>> ultimately userspace will need to deal with failed sensor communication
>>> anyway, so I don't see the point.
>>
>> The sensors are rather flaky and if they go nuts the user/developer
>> maybe wants to know why.
>> From a plain EIO she has no chance to find out. He'll have to add
>> printk()s by hand to find out.
>> With a log message one can start digging into the issue.
>
> I think any log messages that are caused by wiring problems are
> in place already. Also you should get an ETIMEDOUT not an EIO in
> these cases. In these cases the number of edges received is reported,
> which actually tells you a lot about the type of wiring problem.
>
> BTW when I originally submitted the driver, it contained the following
> code that got rejected:
> /*
> * dht11_edges_print: show the data as actually received by the
> * driver.
> * This is dead code, keeping it for now just in case somebody needs
> * it for porting the driver to new sensor HW, etc.
> *
> static void dht11_edges_print(struct dht11_gpio *dht11)
> {
> int i;
>
> pr_err("dht11: transfer timed out:\n");
> for (i = 1; i < dht11->num_edges; ++i) {
> pr_err("dht11: %d: %lld ns %s\n", i,
> dht11->edges[i].ts - dht11->edges[i-1].ts,
> dht11->edges[i-1].value ? "high" : "low");
> }
> }
> */
>
> which I used to call on errors to debug the decoder and the quirks
> necessary to support different sensors. Maybe someting like this would
> get accepted if it integrated properly with the debugging frameworks
> of the kernel instead of flooding the log?
>
> I think the EIO errors are all really of the type:
> "Some noise injection into the data line caused an hiccup, just try again."
>
> I wouldn't object to changing some of the EIOs to EAGAINs or something,
> but I doubt we get this through review.
>
> JFTR: I don't object strongly to additional logging. Just telling you
> 1) This has been considered when writing the driver.
> 2) I probably won't ACK it, so Jonathan will have to decide wether this
> is a valuable addition.
Okay. Let's see what Jonathan's opinion is.
IMHO a driver should use only one logging style not two.
Enough logging style bikeshedded, back to real work. :)
Thanks,
//richard
Am 03.12.2014 um 15:08 schrieb Harald Geyer:
>> I was asking because I see the "dht11: WARNING: decoding ambiguous"
>> very often. (with and without my patches)
>
> Yes, your patches shouldn't have any effect on this.
> "very often" in the sense of "not always"? This would be very surprising,
> because this would involve variable length clock ticks, i think.
>
> I guess we should include timeres into the warning message.
>
> Also I guess now is the time to think about a smarter decoder.
Another question. Your driver defines:
#define DHT11_DATA_BIT_LOW 27000
#define DHT11_DATA_BIT_HIGH 70000
If I read the manual [0] correctly these constants are T_h0/1.
Why did you use 27000 for T_h0?
Setting it to 26000 (the typical value as stated by the manual),
I get the "decoding ambiguous" warning *always*. Setting it higher
makes the message go away.
Thanks,
//richard
[0] http://meteobox.tk/files/AM2302.pdf
Richard Weinberger writes:
> Am 03.12.2014 um 15:08 schrieb Harald Geyer:
> >> I was asking because I see the "dht11: WARNING: decoding ambiguous"
> >> very often. (with and without my patches)
> >
> > Yes, your patches shouldn't have any effect on this.
> > "very often" in the sense of "not always"? This would be very surprising,
> > because this would involve variable length clock ticks, i think.
> >
> > I guess we should include timeres into the warning message.
> >
> > Also I guess now is the time to think about a smarter decoder.
I was wrong.
> Another question. Your driver defines:
> #define DHT11_DATA_BIT_LOW 27000
> #define DHT11_DATA_BIT_HIGH 70000
>
> If I read the manual [0] correctly these constants are T_h0/1.
> Why did you use 27000 for T_h0?
>
> [0] http://meteobox.tk/files/AM2302.pdf
It's a compromise between data sheets for various slightly different
sensors. In particular the data sheet for AM2303 specifies
26-28us, so it's just in the middle. But note that DHT11_DATA_BIT_LOW
isn't used in the actual decoding. Just for printing the warning.
But looking at your data sheet I see the we currently use a start
command that's outside the specified range... I will have to look
up where the current 80ms value was suggested.
> Setting it to 26000 (the typical value as stated by the manual),
> I get the "decoding ambiguous" warning *always*. Setting it higher
> makes the message go away.
If you misstyped "always" and "go away" then this is to be expected.
Also I think I now understand what is going on:
Your board most probably has a clock much faster then 32kH and when
we calculate timeres, we don't actually calculate the timestamp
resolution but the length of the shortest pulse. The decoding
algorithm is robust in such cases, but it generates wrong warnings.
The proper fix is to drop messages about clock speed from the
decoding functin. If we want to keep such diagnostics, we should
have them at probe() time. - This will also resolve our disagreement
about proper formatting of log messages about clock issues. :)
Optionally we can drop the calculation of timeres and just use a
constant threshold.
Thanks,
Harald
Harald,
Am 04.12.2014 um 14:45 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Am 03.12.2014 um 15:08 schrieb Harald Geyer:
>>>> I was asking because I see the "dht11: WARNING: decoding ambiguous"
>>>> very often. (with and without my patches)
>>>
>>> Yes, your patches shouldn't have any effect on this.
>>> "very often" in the sense of "not always"? This would be very surprising,
>>> because this would involve variable length clock ticks, i think.
>>>
>>> I guess we should include timeres into the warning message.
>>>
>>> Also I guess now is the time to think about a smarter decoder.
>
> I was wrong.
>
>> Another question. Your driver defines:
>> #define DHT11_DATA_BIT_LOW 27000
>> #define DHT11_DATA_BIT_HIGH 70000
>>
>> If I read the manual [0] correctly these constants are T_h0/1.
>> Why did you use 27000 for T_h0?
>>
>> [0] http://meteobox.tk/files/AM2302.pdf
>
> It's a compromise between data sheets for various slightly different
> sensors. In particular the data sheet for AM2303 specifies
> 26-28us, so it's just in the middle. But note that DHT11_DATA_BIT_LOW
> isn't used in the actual decoding. Just for printing the warning.
>
> But looking at your data sheet I see the we currently use a start
> command that's outside the specified range... I will have to look
> up where the current 80ms value was suggested.
>
>> Setting it to 26000 (the typical value as stated by the manual),
>> I get the "decoding ambiguous" warning *always*. Setting it higher
>> makes the message go away.
>
> If you misstyped "always" and "go away" then this is to be expected.
Nope, a lower value is bad and a higher makes the warning go away.
> Also I think I now understand what is going on:
> Your board most probably has a clock much faster then 32kH and when
> we calculate timeres, we don't actually calculate the timestamp
> resolution but the length of the shortest pulse. The decoding
> algorithm is robust in such cases, but it generates wrong warnings.
>
> The proper fix is to drop messages about clock speed from the
> decoding functin. If we want to keep such diagnostics, we should
> have them at probe() time. - This will also resolve our disagreement
> about proper formatting of log messages about clock issues. :)
Sounds sane.
> Optionally we can drop the calculation of timeres and just use a
> constant threshold.
Same here.
Thanks,
//richard
Richard Weinberger writes:
> Am 03.12.2014 um 14:56 schrieb Harald Geyer:
> > Richard Weinberger writes:
> >> Am 03.12.2014 um 13:58 schrieb Harald Geyer:
> >> Currently we get log messages of style:
> >> "iio iio:deviceX: foo bar"
> >> and:
> >> "dht11 <name in DT>: foo bar"
> >>
> >> I really favorite "dht11 <name in DT".
> >> In my device tree every senor has a sane name and log messages look like:
> >> "dht11 toiletten sensor: invalid checksum"
> >
> > I think I ACKed the one with "iio iio:deviceX: foo bar"?
>
> No, my patch turns all logs to "dht11 <name in DT>: foo bar".
Maybe I was unclear. AFAICS there is one case where we actually have
"iio iio:deviceX: foo bar" now and I said:
> @@ -193,7 +197,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> free_irq(dht11->irq, iio_dev);
>
> if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
> - dev_err(&iio_dev->dev,
> + dev_err(dht11->dev,
Ack.
Hope that clarifies,
Harald
Richard Weinberger writes:
> Harald,
>
> Am 03.12.2014 um 13:18 schrieb Harald Geyer:
> > Hi Richard,
> >
> > thanks for all the work you put into this!
> >
> > Richard Weinberger writes:
> >> Please see my current patches for your driver.
> >> As discussed in an earlier mail I'm testing with the DHT22 sensor only.
> >> With the IRQ changes I see 84 edges.
> >
> > This still surprises me. With the IRQ changes I would expect the
> > preamble to be 2 edges only. I must be missing something. Can you
> > explain this to me?
>
> Did some more tests.
> With my IRQ changes applied I get as stated 84 edges,
> without I get 85. So we're only losing one edge.
Ok, finally had time to look into this and it's a simple off-by-one
issue (no bug) that confused me:
Without your patch the full number of edges per data transmission
is 86. But since the last edge is not significant, we only store
85 edges. - There should be a comment about this in the source...
With your patch the preamble is shortened by two edges as expected,
but since there is a workaround for sensors where the preamble wasn't
properly recorded, nobody notices unless they are looking closely at
the number of edges recorded. The next thing to do is check on DHT11
if the workaround is still needed.
Note that the old behaviour was nice for debugging wiring problems:
With my standard setup (3.3V, pin drive strenght 4 mA) I got the
following:
supply not connected: 0 edges (sensor always pulls the data line low)
data not connected: 2 edges (we only see the signal we generate ourselves)
gnd not connected: random amount of edges
With the new behaviour we will get 0 edges in both of the first two
cases. So we are really losing a tiny bit of functionality... :-(
HTH,
Harald
Hi Richard,
finally got around to test this patch on all HW I have.
As expected the preamble needs to be shortend by two edges:
With your patch in its current form, the driver stops to work
reliably with DHT11. Also with DHT22 you get some delay when
reading the data, because you always wait for the timeout to
happen, before trying to decode the data.
Since your patch title includes "fix", the commit message
probably should mention that the patch as a side effect
changes behaviour - even if it's just diagnostic messages.
Thanks,
Harald
On Wed, 3 Dec 2014 00:32:54 +0100, Richard Weinberger <[email protected]>
wrote:
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> 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 168ebc4..0023699 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -140,6 +140,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)
> @@ -160,8 +181,17 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> if (ret)
> goto err;
>
> + ret = request_irq(dht11->irq, dht11_handle_irq,
> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> + iio_dev->name, iio_dev);
> + if (ret)
> + goto err;
> +
> 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",
> @@ -197,27 +227,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), },
> @@ -260,11 +269,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;
Richard Weinberger schrieb am 03.12.2014 um 00:32:
> Make sure that the read function is not interrupted...
There is already a mutex iio_dev->info_exist_lock used to serialize
iio_channel_read(), which in turn accesses _read_raw(). See [1].
[1]http://lxr.free-electrons.com/ident?i=iio_channel_read
>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> drivers/iio/humidity/dht11.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index d8771f5..168ebc4 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -29,6 +29,7 @@
> #include <linux/wait.h>
> #include <linux/bitops.h>
> #include <linux/completion.h>
> +#include <linux/mutex.h>
> #include <linux/delay.h>
> #include <linux/gpio.h>
> #include <linux/of_gpio.h>
> @@ -57,6 +58,7 @@ struct dht11 {
> int irq;
>
> struct completion completion;
> + struct mutex lock;
>
> s64 timestamp;
> int temperature;
> @@ -145,6 +147,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> struct dht11 *dht11 = iio_priv(iio_dev);
> int ret;
>
> + mutex_lock(&dht11->lock);
> if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
> reinit_completion(&dht11->completion);
>
> @@ -185,6 +188,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> ret = -EINVAL;
> err:
> dht11->num_edges = -1;
> + mutex_unlock(&dht11->lock);
> return ret;
> }
>
> @@ -268,6 +272,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;
>
Richard Weinberger schrieb am 03.12.2014 um 00:32:
> As we access i-1 we must not start with i=0.
>
> Signed-off-by: Richard Weinberger <[email protected]>
Acked-by: Hartmut Knaack <[email protected]>
> ---
> drivers/iio/humidity/dht11.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index fbcd7cb..5dfe71b 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -90,7 +90,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>
> /* Calculate timestamp resolution */
> - for (i = 0; i < dht11->num_edges; ++i) {
> + for (i = 1; i < dht11->num_edges; ++i) {
> t = dht11->edges[i].ts - dht11->edges[i-1].ts;
> if (t > 0 && t < timeres)
> timeres = t;
>
Am 14.12.2014 um 13:31 schrieb Hartmut Knaack:
> Richard Weinberger schrieb am 03.12.2014 um 00:32:
>> Make sure that the read function is not interrupted...
> There is already a mutex iio_dev->info_exist_lock used to serialize
> iio_channel_read(), which in turn accesses _read_raw(). See [1].
>
> [1]http://lxr.free-electrons.com/ident?i=iio_channel_read
This is only for the in-kernel API and not for sysfs access.
I.e. if you access via sysfs it is not protected.
Thanks,
//richard
>> Signed-off-by: Richard Weinberger <[email protected]>
>> ---
>> drivers/iio/humidity/dht11.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>> index d8771f5..168ebc4 100644
>> --- a/drivers/iio/humidity/dht11.c
>> +++ b/drivers/iio/humidity/dht11.c
>> @@ -29,6 +29,7 @@
>> #include <linux/wait.h>
>> #include <linux/bitops.h>
>> #include <linux/completion.h>
>> +#include <linux/mutex.h>
>> #include <linux/delay.h>
>> #include <linux/gpio.h>
>> #include <linux/of_gpio.h>
>> @@ -57,6 +58,7 @@ struct dht11 {
>> int irq;
>>
>> struct completion completion;
>> + struct mutex lock;
>>
>> s64 timestamp;
>> int temperature;
>> @@ -145,6 +147,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>> struct dht11 *dht11 = iio_priv(iio_dev);
>> int ret;
>>
>> + mutex_lock(&dht11->lock);
>> if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>> reinit_completion(&dht11->completion);
>>
>> @@ -185,6 +188,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>> ret = -EINVAL;
>> err:
>> dht11->num_edges = -1;
>> + mutex_unlock(&dht11->lock);
>> return ret;
>> }
>>
>> @@ -268,6 +272,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;
>>
>