[PATCH 0/3] Provide new API to get the current time resolution

[PATCH 0/3] Provide new API to get the current time resolution

[Harald Geyer]

This patch series introduces a new function
u32 ktime_get_resolution_ns(void)
and shows how this would be used in a driver that greatly benefits from
it.

Since the iio subsystem has it's own wrapper function iio_get_time_ns()
for timestamps a similiar wrapper iio_get_time_resolution_ns() is
introduce in Patch 2 to keep the API consistent.

Patch 3 updates the example driver. Patch 3 depends on other fixes
currently only iio.git. If Patch 3 can't be picked up now then it will
be resent with more cleanup patches to dht11.c later - the main goal
for now is, to get the new code for timekeeping reviewed and merged.

Harald Geyer (3):
  timekeeping: Provide new API to get the current time resolution
  iio: Provide new API to get the current resolution of timestamps
  iio: dht11: Use new function iio_get_time_resolution_ns()

 drivers/iio/humidity/dht11.c |   33 +++++++++++++++++----------------
 include/linux/iio/iio.h      |    5 +++++
 include/linux/timekeeping.h  |    1 +
 kernel/time/timekeeping.c    |   18 ++++++++++++++++++
 4 files changed, 41 insertions(+), 16 deletions(-)

-- 
1.7.2.5

[PATCH 1/3] timekeeping: Provide new API to get the current time resolution

[Harald Geyer]

This has been tested on i386, sunxi and mxs.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 include/linux/timekeeping.h |    1 +
 kernel/time/timekeeping.c   |   18 ++++++++++++++++++
 2 files changed, 19 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..b823eba 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -586,6 +586,24 @@ 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

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

[John Stultz]

On Mon, Mar 2, 2015 at 12:56 PM, Harald Geyer <harald@ccbib.org> wrote:
> This has been tested on i386, sunxi and mxs.

So the patch needs to have the actual rational for inclusion here, not
in the cover-letter, since the cover-letter gets thrown away.


>
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
>  include/linux/timekeeping.h |    1 +
>  kernel/time/timekeeping.c   |   18 ++++++++++++++++++
>  2 files changed, 19 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..b823eba 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -586,6 +586,24 @@ 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);
> +

So is this always going to be an internal-only interface? I know in
the past folks have wanted clock_getres() to return similar hardware
granular resolution, but Thomas has pushed back against this due to
the fact that the resolution could fluctuate during the lifetime of a
system and there's no way to correlate a resolution returned from
clock_getres and a a value returns from clock_gettime.

So while for internal purposes, we could probably support something
like this, we really do need to make sure we're not exposing uselessly
racy interfaces to userland.

thanks
-john

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

[Harald Geyer]

John Stultz writes:
> On Mon, Mar 2, 2015 at 12:56 PM, Harald Geyer <harald@ccbib.org> wrote:
> > This has been tested on i386, sunxi and mxs.
> 
> So the patch needs to have the actual rational for inclusion here, not
> in the cover-letter, since the cover-letter gets thrown away.
 
Hm, to show the rational I included Patch 3 which is probably not
apt for the commit message. How about the following:

| 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.

> > Signed-off-by: Harald Geyer <harald@ccbib.org>
> > ---
> >  include/linux/timekeeping.h |    1 +
> >  kernel/time/timekeeping.c   |   18 ++++++++++++++++++
> >  2 files changed, 19 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..b823eba 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -586,6 +586,24 @@ 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);
> > +
> 
> So is this always going to be an internal-only interface?

As far as I'm concerned: yes.

> I know in
> the past folks have wanted clock_getres() to return similar hardware
> granular resolution, but Thomas has pushed back against this due to
> the fact that the resolution could fluctuate during the lifetime of a
> system and there's no way to correlate a resolution returned from
> clock_getres and a a value returns from clock_gettime.

I don't see why this would be a hard problem: Just have some counter
incremented every time the clock source changes and expose that counter
to userspace too. But I'm not interested in this. If anybody want's 
this information in userspace, it's theirs to make the case and propose
a solution that suits them.

> So while for internal purposes, we could probably support something
> like this, we really do need to make sure we're not exposing uselessly
> racy interfaces to userland.

Right now I report the resolution in the kernel log (if it is too low).
I think this is okay by your standards. Anything else you have in mind?

thanks,
Harald

[PATCH 2/3] iio: Provide new API to get the current resolution of timestamps

[Harald Geyer]

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
 include/linux/iio/iio.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 80d8550..2d352a0 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -286,6 +286,11 @@ static inline s64 iio_get_time_ns(void)
 	return ktime_get_real_ns();
 }
 
+static inline u32 iio_get_time_resolution_ns(void)
+{
+	return ktime_get_resolution_ns();
+}
+
 /* Device operating modes */
 #define INDIO_DIRECT_MODE		0x01
 #define INDIO_BUFFER_TRIGGERED		0x02
-- 
1.7.2.5

Re: [PATCH 2/3] iio: Provide new API to get the current resolution of timestamps

[Jonathan Cameron]

On 02/03/15 20:56, Harald Geyer wrote:
> Signed-off-by: Harald Geyer <harald@ccbib.org>
This use of the iio_get_timestamp call is very driver
specific. I'd be tempted not to wrap it at all and
to call your ktime_get_resolution_ns directly in the driver.
Arguably, don't use the wrapper on the timestamp either
as the intent of that is to provide a consistent way
of getting times that are reported to userspace via the
iio chrdevs.  I'm not entirely sure why a wrapper made
sense a long time ago for that use either though!

Jonathan
> ---
>  include/linux/iio/iio.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 80d8550..2d352a0 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -286,6 +286,11 @@ static inline s64 iio_get_time_ns(void)
>  	return ktime_get_real_ns();
>  }
>  
> +static inline u32 iio_get_time_resolution_ns(void)
> +{
> +	return ktime_get_resolution_ns();
> +}
> +
>  /* Device operating modes */
>  #define INDIO_DIRECT_MODE		0x01
>  #define INDIO_BUFFER_TRIGGERED		0x02
> 

[PATCH 3/3] iio: dht11: Use new function iio_get_time_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 |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 7d79a1a..29cffa1 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -87,23 +87,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");
@@ -170,10 +158,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()) {
+		timeres = iio_get_time_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 +208,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;
 	}
-- 
1.7.2.5