[PATCHv2 0/2] Provide new API to get the current time resolution

[PATCHv2 0/2] Provide new API to get the current time resolution

[Harald Geyer]

Hi,

this patch series introduces a new kernel internal API to get the
current time resolution. The first patch adds the implementation and
the second patch updates an example driver to show the possible code
simplification.

If patches 1 and 2 can't go in via the same tree, I'll resend patch 2
with more cleanup patches to dth11.c later -- the main goal for now is,
to get the new code for timekeeping reviewed and merged.

changes since V1:
* dropped patch for adding wrapper code to the iio framework
* improved commit messages

Harald Geyer (2):
  timekeeping: Provide new API to get the current time resolution
  iio: dht11: Use new function ktime_get_resolution_ns()

 drivers/iio/humidity/dht11.c |   42 ++++++++++++++++++++++--------------------
 include/linux/timekeeping.h  |    1 +
 kernel/time/timekeeping.c    |   17 +++++++++++++++++
 3 files changed, 40 insertions(+), 20 deletions(-)

-- 
1.7.2.5

[PATCHv2 1/2] timekeeping: Provide new API to get the current time resolution

[Harald Geyer]

This patch series introduces a new function
u32 ktime_get_resolution_ns(void)
which allows to clean up some driver code.

In particular the IIO subsystem has a function to provide timestamps for
events but no means to get their resolution. So currently the dht11 driver
tries to guess the resolution in a rather messy and convoluted way. We
can do much better with the new code.

This API is not designed to be exposed to user space.

This has been tested on i386, sunxi and mxs.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
changes since V1:
Improved commit message.

 include/linux/timekeeping.h |    1 +
 kernel/time/timekeeping.c   |   17 +++++++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 3eaae47..983b61e 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -163,6 +163,7 @@ extern ktime_t ktime_get(void);
 extern ktime_t ktime_get_with_offset(enum tk_offsets offs);
 extern ktime_t ktime_mono_to_any(ktime_t tmono, enum tk_offsets offs);
 extern ktime_t ktime_get_raw(void);
+extern u32 ktime_get_resolution_ns(void);
 
 /**
  * ktime_get_real - get the real (wall-) time in ktime_t format
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 91db941..8efd964 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -586,6 +586,23 @@ ktime_t ktime_get(void)
 }
 EXPORT_SYMBOL_GPL(ktime_get);
 
+u32 ktime_get_resolution_ns(void)
+{
+	struct timekeeper *tk = &tk_core.timekeeper;
+	unsigned int seq;
+	u32 nsecs;
+
+	WARN_ON(timekeeping_suspended);
+
+	do {
+		seq = read_seqcount_begin(&tk_core.seq);
+		nsecs = tk->tkr.mult >> tk->tkr.shift;
+	} while (read_seqcount_retry(&tk_core.seq, seq));
+
+	return nsecs;
+}
+EXPORT_SYMBOL_GPL(ktime_get_resolution_ns);
+
 static ktime_t *offsets[TK_OFFS_MAX] = {
 	[TK_OFFS_REAL]	= &tk_core.timekeeper.offs_real,
 	[TK_OFFS_BOOT]	= &tk_core.timekeeper.offs_boot,
-- 
1.7.2.5

[PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[Harald Geyer]

This cleans up the most ugly workaround in this driver. There are no
functional changes yet in the decoding algorithm, but we improve the
following things:
 * Get rid of spurious warning messages on systems with fast HRTIMER.
 * If the clock is not fast enough for decoding to work, we give
   up immediately.
 * In that case we return EAGAIN instead of EIO, so it's easier to
   discriminate causes of failure.

Returning EAGAIN is somewhat controversial: It's technically correct
as a faster clock might become available. OTOH once all clocks are
enabled this is a permanent error. There is no ECLOCKTOOSLOW error
code.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
changes since V1:
call ktime_get_xxx() functions directly instead of using the wrappers
of the iio subsystem.

 drivers/iio/humidity/dht11.c |   42 ++++++++++++++++++++++--------------------
 1 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 7d79a1a..4cb25dc 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -33,6 +33,7 @@
 #include <linux/delay.h>
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
+#include <linux/timekeeping.h>
 
 #include <linux/iio/iio.h>
 
@@ -87,23 +88,11 @@ static unsigned char dht11_decode_byte(int *timing, int threshold)
 	return ret;
 }
 
-static int dht11_decode(struct dht11 *dht11, int offset)
+static int dht11_decode(struct dht11 *dht11, int offset, int timeres)
 {
-	int i, t, timing[DHT11_BITS_PER_READ], threshold,
-		timeres = DHT11_SENSOR_RESPONSE;
+	int i, t, timing[DHT11_BITS_PER_READ], threshold;
 	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
 
-	/* Calculate timestamp resolution */
-	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;
-	}
-	if (2*timeres > DHT11_DATA_BIT_HIGH) {
-		pr_err("dht11: 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");
@@ -126,7 +115,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
 		return -EIO;
 
-	dht11->timestamp = iio_get_time_ns();
+	dht11->timestamp = ktime_get_real_ns();
 	if (hum_int < 20) {  /* DHT22 */
 		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
 					((temp_int & 0x80) ? -100 : 100);
@@ -154,7 +143,7 @@ static irqreturn_t dht11_handle_irq(int irq, void *data)
 
 	/* 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].ts = ktime_get_real_ns();
 		dht11->edges[dht11->num_edges++].value =
 						gpio_get_value(dht11->gpio);
 
@@ -170,10 +159,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 			int *val, int *val2, long m)
 {
 	struct dht11 *dht11 = iio_priv(iio_dev);
-	int ret;
+	int ret, timeres;
 
 	mutex_lock(&dht11->lock);
-	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
+	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
+		timeres = ktime_get_resolution_ns();
+		if (DHT11_DATA_BIT_HIGH < 2*timeres) {
+			dev_err(dht11->dev, "timeresolution %dns too low\n",
+						timeres);
+			/* In theory a better clock could become available
+			 * at some point ... and there is no error code
+			 * that really fits better.
+			 */
+			ret = -EAGAIN;
+			goto err;
+		}
+
 		reinit_completion(&dht11->completion);
 
 		dht11->num_edges = 0;
@@ -208,7 +209,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		ret = dht11_decode(dht11,
 				dht11->num_edges == DHT11_EDGES_PER_READ ?
 					DHT11_EDGES_PREAMBLE :
-					DHT11_EDGES_PREAMBLE - 2);
+					DHT11_EDGES_PREAMBLE - 2,
+				timeres);
 		if (ret)
 			goto err;
 	}
@@ -274,7 +276,7 @@ static int dht11_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
+	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
 	dht11->num_edges = -1;
 
 	platform_set_drvdata(pdev, iio);
-- 
1.7.2.5

Re: [PATCHv2 1/2] timekeeping: Provide new API to get the current time resolution

[Richard Weinberger]

Am 07.04.2015 um 13:12 schrieb Harald Geyer:
> This patch series introduces a new function
> u32 ktime_get_resolution_ns(void)
> which allows to clean up some driver code.
> 
> In particular the IIO subsystem has a function to provide timestamps for
> events but no means to get their resolution. So currently the dht11 driver
> tries to guess the resolution in a rather messy and convoluted way. We
> can do much better with the new code.
> 
> This API is not designed to be exposed to user space.
> 
> This has been tested on i386, sunxi and mxs.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
> changes since V1:
> Improved commit message.
> 
>  include/linux/timekeeping.h |    1 +
>  kernel/time/timekeeping.c   |   17 +++++++++++++++++
>  2 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index 3eaae47..983b61e 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -163,6 +163,7 @@ extern ktime_t ktime_get(void);
>  extern ktime_t ktime_get_with_offset(enum tk_offsets offs);
>  extern ktime_t ktime_mono_to_any(ktime_t tmono, enum tk_offsets offs);
>  extern ktime_t ktime_get_raw(void);
> +extern u32 ktime_get_resolution_ns(void);
>  
>  /**
>   * ktime_get_real - get the real (wall-) time in ktime_t format
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 91db941..8efd964 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -586,6 +586,23 @@ ktime_t ktime_get(void)
>  }
>  EXPORT_SYMBOL_GPL(ktime_get);
>  
> +u32 ktime_get_resolution_ns(void)
> +{
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	unsigned int seq;
> +	u32 nsecs;
> +
> +	WARN_ON(timekeeping_suspended);
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +		nsecs = tk->tkr.mult >> tk->tkr.shift;
> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	return nsecs;
> +}
> +EXPORT_SYMBOL_GPL(ktime_get_resolution_ns);

Hmm, isn't this ktime_get_raw_ns()?

Thanks,
//richard

Re: [PATCHv2 1/2] timekeeping: Provide new API to get the current time resolution

[Richard Weinberger]

Am 08.04.2015 um 00:30 schrieb Richard Weinberger:
> Am 07.04.2015 um 13:12 schrieb Harald Geyer:
>> This patch series introduces a new function
>> u32 ktime_get_resolution_ns(void)
>> which allows to clean up some driver code.
>>
>> In particular the IIO subsystem has a function to provide timestamps for
>> events but no means to get their resolution. So currently the dht11 driver
>> tries to guess the resolution in a rather messy and convoluted way. We
>> can do much better with the new code.
>>
>> This API is not designed to be exposed to user space.
>>
>> This has been tested on i386, sunxi and mxs.
>>
>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>> ---
>> changes since V1:
>> Improved commit message.
>>
>>  include/linux/timekeeping.h |    1 +
>>  kernel/time/timekeeping.c   |   17 +++++++++++++++++
>>  2 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
>> index 3eaae47..983b61e 100644
>> --- a/include/linux/timekeeping.h
>> +++ b/include/linux/timekeeping.h
>> @@ -163,6 +163,7 @@ extern ktime_t ktime_get(void);
>>  extern ktime_t ktime_get_with_offset(enum tk_offsets offs);
>>  extern ktime_t ktime_mono_to_any(ktime_t tmono, enum tk_offsets offs);
>>  extern ktime_t ktime_get_raw(void);
>> +extern u32 ktime_get_resolution_ns(void);
>>  
>>  /**
>>   * ktime_get_real - get the real (wall-) time in ktime_t format
>> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
>> index 91db941..8efd964 100644
>> --- a/kernel/time/timekeeping.c
>> +++ b/kernel/time/timekeeping.c
>> @@ -586,6 +586,23 @@ ktime_t ktime_get(void)
>>  }
>>  EXPORT_SYMBOL_GPL(ktime_get);
>>  
>> +u32 ktime_get_resolution_ns(void)
>> +{
>> +	struct timekeeper *tk = &tk_core.timekeeper;
>> +	unsigned int seq;
>> +	u32 nsecs;
>> +
>> +	WARN_ON(timekeeping_suspended);
>> +
>> +	do {
>> +		seq = read_seqcount_begin(&tk_core.seq);
>> +		nsecs = tk->tkr.mult >> tk->tkr.shift;
>> +	} while (read_seqcount_retry(&tk_core.seq, seq));
>> +
>> +	return nsecs;
>> +}
>> +EXPORT_SYMBOL_GPL(ktime_get_resolution_ns);
> 
> Hmm, isn't this ktime_get_raw_ns()?

Whoops, it is of course not the same.

Thanks,
//richard

Re: [PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[Jonathan Cameron]

On 07/04/15 12:12, Harald Geyer wrote:
> This cleans up the most ugly workaround in this driver. There are no
> functional changes yet in the decoding algorithm, but we improve the
> following things:
>  * Get rid of spurious warning messages on systems with fast HRTIMER.
>  * If the clock is not fast enough for decoding to work, we give
>    up immediately.
>  * In that case we return EAGAIN instead of EIO, so it's easier to
>    discriminate causes of failure.
> 
> Returning EAGAIN is somewhat controversial: It's technically correct
> as a faster clock might become available. OTOH once all clocks are
> enabled this is a permanent error. There is no ECLOCKTOOSLOW error
> code.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Looks good to me.  Will wait for comments on the first patch though
before taking this... clearly that one will need a few acks if I take
it through IIO!

Jonathan
> ---
> changes since V1:
> call ktime_get_xxx() functions directly instead of using the wrappers
> of the iio subsystem.
> 
>  drivers/iio/humidity/dht11.c |   42 ++++++++++++++++++++++--------------------
>  1 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 7d79a1a..4cb25dc 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -33,6 +33,7 @@
>  #include <linux/delay.h>
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
> +#include <linux/timekeeping.h>
>  
>  #include <linux/iio/iio.h>
>  
> @@ -87,23 +88,11 @@ static unsigned char dht11_decode_byte(int *timing, int threshold)
>  	return ret;
>  }
>  
> -static int dht11_decode(struct dht11 *dht11, int offset)
> +static int dht11_decode(struct dht11 *dht11, int offset, int timeres)
>  {
> -	int i, t, timing[DHT11_BITS_PER_READ], threshold,
> -		timeres = DHT11_SENSOR_RESPONSE;
> +	int i, t, timing[DHT11_BITS_PER_READ], threshold;
>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>  
> -	/* Calculate timestamp resolution */
> -	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;
> -	}
> -	if (2*timeres > DHT11_DATA_BIT_HIGH) {
> -		pr_err("dht11: 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");
> @@ -126,7 +115,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>  		return -EIO;
>  
> -	dht11->timestamp = iio_get_time_ns();
> +	dht11->timestamp = ktime_get_real_ns();
>  	if (hum_int < 20) {  /* DHT22 */
>  		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
>  					((temp_int & 0x80) ? -100 : 100);
> @@ -154,7 +143,7 @@ static irqreturn_t dht11_handle_irq(int irq, void *data)
>  
>  	/* 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].ts = ktime_get_real_ns();
>  		dht11->edges[dht11->num_edges++].value =
>  						gpio_get_value(dht11->gpio);
>  
> @@ -170,10 +159,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  			int *val, int *val2, long m)
>  {
>  	struct dht11 *dht11 = iio_priv(iio_dev);
> -	int ret;
> +	int ret, timeres;
>  
>  	mutex_lock(&dht11->lock);
> -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
> +		timeres = ktime_get_resolution_ns();
> +		if (DHT11_DATA_BIT_HIGH < 2*timeres) {
> +			dev_err(dht11->dev, "timeresolution %dns too low\n",
> +						timeres);
> +			/* In theory a better clock could become available
> +			 * at some point ... and there is no error code
> +			 * that really fits better.
> +			 */
> +			ret = -EAGAIN;
> +			goto err;
> +		}
> +
>  		reinit_completion(&dht11->completion);
>  
>  		dht11->num_edges = 0;
> @@ -208,7 +209,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		ret = dht11_decode(dht11,
>  				dht11->num_edges == DHT11_EDGES_PER_READ ?
>  					DHT11_EDGES_PREAMBLE :
> -					DHT11_EDGES_PREAMBLE - 2);
> +					DHT11_EDGES_PREAMBLE - 2,
> +				timeres);
>  		if (ret)
>  			goto err;
>  	}
> @@ -274,7 +276,7 @@ static int dht11_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
> +	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
>  	dht11->num_edges = -1;
>  
>  	platform_set_drvdata(pdev, iio);
> 

Re: [PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[Harald Geyer]

Hi Jonathan!

Jonathan Cameron writes:
> On 07/04/15 12:12, Harald Geyer wrote:
> > This cleans up the most ugly workaround in this driver. There are no
> > functional changes yet in the decoding algorithm, but we improve the
> > following things:
> >  * Get rid of spurious warning messages on systems with fast HRTIMER.
> >  * If the clock is not fast enough for decoding to work, we give
> >    up immediately.
> >  * In that case we return EAGAIN instead of EIO, so it's easier to
> >    discriminate causes of failure.
> > 
> > Returning EAGAIN is somewhat controversial: It's technically correct
> > as a faster clock might become available. OTOH once all clocks are
> > enabled this is a permanent error. There is no ECLOCKTOOSLOW error
> > code.
> > 
> > Signed-off-by: Harald Geyer <harald@ccbib.org>
> Looks good to me.  Will wait for comments on the first patch though
> before taking this... clearly that one will need a few acks if I take
> it through IIO!

The first patch has been merged via tip tree for 4.2 (is already available
in staging-next tree). Are you going to pick up this patch or do I need
to resend?

Thanks,
Harald

> Jonathan
> > ---
> > changes since V1:
> > call ktime_get_xxx() functions directly instead of using the wrappers
> > of the iio subsystem.
> > 
> >  drivers/iio/humidity/dht11.c |   42 ++++++++++++++++++++++--------------------
> >  1 files changed, 22 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> > index 7d79a1a..4cb25dc 100644
> > --- a/drivers/iio/humidity/dht11.c
> > +++ b/drivers/iio/humidity/dht11.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/gpio.h>
> >  #include <linux/of_gpio.h>
> > +#include <linux/timekeeping.h>
> >  
> >  #include <linux/iio/iio.h>
> >  
> > @@ -87,23 +88,11 @@ static unsigned char dht11_decode_byte(int *timing, int threshold)
> >  	return ret;
> >  }
> >  
> > -static int dht11_decode(struct dht11 *dht11, int offset)
> > +static int dht11_decode(struct dht11 *dht11, int offset, int timeres)
> >  {
> > -	int i, t, timing[DHT11_BITS_PER_READ], threshold,
> > -		timeres = DHT11_SENSOR_RESPONSE;
> > +	int i, t, timing[DHT11_BITS_PER_READ], threshold;
> >  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
> >  
> > -	/* Calculate timestamp resolution */
> > -	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;
> > -	}
> > -	if (2*timeres > DHT11_DATA_BIT_HIGH) {
> > -		pr_err("dht11: 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");
> > @@ -126,7 +115,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
> >  	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
> >  		return -EIO;
> >  
> > -	dht11->timestamp = iio_get_time_ns();
> > +	dht11->timestamp = ktime_get_real_ns();
> >  	if (hum_int < 20) {  /* DHT22 */
> >  		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
> >  					((temp_int & 0x80) ? -100 : 100);
> > @@ -154,7 +143,7 @@ static irqreturn_t dht11_handle_irq(int irq, void *data)
> >  
> >  	/* 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].ts = ktime_get_real_ns();
> >  		dht11->edges[dht11->num_edges++].value =
> >  						gpio_get_value(dht11->gpio);
> >  
> > @@ -170,10 +159,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> >  			int *val, int *val2, long m)
> >  {
> >  	struct dht11 *dht11 = iio_priv(iio_dev);
> > -	int ret;
> > +	int ret, timeres;
> >  
> >  	mutex_lock(&dht11->lock);
> > -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
> > +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
> > +		timeres = ktime_get_resolution_ns();
> > +		if (DHT11_DATA_BIT_HIGH < 2*timeres) {
> > +			dev_err(dht11->dev, "timeresolution %dns too low\n",
> > +						timeres);
> > +			/* In theory a better clock could become available
> > +			 * at some point ... and there is no error code
> > +			 * that really fits better.
> > +			 */
> > +			ret = -EAGAIN;
> > +			goto err;
> > +		}
> > +
> >  		reinit_completion(&dht11->completion);
> >  
> >  		dht11->num_edges = 0;
> > @@ -208,7 +209,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
> >  		ret = dht11_decode(dht11,
> >  				dht11->num_edges == DHT11_EDGES_PER_READ ?
> >  					DHT11_EDGES_PREAMBLE :
> > -					DHT11_EDGES_PREAMBLE - 2);
> > +					DHT11_EDGES_PREAMBLE - 2,
> > +				timeres);
> >  		if (ret)
> >  			goto err;
> >  	}
> > @@ -274,7 +276,7 @@ static int dht11_probe(struct platform_device *pdev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
> > +	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
> >  	dht11->num_edges = -1;
> >  
> >  	platform_set_drvdata(pdev, iio);
> > 
> 

Re: [PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[Hartmut Knaack]

Harald Geyer schrieb am 29.06.2015 um 20:59:
> Hi Jonathan!
> 
> Jonathan Cameron writes:
>> On 07/04/15 12:12, Harald Geyer wrote:
>>> This cleans up the most ugly workaround in this driver. There are no
>>> functional changes yet in the decoding algorithm, but we improve the
>>> following things:
>>>  * Get rid of spurious warning messages on systems with fast HRTIMER.
>>>  * If the clock is not fast enough for decoding to work, we give
>>>    up immediately.
>>>  * In that case we return EAGAIN instead of EIO, so it's easier to
>>>    discriminate causes of failure.
>>>
>>> Returning EAGAIN is somewhat controversial: It's technically correct
>>> as a faster clock might become available. OTOH once all clocks are
>>> enabled this is a permanent error. There is no ECLOCKTOOSLOW error
>>> code.
>>>
>>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>> Looks good to me.  Will wait for comments on the first patch though
>> before taking this... clearly that one will need a few acks if I take
>> it through IIO!
> 
> The first patch has been merged via tip tree for 4.2 (is already available
> in staging-next tree). Are you going to pick up this patch or do I need
> to resend?
> 
> Thanks,
> Harald
> 

Hi Harald,
there are a few small style issues, which checkpatch.pl --strict should
show you. It would be good to have them eliminated before this patch
gets applied.
Thanks,
Hartmut

>> Jonathan
>>> ---
>>> changes since V1:
>>> call ktime_get_xxx() functions directly instead of using the wrappers
>>> of the iio subsystem.
>>>
>>>  drivers/iio/humidity/dht11.c |   42 ++++++++++++++++++++++--------------------
>>>  1 files changed, 22 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>>> index 7d79a1a..4cb25dc 100644
>>> --- a/drivers/iio/humidity/dht11.c
>>> +++ b/drivers/iio/humidity/dht11.c
>>> @@ -33,6 +33,7 @@
>>>  #include <linux/delay.h>
>>>  #include <linux/gpio.h>
>>>  #include <linux/of_gpio.h>
>>> +#include <linux/timekeeping.h>
>>>  
>>>  #include <linux/iio/iio.h>
>>>  
>>> @@ -87,23 +88,11 @@ static unsigned char dht11_decode_byte(int *timing, int threshold)
>>>  	return ret;
>>>  }
>>>  
>>> -static int dht11_decode(struct dht11 *dht11, int offset)
>>> +static int dht11_decode(struct dht11 *dht11, int offset, int timeres)
>>>  {
>>> -	int i, t, timing[DHT11_BITS_PER_READ], threshold,
>>> -		timeres = DHT11_SENSOR_RESPONSE;
>>> +	int i, t, timing[DHT11_BITS_PER_READ], threshold;
>>>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>>>  
>>> -	/* Calculate timestamp resolution */
>>> -	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;
>>> -	}
>>> -	if (2*timeres > DHT11_DATA_BIT_HIGH) {
>>> -		pr_err("dht11: 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");
>>> @@ -126,7 +115,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>>>  	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>>>  		return -EIO;
>>>  
>>> -	dht11->timestamp = iio_get_time_ns();
>>> +	dht11->timestamp = ktime_get_real_ns();
>>>  	if (hum_int < 20) {  /* DHT22 */
>>>  		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
>>>  					((temp_int & 0x80) ? -100 : 100);
>>> @@ -154,7 +143,7 @@ static irqreturn_t dht11_handle_irq(int irq, void *data)
>>>  
>>>  	/* 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].ts = ktime_get_real_ns();
>>>  		dht11->edges[dht11->num_edges++].value =
>>>  						gpio_get_value(dht11->gpio);
>>>  
>>> @@ -170,10 +159,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>>>  			int *val, int *val2, long m)
>>>  {
>>>  	struct dht11 *dht11 = iio_priv(iio_dev);
>>> -	int ret;
>>> +	int ret, timeres;
>>>  
>>>  	mutex_lock(&dht11->lock);
>>> -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>>> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
>>> +		timeres = ktime_get_resolution_ns();
>>> +		if (DHT11_DATA_BIT_HIGH < 2*timeres) {
>>> +			dev_err(dht11->dev, "timeresolution %dns too low\n",
>>> +						timeres);
>>> +			/* In theory a better clock could become available
>>> +			 * at some point ... and there is no error code
>>> +			 * that really fits better.
>>> +			 */
>>> +			ret = -EAGAIN;
>>> +			goto err;
>>> +		}
>>> +
>>>  		reinit_completion(&dht11->completion);
>>>  
>>>  		dht11->num_edges = 0;
>>> @@ -208,7 +209,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>>>  		ret = dht11_decode(dht11,
>>>  				dht11->num_edges == DHT11_EDGES_PER_READ ?
>>>  					DHT11_EDGES_PREAMBLE :
>>> -					DHT11_EDGES_PREAMBLE - 2);
>>> +					DHT11_EDGES_PREAMBLE - 2,
>>> +				timeres);
>>>  		if (ret)
>>>  			goto err;
>>>  	}
>>> @@ -274,7 +276,7 @@ static int dht11_probe(struct platform_device *pdev)
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
>>> +	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
>>>  	dht11->num_edges = -1;
>>>  
>>>  	platform_set_drvdata(pdev, iio);
>>>
>>
> --
> 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: [PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[harald]

On Mon, 29 Jun 2015 21:37:22 +0200, Hartmut Knaack <knaack.h@gmx.de>
wrote:
> Harald Geyer schrieb am 29.06.2015 um 20:59:
>> Hi Jonathan!
>> 
>> Jonathan Cameron writes:
[...]
>>> Looks good to me.  Will wait for comments on the first patch though
>>> before taking this... clearly that one will need a few acks if I take
>>> it through IIO!
>> 
>> The first patch has been merged via tip tree for 4.2 (is already
>> available
>> in staging-next tree). Are you going to pick up this patch or do I need
>> to resend?
>> 
>> Thanks,
>> Harald
>> 
> 
> Hi Harald,
> there are a few small style issues, which checkpatch.pl --strict should
> show you. 

Thanks for pointing out --strict - didn't know about that yet.

I get the following output:

CHECK: spaces preferred around that '*' (ctx:VxV)
#96: FILE: drivers/iio/humidity/dht11.c:167:
+               if (DHT11_DATA_BIT_HIGH < 2*timeres) {
                                           ^

With this one I disagree. Omitting the spaces makes the code easier to
read, because it reflects the relative binding power of operators involed.
This style (omitting spaces around * if there is a +, <, etc in the same
experession) is used throughout the driver. I'd change this only if there
is consensus, that this really is the preferred way.

CHECK: Alignment should match open parenthesis
#98: FILE: drivers/iio/humidity/dht11.c:169:
+                       dev_err(dht11->dev, "timeresolution %dns too
low\n",
+                                               timeres);

Ok, I'll post a patch that updates the driver to this style and update the
above patch acordingly.

Thanks,
Harald


> Thanks,
> Hartmut
> 
>>> Jonathan
>>>> ---
>>>> changes since V1:
>>>> call ktime_get_xxx() functions directly instead of using the wrappers
>>>> of the iio subsystem.
>>>>
>>>>  drivers/iio/humidity/dht11.c |   42
>>>>  ++++++++++++++++++++++--------------------
>>>>  1 files changed, 22 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/humidity/dht11.c
>>>> b/drivers/iio/humidity/dht11.c
>>>> index 7d79a1a..4cb25dc 100644
>>>> --- a/drivers/iio/humidity/dht11.c
>>>> +++ b/drivers/iio/humidity/dht11.c
>>>> @@ -33,6 +33,7 @@
>>>>  #include <linux/delay.h>
>>>>  #include <linux/gpio.h>
>>>>  #include <linux/of_gpio.h>
>>>> +#include <linux/timekeeping.h>
>>>>  
>>>>  #include <linux/iio/iio.h>
>>>>  
>>>> @@ -87,23 +88,11 @@ static unsigned char dht11_decode_byte(int
>>>> *timing, int threshold)
>>>>  	return ret;
>>>>  }
>>>>  
>>>> -static int dht11_decode(struct dht11 *dht11, int offset)
>>>> +static int dht11_decode(struct dht11 *dht11, int offset, int
timeres)
>>>>  {
>>>> -	int i, t, timing[DHT11_BITS_PER_READ], threshold,
>>>> -		timeres = DHT11_SENSOR_RESPONSE;
>>>> +	int i, t, timing[DHT11_BITS_PER_READ], threshold;
>>>>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>>>>  
>>>> -	/* Calculate timestamp resolution */
>>>> -	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;
>>>> -	}
>>>> -	if (2*timeres > DHT11_DATA_BIT_HIGH) {
>>>> -		pr_err("dht11: 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");
>>>> @@ -126,7 +115,7 @@ static int dht11_decode(struct dht11 *dht11, int
>>>> offset)
>>>>  	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>>>>  		return -EIO;
>>>>  
>>>> -	dht11->timestamp = iio_get_time_ns();
>>>> +	dht11->timestamp = ktime_get_real_ns();
>>>>  	if (hum_int < 20) {  /* DHT22 */
>>>>  		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
>>>>  					((temp_int & 0x80) ? -100 : 100);
>>>> @@ -154,7 +143,7 @@ static irqreturn_t dht11_handle_irq(int irq, void
>>>> *data)
>>>>  
>>>>  	/* 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].ts = ktime_get_real_ns();
>>>>  		dht11->edges[dht11->num_edges++].value =
>>>>  						gpio_get_value(dht11->gpio);
>>>>  
>>>> @@ -170,10 +159,22 @@ static int dht11_read_raw(struct iio_dev
>>>> *iio_dev,
>>>>  			int *val, int *val2, long m)
>>>>  {
>>>>  	struct dht11 *dht11 = iio_priv(iio_dev);
>>>> -	int ret;
>>>> +	int ret, timeres;
>>>>  
>>>>  	mutex_lock(&dht11->lock);
>>>> -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>>>> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns())
{
>>>> +		timeres = ktime_get_resolution_ns();
>>>> +		if (DHT11_DATA_BIT_HIGH < 2*timeres) {
>>>> +			dev_err(dht11->dev, "timeresolution %dns too low\n",
>>>> +						timeres);
>>>> +			/* In theory a better clock could become available
>>>> +			 * at some point ... and there is no error code
>>>> +			 * that really fits better.
>>>> +			 */
>>>> +			ret = -EAGAIN;
>>>> +			goto err;
>>>> +		}
>>>> +
>>>>  		reinit_completion(&dht11->completion);
>>>>  
>>>>  		dht11->num_edges = 0;
>>>> @@ -208,7 +209,8 @@ static int dht11_read_raw(struct iio_dev
*iio_dev,
>>>>  		ret = dht11_decode(dht11,
>>>>  				dht11->num_edges == DHT11_EDGES_PER_READ ?
>>>>  					DHT11_EDGES_PREAMBLE :
>>>> -					DHT11_EDGES_PREAMBLE - 2);
>>>> +					DHT11_EDGES_PREAMBLE - 2,
>>>> +				timeres);
>>>>  		if (ret)
>>>>  			goto err;
>>>>  	}
>>>> @@ -274,7 +276,7 @@ static int dht11_probe(struct platform_device
>>>> *pdev)
>>>>  		return -EINVAL;
>>>>  	}
>>>>  
>>>> -	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
>>>> +	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
>>>>  	dht11->num_edges = -1;
>>>>  
>>>>  	platform_set_drvdata(pdev, iio);
>>>>
>>>
>> --
>> 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: [PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[Richard Weinberger]

Am 30.06.2015 um 21:37 schrieb harald@ccbib.org:
> On Mon, 29 Jun 2015 21:37:22 +0200, Hartmut Knaack <knaack.h@gmx.de>
> wrote:
>> Harald Geyer schrieb am 29.06.2015 um 20:59:
>>> Hi Jonathan!
>>>
>>> Jonathan Cameron writes:
> [...]
>>>> Looks good to me.  Will wait for comments on the first patch though
>>>> before taking this... clearly that one will need a few acks if I take
>>>> it through IIO!
>>>
>>> The first patch has been merged via tip tree for 4.2 (is already
>>> available
>>> in staging-next tree). Are you going to pick up this patch or do I need
>>> to resend?
>>>
>>> Thanks,
>>> Harald
>>>
>>
>> Hi Harald,
>> there are a few small style issues, which checkpatch.pl --strict should
>> show you. 
> 
> Thanks for pointing out --strict - didn't know about that yet.

Don't take checkpatch.pl too seriously.

Thanks,
//richard

Re: [PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[Hartmut Knaack]

harald@ccbib.org schrieb am 30.06.2015 um 21:37:
> On Mon, 29 Jun 2015 21:37:22 +0200, Hartmut Knaack <knaack.h@gmx.de>
> wrote:
>> Harald Geyer schrieb am 29.06.2015 um 20:59:
>>> Hi Jonathan!
>>>
>>> Jonathan Cameron writes:
> [...]
>>>> Looks good to me.  Will wait for comments on the first patch though
>>>> before taking this... clearly that one will need a few acks if I take
>>>> it through IIO!
>>>
>>> The first patch has been merged via tip tree for 4.2 (is already
>>> available
>>> in staging-next tree). Are you going to pick up this patch or do I need
>>> to resend?
>>>
>>> Thanks,
>>> Harald
>>>
>>
>> Hi Harald,
>> there are a few small style issues, which checkpatch.pl --strict should
>> show you. 
> 
> Thanks for pointing out --strict - didn't know about that yet.
> 
> I get the following output:
> 
> CHECK: spaces preferred around that '*' (ctx:VxV)
> #96: FILE: drivers/iio/humidity/dht11.c:167:
> +               if (DHT11_DATA_BIT_HIGH < 2*timeres) {
>                                            ^
> 
> With this one I disagree. Omitting the spaces makes the code easier to
> read, because it reflects the relative binding power of operators involed.
> This style (omitting spaces around * if there is a +, <, etc in the same
> experession) is used throughout the driver. I'd change this only if there
> is consensus, that this really is the preferred way.

I would prefer to make use of parenthesis to reflect a precedence instead
of violating coding style guidelines.
Honestly, it didn't come to my mind that you left out those spaces on
purpose.

> 
> CHECK: Alignment should match open parenthesis
> #98: FILE: drivers/iio/humidity/dht11.c:169:
> +                       dev_err(dht11->dev, "timeresolution %dns too
> low\n",
> +                                               timeres);
> 
> Ok, I'll post a patch that updates the driver to this style and update the
> above patch acordingly.
> 
> Thanks,
> Harald
> 
> 
>> Thanks,
>> Hartmut
>>
>>>> Jonathan
>>>>> ---
>>>>> changes since V1:
>>>>> call ktime_get_xxx() functions directly instead of using the wrappers
>>>>> of the iio subsystem.
>>>>>
>>>>>  drivers/iio/humidity/dht11.c |   42
>>>>>  ++++++++++++++++++++++--------------------
>>>>>  1 files changed, 22 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/humidity/dht11.c
>>>>> b/drivers/iio/humidity/dht11.c
>>>>> index 7d79a1a..4cb25dc 100644
>>>>> --- a/drivers/iio/humidity/dht11.c
>>>>> +++ b/drivers/iio/humidity/dht11.c
>>>>> @@ -33,6 +33,7 @@
>>>>>  #include <linux/delay.h>
>>>>>  #include <linux/gpio.h>
>>>>>  #include <linux/of_gpio.h>
>>>>> +#include <linux/timekeeping.h>
>>>>>  
>>>>>  #include <linux/iio/iio.h>
>>>>>  
>>>>> @@ -87,23 +88,11 @@ static unsigned char dht11_decode_byte(int
>>>>> *timing, int threshold)
>>>>>  	return ret;
>>>>>  }
>>>>>  
>>>>> -static int dht11_decode(struct dht11 *dht11, int offset)
>>>>> +static int dht11_decode(struct dht11 *dht11, int offset, int
> timeres)
>>>>>  {
>>>>> -	int i, t, timing[DHT11_BITS_PER_READ], threshold,
>>>>> -		timeres = DHT11_SENSOR_RESPONSE;
>>>>> +	int i, t, timing[DHT11_BITS_PER_READ], threshold;
>>>>>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>>>>>  
>>>>> -	/* Calculate timestamp resolution */
>>>>> -	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;
>>>>> -	}
>>>>> -	if (2*timeres > DHT11_DATA_BIT_HIGH) {
>>>>> -		pr_err("dht11: 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");
>>>>> @@ -126,7 +115,7 @@ static int dht11_decode(struct dht11 *dht11, int
>>>>> offset)
>>>>>  	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>>>>>  		return -EIO;
>>>>>  
>>>>> -	dht11->timestamp = iio_get_time_ns();
>>>>> +	dht11->timestamp = ktime_get_real_ns();
>>>>>  	if (hum_int < 20) {  /* DHT22 */
>>>>>  		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
>>>>>  					((temp_int & 0x80) ? -100 : 100);
>>>>> @@ -154,7 +143,7 @@ static irqreturn_t dht11_handle_irq(int irq, void
>>>>> *data)
>>>>>  
>>>>>  	/* 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].ts = ktime_get_real_ns();
>>>>>  		dht11->edges[dht11->num_edges++].value =
>>>>>  						gpio_get_value(dht11->gpio);
>>>>>  
>>>>> @@ -170,10 +159,22 @@ static int dht11_read_raw(struct iio_dev
>>>>> *iio_dev,
>>>>>  			int *val, int *val2, long m)
>>>>>  {
>>>>>  	struct dht11 *dht11 = iio_priv(iio_dev);
>>>>> -	int ret;
>>>>> +	int ret, timeres;
>>>>>  
>>>>>  	mutex_lock(&dht11->lock);
>>>>> -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>>>>> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns())
> {
>>>>> +		timeres = ktime_get_resolution_ns();
>>>>> +		if (DHT11_DATA_BIT_HIGH < 2*timeres) {
>>>>> +			dev_err(dht11->dev, "timeresolution %dns too low\n",
>>>>> +						timeres);
>>>>> +			/* In theory a better clock could become available
>>>>> +			 * at some point ... and there is no error code
>>>>> +			 * that really fits better.
>>>>> +			 */
>>>>> +			ret = -EAGAIN;
>>>>> +			goto err;
>>>>> +		}
>>>>> +
>>>>>  		reinit_completion(&dht11->completion);
>>>>>  
>>>>>  		dht11->num_edges = 0;
>>>>> @@ -208,7 +209,8 @@ static int dht11_read_raw(struct iio_dev
> *iio_dev,
>>>>>  		ret = dht11_decode(dht11,
>>>>>  				dht11->num_edges == DHT11_EDGES_PER_READ ?
>>>>>  					DHT11_EDGES_PREAMBLE :
>>>>> -					DHT11_EDGES_PREAMBLE - 2);
>>>>> +					DHT11_EDGES_PREAMBLE - 2,
>>>>> +				timeres);
>>>>>  		if (ret)
>>>>>  			goto err;
>>>>>  	}
>>>>> @@ -274,7 +276,7 @@ static int dht11_probe(struct platform_device
>>>>> *pdev)
>>>>>  		return -EINVAL;
>>>>>  	}
>>>>>  
>>>>> -	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
>>>>> +	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
>>>>>  	dht11->num_edges = -1;
>>>>>  
>>>>>  	platform_set_drvdata(pdev, iio);
>>>>>
>>>>
>>> --
>>> 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
>>>
> --
> 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: [PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[Jonathan Cameron]

On 30/06/15 20:37, harald@ccbib.org wrote:
> On Mon, 29 Jun 2015 21:37:22 +0200, Hartmut Knaack <knaack.h@gmx.de>
> wrote:
>> Harald Geyer schrieb am 29.06.2015 um 20:59:
>>> Hi Jonathan!
>>>
>>> Jonathan Cameron writes:
> [...]
>>>> Looks good to me.  Will wait for comments on the first patch though
>>>> before taking this... clearly that one will need a few acks if I take
>>>> it through IIO!
>>>
>>> The first patch has been merged via tip tree for 4.2 (is already
>>> available
>>> in staging-next tree). Are you going to pick up this patch or do I need
>>> to resend?
>>>
>>> Thanks,
>>> Harald
>>>
>>
>> Hi Harald,
>> there are a few small style issues, which checkpatch.pl --strict should
>> show you. 
> 
> Thanks for pointing out --strict - didn't know about that yet.
> 
> I get the following output:
> 
> CHECK: spaces preferred around that '*' (ctx:VxV)
> #96: FILE: drivers/iio/humidity/dht11.c:167:
> +               if (DHT11_DATA_BIT_HIGH < 2*timeres) {
>                                            ^
> 
> With this one I disagree. Omitting the spaces makes the code easier to
> read, because it reflects the relative binding power of operators involed.
> This style (omitting spaces around * if there is a +, <, etc in the same
> experession) is used throughout the driver. I'd change this only if there
> is consensus, that this really is the preferred way.
Personal taste wise I agree with you, but conventions are conventions
and the kernel way is spaces around binary operators to distinguish them
from unary operators...

I'll only get a newbie patch 'fixing' this otherwise, so please do change it!
> 
> CHECK: Alignment should match open parenthesis
> #98: FILE: drivers/iio/humidity/dht11.c:169:
> +                       dev_err(dht11->dev, "timeresolution %dns too
> low\n",
> +                                               timeres);
> 
> Ok, I'll post a patch that updates the driver to this style and update the
> above patch acordingly.
> 
> Thanks,
> Harald
> 
> 
>> Thanks,
>> Hartmut
>>
>>>> Jonathan
>>>>> ---
>>>>> changes since V1:
>>>>> call ktime_get_xxx() functions directly instead of using the wrappers
>>>>> of the iio subsystem.
>>>>>
>>>>>  drivers/iio/humidity/dht11.c |   42
>>>>>  ++++++++++++++++++++++--------------------
>>>>>  1 files changed, 22 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/humidity/dht11.c
>>>>> b/drivers/iio/humidity/dht11.c
>>>>> index 7d79a1a..4cb25dc 100644
>>>>> --- a/drivers/iio/humidity/dht11.c
>>>>> +++ b/drivers/iio/humidity/dht11.c
>>>>> @@ -33,6 +33,7 @@
>>>>>  #include <linux/delay.h>
>>>>>  #include <linux/gpio.h>
>>>>>  #include <linux/of_gpio.h>
>>>>> +#include <linux/timekeeping.h>
>>>>>  
>>>>>  #include <linux/iio/iio.h>
>>>>>  
>>>>> @@ -87,23 +88,11 @@ static unsigned char dht11_decode_byte(int
>>>>> *timing, int threshold)
>>>>>  	return ret;
>>>>>  }
>>>>>  
>>>>> -static int dht11_decode(struct dht11 *dht11, int offset)
>>>>> +static int dht11_decode(struct dht11 *dht11, int offset, int
> timeres)
>>>>>  {
>>>>> -	int i, t, timing[DHT11_BITS_PER_READ], threshold,
>>>>> -		timeres = DHT11_SENSOR_RESPONSE;
>>>>> +	int i, t, timing[DHT11_BITS_PER_READ], threshold;
>>>>>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>>>>>  
>>>>> -	/* Calculate timestamp resolution */
>>>>> -	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;
>>>>> -	}
>>>>> -	if (2*timeres > DHT11_DATA_BIT_HIGH) {
>>>>> -		pr_err("dht11: 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");
>>>>> @@ -126,7 +115,7 @@ static int dht11_decode(struct dht11 *dht11, int
>>>>> offset)
>>>>>  	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>>>>>  		return -EIO;
>>>>>  
>>>>> -	dht11->timestamp = iio_get_time_ns();
>>>>> +	dht11->timestamp = ktime_get_real_ns();
>>>>>  	if (hum_int < 20) {  /* DHT22 */
>>>>>  		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
>>>>>  					((temp_int & 0x80) ? -100 : 100);
>>>>> @@ -154,7 +143,7 @@ static irqreturn_t dht11_handle_irq(int irq, void
>>>>> *data)
>>>>>  
>>>>>  	/* 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].ts = ktime_get_real_ns();
>>>>>  		dht11->edges[dht11->num_edges++].value =
>>>>>  						gpio_get_value(dht11->gpio);
>>>>>  
>>>>> @@ -170,10 +159,22 @@ static int dht11_read_raw(struct iio_dev
>>>>> *iio_dev,
>>>>>  			int *val, int *val2, long m)
>>>>>  {
>>>>>  	struct dht11 *dht11 = iio_priv(iio_dev);
>>>>> -	int ret;
>>>>> +	int ret, timeres;
>>>>>  
>>>>>  	mutex_lock(&dht11->lock);
>>>>> -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>>>>> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns())
> {
>>>>> +		timeres = ktime_get_resolution_ns();
>>>>> +		if (DHT11_DATA_BIT_HIGH < 2*timeres) {
>>>>> +			dev_err(dht11->dev, "timeresolution %dns too low\n",
>>>>> +						timeres);
>>>>> +			/* In theory a better clock could become available
>>>>> +			 * at some point ... and there is no error code
>>>>> +			 * that really fits better.
>>>>> +			 */
>>>>> +			ret = -EAGAIN;
>>>>> +			goto err;
>>>>> +		}
>>>>> +
>>>>>  		reinit_completion(&dht11->completion);
>>>>>  
>>>>>  		dht11->num_edges = 0;
>>>>> @@ -208,7 +209,8 @@ static int dht11_read_raw(struct iio_dev
> *iio_dev,
>>>>>  		ret = dht11_decode(dht11,
>>>>>  				dht11->num_edges == DHT11_EDGES_PER_READ ?
>>>>>  					DHT11_EDGES_PREAMBLE :
>>>>> -					DHT11_EDGES_PREAMBLE - 2);
>>>>> +					DHT11_EDGES_PREAMBLE - 2,
>>>>> +				timeres);
>>>>>  		if (ret)
>>>>>  			goto err;
>>>>>  	}
>>>>> @@ -274,7 +276,7 @@ static int dht11_probe(struct platform_device
>>>>> *pdev)
>>>>>  		return -EINVAL;
>>>>>  	}
>>>>>  
>>>>> -	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
>>>>> +	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
>>>>>  	dht11->num_edges = -1;
>>>>>  
>>>>>  	platform_set_drvdata(pdev, iio);
>>>>>
>>>>
>>> --
>>> 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: [PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[harald]

On Sun, 05 Jul 2015 13:15:38 +0100, Jonathan Cameron <jic23@kernel.org>
wrote:
> On 30/06/15 20:37, harald@ccbib.org wrote:
>> Thanks for pointing out --strict - didn't know about that yet.
>> 
>> I get the following output:
>> 
>> CHECK: spaces preferred around that '*' (ctx:VxV)
>> #96: FILE: drivers/iio/humidity/dht11.c:167:
>> +               if (DHT11_DATA_BIT_HIGH < 2*timeres) {
>>                                            ^
>> 
>> With this one I disagree. Omitting the spaces makes the code easier to
>> read, because it reflects the relative binding power of operators
>> involed.
>> This style (omitting spaces around * if there is a +, <, etc in the
same
>> experession) is used throughout the driver. I'd change this only if
there
>> is consensus, that this really is the preferred way.
> Personal taste wise I agree with you, but conventions are conventions
> and the kernel way is spaces around binary operators to distinguish them
> from unary operators...
> 
> I'll only get a newbie patch 'fixing' this otherwise, so please do
change
> it!

Of course you have the last word on this. As long as the newbies are
competent enough to really only do whitespace changes, I'd be tempted
to wait for them to do the work. Alas I really want the original patch
merged soon so I will do the obfuscation myself.

Thanks,
Harald the newbie :)

[PATCHv3 1/4] iio: dht11: whitespace changes to make checkpatch.pl --strict happy

[Harald Geyer]

* add spaces around binary operators in cases where it reduces readability
* align multiline statements around opening parenthesis

Reported-by: Hartmut Knaack <knaack.h@gmx.de>
	in Message-ID: <55919E72.3010807@gmx.de>

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 drivers/iio/humidity/dht11.c |   25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 7d79a1a..343d26a 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -46,7 +46,8 @@
  * Note that when reading the sensor actually 84 edges are detected, but
  * since the last edge is not significant, we only store 83:
  */
-#define DHT11_EDGES_PER_READ (2*DHT11_BITS_PER_READ + DHT11_EDGES_PREAMBLE + 1)
+#define DHT11_EDGES_PER_READ (2 * DHT11_BITS_PER_READ + \
+			      DHT11_EDGES_PREAMBLE + 1)
 
 /* Data transmission timing (nano seconds) */
 #define DHT11_START_TRANSMISSION	18  /* ms */
@@ -95,24 +96,24 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 
 	/* Calculate timestamp resolution */
 	for (i = 1; i < dht11->num_edges; ++i) {
-		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
+		t = dht11->edges[i].ts - dht11->edges[i - 1].ts;
 		if (t > 0 && t < timeres)
 			timeres = t;
 	}
-	if (2*timeres > DHT11_DATA_BIT_HIGH) {
+	if (2 * timeres > DHT11_DATA_BIT_HIGH) {
 		pr_err("dht11: timeresolution %d too bad for decoding\n",
-			timeres);
+		       timeres);
 		return -EIO;
 	}
 	threshold = DHT11_DATA_BIT_HIGH / timeres;
-	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
+	if (DHT11_DATA_BIT_LOW / timeres + 1 >= threshold)
 		pr_err("dht11: WARNING: 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)
+		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 */
 		timing[i] = t / timeres;
 	}
@@ -166,7 +167,7 @@ static irqreturn_t dht11_handle_irq(int irq, void *data)
 }
 
 static int dht11_read_raw(struct iio_dev *iio_dev,
-			const struct iio_chan_spec *chan,
+			  const struct iio_chan_spec *chan,
 			int *val, int *val2, long m)
 {
 	struct dht11 *dht11 = iio_priv(iio_dev);
@@ -192,13 +193,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 			goto err;
 
 		ret = wait_for_completion_killable_timeout(&dht11->completion,
-								 HZ);
+							   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",
+				"Only %d signal edges detected\n",
 					dht11->num_edges);
 			ret = -ETIMEDOUT;
 		}
@@ -206,7 +207,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 			goto err;
 
 		ret = dht11_decode(dht11,
-				dht11->num_edges == DHT11_EDGES_PER_READ ?
+				   dht11->num_edges == DHT11_EDGES_PER_READ ?
 					DHT11_EDGES_PREAMBLE :
 					DHT11_EDGES_PREAMBLE - 2);
 		if (ret)
-- 
1.7.10.4

[PATCHv3 2/4] iio: dht11: add comment to make checkpatch.pl --strict happy

[Harald Geyer]

Explain why the driver needs a mutex.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 drivers/iio/humidity/dht11.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 343d26a..bb5ad57 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -63,6 +63,7 @@ struct dht11 {
 	int				irq;
 
 	struct completion		completion;
+	/* The iio sysfs interface doesn't prevent concurrent reads: */
 	struct mutex			lock;
 
 	s64				timestamp;
-- 
1.7.10.4

[PATCHv3 3/4] iio: dht11: avoid multiple assignments to make checkpatch.pl --strict happy

[Harald Geyer]

We just do the assignments in two steps.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 drivers/iio/humidity/dht11.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index bb5ad57..91976e0 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -263,9 +263,10 @@ static int dht11_probe(struct platform_device *pdev)
 	dht11 = iio_priv(iio);
 	dht11->dev = dev;
 
-	dht11->gpio = ret = of_get_gpio(node, 0);
+	ret = of_get_gpio(node, 0);
 	if (ret < 0)
 		return ret;
+	dht11->gpio = ret;
 	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
 	if (ret)
 		return ret;
-- 
1.7.10.4

[PATCHv3 4/4] iio: dht11: Use new function ktime_get_resolution_ns()

[Harald Geyer]

This cleans up the most ugly workaround in this driver. There are no
functional changes yet in the decoding algorithm, but we improve the
following things:
 * Get rid of spurious warning messages on systems with fast HRTIMER.
 * If the clock is not fast enough for decoding to work, we give
   up immediately.
 * In that case we return EAGAIN instead of EIO, so it's easier to
   discriminate causes of failure.

Returning EAGAIN is somewhat controversial: It's technically correct
as a faster clock might become available. OTOH once all clocks are
enabled this is a permanent error. There is no ECLOCKTOOSLOW error
code.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 drivers/iio/humidity/dht11.c |   42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 91976e0..1165b1c 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -33,6 +33,7 @@
 #include <linux/delay.h>
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
+#include <linux/timekeeping.h>
 
 #include <linux/iio/iio.h>
 
@@ -89,23 +90,11 @@ static unsigned char dht11_decode_byte(int *timing, int threshold)
 	return ret;
 }
 
-static int dht11_decode(struct dht11 *dht11, int offset)
+static int dht11_decode(struct dht11 *dht11, int offset, int timeres)
 {
-	int i, t, timing[DHT11_BITS_PER_READ], threshold,
-		timeres = DHT11_SENSOR_RESPONSE;
+	int i, t, timing[DHT11_BITS_PER_READ], threshold;
 	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
 
-	/* Calculate timestamp resolution */
-	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;
-	}
-	if (2 * timeres > DHT11_DATA_BIT_HIGH) {
-		pr_err("dht11: 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");
@@ -128,7 +117,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
 	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
 		return -EIO;
 
-	dht11->timestamp = iio_get_time_ns();
+	dht11->timestamp = ktime_get_real_ns();
 	if (hum_int < 20) {  /* DHT22 */
 		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
 					((temp_int & 0x80) ? -100 : 100);
@@ -156,7 +145,7 @@ static irqreturn_t dht11_handle_irq(int irq, void *data)
 
 	/* 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].ts = ktime_get_real_ns();
 		dht11->edges[dht11->num_edges++].value =
 						gpio_get_value(dht11->gpio);
 
@@ -172,10 +161,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 			int *val, int *val2, long m)
 {
 	struct dht11 *dht11 = iio_priv(iio_dev);
-	int ret;
+	int ret, timeres;
 
 	mutex_lock(&dht11->lock);
-	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
+	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
+		timeres = ktime_get_resolution_ns();
+		if (DHT11_DATA_BIT_HIGH < 2 * timeres) {
+			dev_err(dht11->dev, "timeresolution %dns too low\n",
+				timeres);
+			/* In theory a better clock could become available
+			 * at some point ... and there is no error code
+			 * that really fits better.
+			 */
+			ret = -EAGAIN;
+			goto err;
+		}
+
 		reinit_completion(&dht11->completion);
 
 		dht11->num_edges = 0;
@@ -210,7 +211,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		ret = dht11_decode(dht11,
 				   dht11->num_edges == DHT11_EDGES_PER_READ ?
 					DHT11_EDGES_PREAMBLE :
-					DHT11_EDGES_PREAMBLE - 2);
+					DHT11_EDGES_PREAMBLE - 2,
+				timeres);
 		if (ret)
 			goto err;
 	}
@@ -277,7 +279,7 @@ static int dht11_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
+	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
 	dht11->num_edges = -1;
 
 	platform_set_drvdata(pdev, iio);
-- 
1.7.10.4

Re: [PATCHv3 1/4] iio: dht11: whitespace changes to make checkpatch.pl --strict happy

[Jonathan Cameron]

On 07/07/15 14:39, Harald Geyer wrote:
> * add spaces around binary operators in cases where it reduces readability
> * align multiline statements around opening parenthesis
> 
> Reported-by: Hartmut Knaack <knaack.h@gmx.de>
> 	in Message-ID: <55919E72.3010807@gmx.de>
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Applied to the togreg branch of iio.git

Thanks,

Jonathan
> ---
>  drivers/iio/humidity/dht11.c |   25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 7d79a1a..343d26a 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -46,7 +46,8 @@
>   * Note that when reading the sensor actually 84 edges are detected, but
>   * since the last edge is not significant, we only store 83:
>   */
> -#define DHT11_EDGES_PER_READ (2*DHT11_BITS_PER_READ + DHT11_EDGES_PREAMBLE + 1)
> +#define DHT11_EDGES_PER_READ (2 * DHT11_BITS_PER_READ + \
> +			      DHT11_EDGES_PREAMBLE + 1)
>  
>  /* Data transmission timing (nano seconds) */
>  #define DHT11_START_TRANSMISSION	18  /* ms */
> @@ -95,24 +96,24 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  
>  	/* Calculate timestamp resolution */
>  	for (i = 1; i < dht11->num_edges; ++i) {
> -		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
> +		t = dht11->edges[i].ts - dht11->edges[i - 1].ts;
>  		if (t > 0 && t < timeres)
>  			timeres = t;
>  	}
> -	if (2*timeres > DHT11_DATA_BIT_HIGH) {
> +	if (2 * timeres > DHT11_DATA_BIT_HIGH) {
>  		pr_err("dht11: timeresolution %d too bad for decoding\n",
> -			timeres);
> +		       timeres);
>  		return -EIO;
>  	}
>  	threshold = DHT11_DATA_BIT_HIGH / timeres;
> -	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
> +	if (DHT11_DATA_BIT_LOW / timeres + 1 >= threshold)
>  		pr_err("dht11: WARNING: 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)
> +		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 */
>  		timing[i] = t / timeres;
>  	}
> @@ -166,7 +167,7 @@ static irqreturn_t dht11_handle_irq(int irq, void *data)
>  }
>  
>  static int dht11_read_raw(struct iio_dev *iio_dev,
> -			const struct iio_chan_spec *chan,
> +			  const struct iio_chan_spec *chan,
>  			int *val, int *val2, long m)
>  {
>  	struct dht11 *dht11 = iio_priv(iio_dev);
> @@ -192,13 +193,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  			goto err;
>  
>  		ret = wait_for_completion_killable_timeout(&dht11->completion,
> -								 HZ);
> +							   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",
> +				"Only %d signal edges detected\n",
>  					dht11->num_edges);
>  			ret = -ETIMEDOUT;
>  		}
> @@ -206,7 +207,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  			goto err;
>  
>  		ret = dht11_decode(dht11,
> -				dht11->num_edges == DHT11_EDGES_PER_READ ?
> +				   dht11->num_edges == DHT11_EDGES_PER_READ ?
>  					DHT11_EDGES_PREAMBLE :
>  					DHT11_EDGES_PREAMBLE - 2);
>  		if (ret)
> 

Re: [PATCHv3 2/4] iio: dht11: add comment to make checkpatch.pl --strict happy

[Jonathan Cameron]

On 07/07/15 14:39, Harald Geyer wrote:
> Explain why the driver needs a mutex.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Applied to the togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  drivers/iio/humidity/dht11.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 343d26a..bb5ad57 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -63,6 +63,7 @@ struct dht11 {
>  	int				irq;
>  
>  	struct completion		completion;
> +	/* The iio sysfs interface doesn't prevent concurrent reads: */
>  	struct mutex			lock;
>  
>  	s64				timestamp;
> 

Re: [PATCHv3 3/4] iio: dht11: avoid multiple assignments to make checkpatch.pl --strict happy

[Jonathan Cameron]

On 07/07/15 14:39, Harald Geyer wrote:
> We just do the assignments in two steps.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Applied to the togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  drivers/iio/humidity/dht11.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index bb5ad57..91976e0 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -263,9 +263,10 @@ static int dht11_probe(struct platform_device *pdev)
>  	dht11 = iio_priv(iio);
>  	dht11->dev = dev;
>  
> -	dht11->gpio = ret = of_get_gpio(node, 0);
> +	ret = of_get_gpio(node, 0);
>  	if (ret < 0)
>  		return ret;
> +	dht11->gpio = ret;
>  	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
>  	if (ret)
>  		return ret;
> 

Re: [PATCHv3 4/4] iio: dht11: Use new function ktime_get_resolution_ns()

[Jonathan Cameron]

On 07/07/15 14:39, Harald Geyer wrote:
> This cleans up the most ugly workaround in this driver. There are no
> functional changes yet in the decoding algorithm, but we improve the
> following things:
>  * Get rid of spurious warning messages on systems with fast HRTIMER.
>  * If the clock is not fast enough for decoding to work, we give
>    up immediately.
>  * In that case we return EAGAIN instead of EIO, so it's easier to
>    discriminate causes of failure.
> 
> Returning EAGAIN is somewhat controversial: It's technically correct
> as a faster clock might become available. OTOH once all clocks are
> enabled this is a permanent error. There is no ECLOCKTOOSLOW error
> code.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Applied to the togreg branch of iio.git. Thanks,

Jonathan
> ---
>  drivers/iio/humidity/dht11.c |   42 ++++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 91976e0..1165b1c 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -33,6 +33,7 @@
>  #include <linux/delay.h>
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
> +#include <linux/timekeeping.h>
>  
>  #include <linux/iio/iio.h>
>  
> @@ -89,23 +90,11 @@ static unsigned char dht11_decode_byte(int *timing, int threshold)
>  	return ret;
>  }
>  
> -static int dht11_decode(struct dht11 *dht11, int offset)
> +static int dht11_decode(struct dht11 *dht11, int offset, int timeres)
>  {
> -	int i, t, timing[DHT11_BITS_PER_READ], threshold,
> -		timeres = DHT11_SENSOR_RESPONSE;
> +	int i, t, timing[DHT11_BITS_PER_READ], threshold;
>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>  
> -	/* Calculate timestamp resolution */
> -	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;
> -	}
> -	if (2 * timeres > DHT11_DATA_BIT_HIGH) {
> -		pr_err("dht11: 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");
> @@ -128,7 +117,7 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>  	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>  		return -EIO;
>  
> -	dht11->timestamp = iio_get_time_ns();
> +	dht11->timestamp = ktime_get_real_ns();
>  	if (hum_int < 20) {  /* DHT22 */
>  		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
>  					((temp_int & 0x80) ? -100 : 100);
> @@ -156,7 +145,7 @@ static irqreturn_t dht11_handle_irq(int irq, void *data)
>  
>  	/* 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].ts = ktime_get_real_ns();
>  		dht11->edges[dht11->num_edges++].value =
>  						gpio_get_value(dht11->gpio);
>  
> @@ -172,10 +161,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  			int *val, int *val2, long m)
>  {
>  	struct dht11 *dht11 = iio_priv(iio_dev);
> -	int ret;
> +	int ret, timeres;
>  
>  	mutex_lock(&dht11->lock);
> -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns()) {
> +		timeres = ktime_get_resolution_ns();
> +		if (DHT11_DATA_BIT_HIGH < 2 * timeres) {
> +			dev_err(dht11->dev, "timeresolution %dns too low\n",
> +				timeres);
> +			/* In theory a better clock could become available
> +			 * at some point ... and there is no error code
> +			 * that really fits better.
> +			 */
> +			ret = -EAGAIN;
> +			goto err;
> +		}
> +
>  		reinit_completion(&dht11->completion);
>  
>  		dht11->num_edges = 0;
> @@ -210,7 +211,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		ret = dht11_decode(dht11,
>  				   dht11->num_edges == DHT11_EDGES_PER_READ ?
>  					DHT11_EDGES_PREAMBLE :
> -					DHT11_EDGES_PREAMBLE - 2);
> +					DHT11_EDGES_PREAMBLE - 2,
> +				timeres);
>  		if (ret)
>  			goto err;
>  	}
> @@ -277,7 +279,7 @@ static int dht11_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
> +	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
>  	dht11->num_edges = -1;
>  
>  	platform_set_drvdata(pdev, iio);
>