[PATCH v2 1/7] iio: adc: ina2xx: Remove bogus cast for data argument

[PATCH v2 1/7] iio: adc: ina2xx: Remove bogus cast for data argument

[Stefan Brüns]

iio_push_to_buffers_with_timestamp expects a void pointer, so the cast
is both unnecessary and misleading.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---

Changes in v2: None

 drivers/iio/adc/ina2xx-adc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index ddf878163bf9..3195f8754c3b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -767,8 +767,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 
 	time_b = iio_get_time_ns(indio_dev);
 
-	iio_push_to_buffers_with_timestamp(indio_dev,
-					   (unsigned int *)data, time_a);
+	iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
 
 	return (unsigned long)(time_b - time_a) / 1000;
 };
-- 
2.15.1

[PATCH v2 2/7] iio: adc: ina2xx: Clarify size requirement for data buffer

[Stefan Brüns]

The timestamp is inserted into the buffer after the sample data by
iio_push_to_buffers_with_timestamp, document the space requirement for
the timestamp.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---

Changes in v2: None

 drivers/iio/adc/ina2xx-adc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 3195f8754c3b..8c8120406f52 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -700,7 +700,8 @@ static const struct iio_chan_spec ina219_channels[] = {
 static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 {
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
-	unsigned short data[8];
+	/* data buffer needs space for channel data and timestap */
+	unsigned short data[4 + sizeof(s64)/sizeof(short)];
 	int bit, ret, i = 0;
 	s64 time_a, time_b;
 	unsigned int alert;
-- 
2.15.1

[PATCH v2 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag

[Stefan Brüns]

Although the datasheet states the CNVR flag is cleared by reading the
BUS_VOLTAGE register, it is actually cleared by reading any of the
voltage/current/power registers.

The behaviour has been confirmed by TI support:
http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053/2378282

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---

Changes in v2: None

 drivers/iio/adc/ina2xx-adc.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 8c8120406f52..b027d485398b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -705,7 +705,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 	int bit, ret, i = 0;
 	s64 time_a, time_b;
 	unsigned int alert;
-	int cnvr_need_clear = 0;
 
 	time_a = iio_get_time_ns(indio_dev);
 
@@ -730,7 +729,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 				ret = regmap_read(chip->regmap,
 						  INA2XX_BUS_VOLTAGE, &alert);
 				alert &= INA219_CNVR;
-				cnvr_need_clear = alert;
 			}
 
 			if (ret < 0)
@@ -752,18 +750,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 			return ret;
 
 		data[i++] = val;
-
-		if (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_POWER)
-			cnvr_need_clear = 0;
-	}
-
-	/* Dummy read on INA219 power register to clear CNVR flag */
-	if (cnvr_need_clear && chip->config->chip_id == ina219) {
-		unsigned int val;
-
-		ret = regmap_read(chip->regmap, INA2XX_POWER, &val);
-		if (ret < 0)
-			return ret;
 	}
 
 	time_b = iio_get_time_ns(indio_dev);
-- 
2.15.1

[PATCH v2 4/7] iio: adc: ina2xx: Do not udelay for several seconds

[Stefan Brüns]

The conversion time can be up to 16 seconds (8 ms per channel, 2 channels,
1024 times averaging).

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---

Changes in v2: None

 drivers/iio/adc/ina2xx-adc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index b027d485398b..2621a34ee5c6 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -764,7 +764,7 @@ static int ina2xx_capture_thread(void *data)
 	struct iio_dev *indio_dev = data;
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
 	int sampling_us = SAMPLING_PERIOD(chip);
-	int buffer_us;
+	int buffer_us, delay_us;
 
 	/*
 	 * Poll a bit faster than the chip internal Fs, in case
@@ -778,8 +778,10 @@ static int ina2xx_capture_thread(void *data)
 		if (buffer_us < 0)
 			return buffer_us;
 
-		if (sampling_us > buffer_us)
-			udelay(sampling_us - buffer_us);
+		if (sampling_us > buffer_us) {
+			delay_us = sampling_us - buffer_us;
+			usleep_range(delay_us, (delay_us * 3) >> 1);
+		}
 
 	} while (!kthread_should_stop());
 
-- 
2.15.1

[PATCH v2 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation

[Stefan Brüns]

The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.

Use a monotonic timestamp to track the time for the next poll iteration.
The timestamp is advanced by the sampling interval each iteration. In case
the conversion overrruns the register readout (i.e. fast sampling combined
with a slow bus), one or multiple samples will be dropped.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

---

Changes in v2:
- add a comment mentioning skipping samples on overrun

 drivers/iio/adc/ina2xx-adc.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..415e53b5c0a6 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 	/* data buffer needs space for channel data and timestap */
 	unsigned short data[4 + sizeof(s64)/sizeof(short)];
 	int bit, ret, i = 0;
-	s64 time_a, time_b;
+	s64 time;
 	unsigned int alert;
 
-	time_a = iio_get_time_ns(indio_dev);
+	time = iio_get_time_ns(indio_dev);
 
 	/*
 	 * Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 		data[i++] = val;
 	}
 
-	time_b = iio_get_time_ns(indio_dev);
+	iio_push_to_buffers_with_timestamp(indio_dev, data, time);
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
-	return (unsigned long)(time_b - time_a) / 1000;
+	return 0;
 };
 
 static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
 	struct iio_dev *indio_dev = data;
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
 	int sampling_us = SAMPLING_PERIOD(chip);
-	int buffer_us, delay_us;
+	int ret;
+	struct timespec64 next, now, delta;
+	s64 delay_us;
 
 	/*
 	 * Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
 	if (!chip->allow_async_readout)
 		sampling_us -= 200;
 
+	ktime_get_ts64(&next);
+
 	do {
-		buffer_us = ina2xx_work_buffer(indio_dev);
-		if (buffer_us < 0)
-			return buffer_us;
+		ret = ina2xx_work_buffer(indio_dev);
+		if (ret < 0)
+			return ret;
 
-		if (sampling_us > buffer_us) {
-			delay_us = sampling_us - buffer_us;
-			usleep_range(delay_us, (delay_us * 3) >> 1);
-		}
+		ktime_get_ts64(&now);
+
+		/*
+		 * Advance the timestamp for the next poll by one sampling
+		 * interval, and sleep for the remainder (next - now).
+		 * In case "next" has already passed, the interval is added
+		 * multiple times, i.e. samples are dropped.
+		 */
+		do {
+			timespec64_add_ns(&next, 1000 * sampling_us);
+			delta = timespec64_sub(next, now);
+			delay_us = timespec64_to_ns(&delta) / 1000;
+		} while (delay_us <= 0);
+
+		usleep_range(delay_us, (delay_us * 3) >> 1);
 
 	} while (!kthread_should_stop());
 
-- 
2.15.1

[PATCH v2 6/7] iio: adc: ina2xx: Align timestamp with conversion ready flag

[Stefan Brüns]

As the timestamp is no longer (ab-)used to measure the function run time,
it can be taken at the correct time, i.e. when the conversion has finished.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---

Changes in v2: None

 drivers/iio/adc/ina2xx-adc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 415e53b5c0a6..b7407ac91b59 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -706,8 +706,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 	s64 time;
 	unsigned int alert;
 
-	time = iio_get_time_ns(indio_dev);
-
 	/*
 	 * Because the timer thread and the chip conversion clock
 	 * are asynchronous, the period difference will eventually
@@ -736,6 +734,8 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 
 		} while (!alert);
 
+	time = iio_get_time_ns(indio_dev);
+
 	/*
 	 * Single register reads: bulk_read will not work with ina226/219
 	 * as there is no auto-increment of the register pointer.
-- 
2.15.1

[PATCH v2 7/7] iio: adc: ina2xx: Actually align the loop with the conversion ready flag

[Stefan Brüns]

Currently, the registers are read out once per conversion interval. If
the reading is delayed as the conversion has not yet finished, this extra
time is treated as being part of the readout, although it should delay
the start of the poll interval. This results in the interval starting
slightly earlier in each iteration, until all time between reads is
spent polling the status registers instead of sleeping.

To fix this, the delay has to account for the state of the conversion
ready flag. Whenever the conversion is already finished, schedule the next
read on the regular interval, otherwise schedule it one interval after the
flag bit has been set.

Split the work function in two functions, one for the status poll and one
for reading the values, to be able to note down the time when the flag
bit is raised.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

---

Changes in v2:
- Describe old behaviour in commit message more clearly

 drivers/iio/adc/ina2xx-adc.c | 57 +++++++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index b7407ac91b59..80af0d2322a3 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -697,13 +697,10 @@ static const struct iio_chan_spec ina219_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
-static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+static int ina2xx_conversion_ready(struct iio_dev *indio_dev)
 {
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
-	/* data buffer needs space for channel data and timestap */
-	unsigned short data[4 + sizeof(s64)/sizeof(short)];
-	int bit, ret, i = 0;
-	s64 time;
+	int ret;
 	unsigned int alert;
 
 	/*
@@ -717,22 +714,29 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 	 * For now, we do an extra read of the MASK_ENABLE register (INA226)
 	 * resp. the BUS_VOLTAGE register (INA219).
 	 */
-	if (!chip->allow_async_readout)
-		do {
-			if (chip->config->chip_id == ina226) {
-				ret = regmap_read(chip->regmap,
-						  INA226_MASK_ENABLE, &alert);
-				alert &= INA226_CVRF;
-			} else {
-				ret = regmap_read(chip->regmap,
-						  INA2XX_BUS_VOLTAGE, &alert);
-				alert &= INA219_CNVR;
-			}
+	if (chip->config->chip_id == ina226) {
+		ret = regmap_read(chip->regmap,
+				  INA226_MASK_ENABLE, &alert);
+		alert &= INA226_CVRF;
+	} else {
+		ret = regmap_read(chip->regmap,
+				  INA2XX_BUS_VOLTAGE, &alert);
+		alert &= INA219_CNVR;
+	}
 
-			if (ret < 0)
-				return ret;
+	if (ret < 0)
+		return ret;
 
-		} while (!alert);
+	return !!alert;
+}
+
+static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+{
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	/* data buffer needs space for channel data and timestap */
+	unsigned short data[4 + sizeof(s64)/sizeof(short)];
+	int bit, ret, i = 0;
+	s64 time;
 
 	time = iio_get_time_ns(indio_dev);
 
@@ -776,6 +780,21 @@ static int ina2xx_capture_thread(void *data)
 	ktime_get_ts64(&next);
 
 	do {
+		while (!chip->allow_async_readout) {
+			ret = ina2xx_conversion_ready(indio_dev);
+			if (ret < 0)
+				return ret;
+
+			/*
+			 * If the conversion was not yet finished,
+			 * reset the reference timestamp.
+			 */
+			if (ret == 0)
+				ktime_get_ts64(&next);
+			else
+				break;
+		}
+
 		ret = ina2xx_work_buffer(indio_dev);
 		if (ret < 0)
 			return ret;
-- 
2.15.1

Re: [PATCH v2 1/7] iio: adc: ina2xx: Remove bogus cast for data argument

[Jonathan Cameron]

On Thu, 21 Dec 2017 19:31:32 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> iio_push_to_buffers_with_timestamp expects a void pointer, so the cast
> is both unnecessary and misleading.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
Hi Stefan,

I applied the first few patches the first time around.
Please drop them from new versions - if nothing else 
if confuses me ;)

> ---
> 
> Changes in v2: None
> 
>  drivers/iio/adc/ina2xx-adc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index ddf878163bf9..3195f8754c3b 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -767,8 +767,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  
>  	time_b = iio_get_time_ns(indio_dev);
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev,
> -					   (unsigned int *)data, time_a);
> +	iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
>  
>  	return (unsigned long)(time_b - time_a) / 1000;
>  };

Re: [PATCH v2 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation

[Jonathan Cameron]

On Thu, 21 Dec 2017 19:31:36 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> The iio timestamp clock is user selectable and may be non-monotonic. Also,
> only part of the acquisition time is measured, thus the delay was longer
> than intended.
> 
> Use a monotonic timestamp to track the time for the next poll iteration.
> The timestamp is advanced by the sampling interval each iteration. In case
> the conversion overrruns the register readout (i.e. fast sampling combined
> with a slow bus), one or multiple samples will be dropped.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> 
> ---
> 
> Changes in v2:
> - add a comment mentioning skipping samples on overrun
> 
>  drivers/iio/adc/ina2xx-adc.c | 41 +++++++++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 2621a34ee5c6..415e53b5c0a6 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  	/* data buffer needs space for channel data and timestap */
>  	unsigned short data[4 + sizeof(s64)/sizeof(short)];
>  	int bit, ret, i = 0;
> -	s64 time_a, time_b;
> +	s64 time;
>  	unsigned int alert;
>  
> -	time_a = iio_get_time_ns(indio_dev);
> +	time = iio_get_time_ns(indio_dev);
>  
>  	/*
>  	 * Because the timer thread and the chip conversion clock
> @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  		data[i++] = val;
>  	}
>  
> -	time_b = iio_get_time_ns(indio_dev);
> +	iio_push_to_buffers_with_timestamp(indio_dev, data, time);
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
> -
> -	return (unsigned long)(time_b - time_a) / 1000;
> +	return 0;
>  };
>  
>  static int ina2xx_capture_thread(void *data)
> @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
>  	struct iio_dev *indio_dev = data;
>  	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>  	int sampling_us = SAMPLING_PERIOD(chip);
> -	int buffer_us, delay_us;
> +	int ret;
> +	struct timespec64 next, now, delta;
> +	s64 delay_us;
>  
>  	/*
>  	 * Poll a bit faster than the chip internal Fs, in case
> @@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
>  	if (!chip->allow_async_readout)
>  		sampling_us -= 200;
>  
> +	ktime_get_ts64(&next);
> +
>  	do {
> -		buffer_us = ina2xx_work_buffer(indio_dev);
> -		if (buffer_us < 0)
> -			return buffer_us;
> +		ret = ina2xx_work_buffer(indio_dev);
> +		if (ret < 0)
> +			return ret;
>  
> -		if (sampling_us > buffer_us) {
> -			delay_us = sampling_us - buffer_us;
> -			usleep_range(delay_us, (delay_us * 3) >> 1);
> -		}
> +		ktime_get_ts64(&now);
> +
> +		/*
> +		 * Advance the timestamp for the next poll by one sampling
> +		 * interval, and sleep for the remainder (next - now).
> +		 * In case "next" has already passed, the interval is added
> +		 * multiple times, i.e. samples are dropped.
> +		 */
> +		do {
> +			timespec64_add_ns(&next, 1000 * sampling_us);
> +			delta = timespec64_sub(next, now);
> +			delay_us = timespec64_to_ns(&delta) / 1000;
> +		} while (delay_us <= 0);
> +
> +		usleep_range(delay_us, (delay_us * 3) >> 1);
>  
>  	} while (!kthread_should_stop());
>  

Re: [PATCH v2 6/7] iio: adc: ina2xx: Align timestamp with conversion ready flag

[Jonathan Cameron]

On Thu, 21 Dec 2017 19:31:37 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> As the timestamp is no longer (ab-)used to measure the function run time,
> it can be taken at the correct time, i.e. when the conversion has finished.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
Applied.

Thanks,

Jonathan

> ---
> 
> Changes in v2: None
> 
>  drivers/iio/adc/ina2xx-adc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 415e53b5c0a6..b7407ac91b59 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -706,8 +706,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  	s64 time;
>  	unsigned int alert;
>  
> -	time = iio_get_time_ns(indio_dev);
> -
>  	/*
>  	 * Because the timer thread and the chip conversion clock
>  	 * are asynchronous, the period difference will eventually
> @@ -736,6 +734,8 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  
>  		} while (!alert);
>  
> +	time = iio_get_time_ns(indio_dev);
> +
>  	/*
>  	 * Single register reads: bulk_read will not work with ina226/219
>  	 * as there is no auto-increment of the register pointer.

Re: [PATCH v2 7/7] iio: adc: ina2xx: Actually align the loop with the conversion ready flag

[Jonathan Cameron]

On Thu, 21 Dec 2017 19:31:38 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> Currently, the registers are read out once per conversion interval. If
> the reading is delayed as the conversion has not yet finished, this extra
> time is treated as being part of the readout, although it should delay
> the start of the poll interval. This results in the interval starting
> slightly earlier in each iteration, until all time between reads is
> spent polling the status registers instead of sleeping.
> 
> To fix this, the delay has to account for the state of the conversion
> ready flag. Whenever the conversion is already finished, schedule the next
> read on the regular interval, otherwise schedule it one interval after the
> flag bit has been set.
> 
> Split the work function in two functions, one for the status poll and one
> for reading the values, to be able to note down the time when the flag
> bit is raised.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
Great thanks.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> 
> ---
> 
> Changes in v2:
> - Describe old behaviour in commit message more clearly
> 
>  drivers/iio/adc/ina2xx-adc.c | 57 +++++++++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index b7407ac91b59..80af0d2322a3 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -697,13 +697,10 @@ static const struct iio_chan_spec ina219_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
> -static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> +static int ina2xx_conversion_ready(struct iio_dev *indio_dev)
>  {
>  	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> -	/* data buffer needs space for channel data and timestap */
> -	unsigned short data[4 + sizeof(s64)/sizeof(short)];
> -	int bit, ret, i = 0;
> -	s64 time;
> +	int ret;
>  	unsigned int alert;
>  
>  	/*
> @@ -717,22 +714,29 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  	 * For now, we do an extra read of the MASK_ENABLE register (INA226)
>  	 * resp. the BUS_VOLTAGE register (INA219).
>  	 */
> -	if (!chip->allow_async_readout)
> -		do {
> -			if (chip->config->chip_id == ina226) {
> -				ret = regmap_read(chip->regmap,
> -						  INA226_MASK_ENABLE, &alert);
> -				alert &= INA226_CVRF;
> -			} else {
> -				ret = regmap_read(chip->regmap,
> -						  INA2XX_BUS_VOLTAGE, &alert);
> -				alert &= INA219_CNVR;
> -			}
> +	if (chip->config->chip_id == ina226) {
> +		ret = regmap_read(chip->regmap,
> +				  INA226_MASK_ENABLE, &alert);
> +		alert &= INA226_CVRF;
> +	} else {
> +		ret = regmap_read(chip->regmap,
> +				  INA2XX_BUS_VOLTAGE, &alert);
> +		alert &= INA219_CNVR;
> +	}
>  
> -			if (ret < 0)
> -				return ret;
> +	if (ret < 0)
> +		return ret;
>  
> -		} while (!alert);
> +	return !!alert;
> +}
> +
> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	/* data buffer needs space for channel data and timestap */
> +	unsigned short data[4 + sizeof(s64)/sizeof(short)];
> +	int bit, ret, i = 0;
> +	s64 time;
>  
>  	time = iio_get_time_ns(indio_dev);
>  
> @@ -776,6 +780,21 @@ static int ina2xx_capture_thread(void *data)
>  	ktime_get_ts64(&next);
>  
>  	do {
> +		while (!chip->allow_async_readout) {
> +			ret = ina2xx_conversion_ready(indio_dev);
> +			if (ret < 0)
> +				return ret;
> +
> +			/*
> +			 * If the conversion was not yet finished,
> +			 * reset the reference timestamp.
> +			 */
> +			if (ret == 0)
> +				ktime_get_ts64(&next);
> +			else
> +				break;
> +		}
> +
>  		ret = ina2xx_work_buffer(indio_dev);
>  		if (ret < 0)
>  			return ret;

[PATCH v3 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation

[Stefan Brüns]

The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.

Use a monotonic timestamp to track the time for the next poll iteration.
The timestamp is advanced by the sampling interval each iteration. In case
the conversion overrruns the register readout (i.e. fast sampling combined
with a slow bus), one or multiple samples will be dropped.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

---

Changes in v3:
- use s64_div for 64 bit integer division to fix compiles on 32 bit archs

Changes in v2:
- add a comment mentioning skipping samples on overrun

 drivers/iio/adc/ina2xx-adc.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..64e9e8e1ad70 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 	/* data buffer needs space for channel data and timestap */
 	unsigned short data[4 + sizeof(s64)/sizeof(short)];
 	int bit, ret, i = 0;
-	s64 time_a, time_b;
+	s64 time;
 	unsigned int alert;
 
-	time_a = iio_get_time_ns(indio_dev);
+	time = iio_get_time_ns(indio_dev);
 
 	/*
 	 * Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 		data[i++] = val;
 	}
 
-	time_b = iio_get_time_ns(indio_dev);
+	iio_push_to_buffers_with_timestamp(indio_dev, data, time);
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
-	return (unsigned long)(time_b - time_a) / 1000;
+	return 0;
 };
 
 static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
 	struct iio_dev *indio_dev = data;
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
 	int sampling_us = SAMPLING_PERIOD(chip);
-	int buffer_us, delay_us;
+	int ret;
+	struct timespec64 next, now, delta;
+	s64 delay_us;
 
 	/*
 	 * Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
 	if (!chip->allow_async_readout)
 		sampling_us -= 200;
 
+	ktime_get_ts64(&next);
+
 	do {
-		buffer_us = ina2xx_work_buffer(indio_dev);
-		if (buffer_us < 0)
-			return buffer_us;
+		ret = ina2xx_work_buffer(indio_dev);
+		if (ret < 0)
+			return ret;
 
-		if (sampling_us > buffer_us) {
-			delay_us = sampling_us - buffer_us;
-			usleep_range(delay_us, (delay_us * 3) >> 1);
-		}
+		ktime_get_ts64(&now);
+
+		/*
+		 * Advance the timestamp for the next poll by one sampling
+		 * interval, and sleep for the remainder (next - now)
+		 * In case "next" has already passed, the interval is added
+		 * multiple times, i.e. samples are dropped.
+		 */
+		do {
+			timespec64_add_ns(&next, 1000 * sampling_us);
+			delta = timespec64_sub(next, now);
+			delay_us = s64_div(timespec64_to_ns(&delta), 1000);
+		} while (delay_us <= 0);
+
+		usleep_range(delay_us, (delay_us * 3) >> 1);
 
 	} while (!kthread_should_stop());
 
-- 
2.15.1

[PATCH v4 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation

[Stefan Brüns]

The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.

Use a monotonic timestamp to track the time for the next poll iteration.
The timestamp is advanced by the sampling interval each iteration. In case
the conversion overrruns the register readout (i.e. fast sampling combined
with a slow bus), one or multiple samples will be dropped.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

---

Changes in v4:
- Typo, div_s64, not s64_div

Changes in v3:
- use s64_div for 64 bit integer division to fix compiles on 32 bit archs

Changes in v2:
- add a comment mentioning skipping samples on overrun

 drivers/iio/adc/ina2xx-adc.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..b55b8bd1427b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 	/* data buffer needs space for channel data and timestap */
 	unsigned short data[4 + sizeof(s64)/sizeof(short)];
 	int bit, ret, i = 0;
-	s64 time_a, time_b;
+	s64 time;
 	unsigned int alert;
 
-	time_a = iio_get_time_ns(indio_dev);
+	time = iio_get_time_ns(indio_dev);
 
 	/*
 	 * Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 		data[i++] = val;
 	}
 
-	time_b = iio_get_time_ns(indio_dev);
+	iio_push_to_buffers_with_timestamp(indio_dev, data, time);
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
-	return (unsigned long)(time_b - time_a) / 1000;
+	return 0;
 };
 
 static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
 	struct iio_dev *indio_dev = data;
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
 	int sampling_us = SAMPLING_PERIOD(chip);
-	int buffer_us, delay_us;
+	int ret;
+	struct timespec64 next, now, delta;
+	s64 delay_us;
 
 	/*
 	 * Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
 	if (!chip->allow_async_readout)
 		sampling_us -= 200;
 
+	ktime_get_ts64(&next);
+
 	do {
-		buffer_us = ina2xx_work_buffer(indio_dev);
-		if (buffer_us < 0)
-			return buffer_us;
+		ret = ina2xx_work_buffer(indio_dev);
+		if (ret < 0)
+			return ret;
 
-		if (sampling_us > buffer_us) {
-			delay_us = sampling_us - buffer_us;
-			usleep_range(delay_us, (delay_us * 3) >> 1);
-		}
+		ktime_get_ts64(&now);
+
+		/*
+		 * Advance the timestamp for the next poll by one sampling
+		 * interval, and sleep for the remainder (next - now)
+		 * In case "next" has already passed, the interval is added
+		 * multiple times, i.e. samples are dropped.
+		 */
+		do {
+			timespec64_add_ns(&next, 1000 * sampling_us);
+			delta = timespec64_sub(next, now);
+			delay_us = div_s64(timespec64_to_ns(&delta), 1000);
+		} while (delay_us <= 0);
+
+		usleep_range(delay_us, (delay_us * 3) >> 1);
 
 	} while (!kthread_should_stop());
 
-- 
2.15.1

Re: [PATCH v4 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation

[Jonathan Cameron]

On Mon, 1 Jan 2018 02:24:42 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> The iio timestamp clock is user selectable and may be non-monotonic. Also,
> only part of the acquisition time is measured, thus the delay was longer
> than intended.
> 
> Use a monotonic timestamp to track the time for the next poll iteration.
> The timestamp is advanced by the sampling interval each iteration. In case
> the conversion overrruns the register readout (i.e. fast sampling combined
> with a slow bus), one or multiple samples will be dropped.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
Replaced v3

Thanks,

Jonathan
> 
> ---
> 
> Changes in v4:
> - Typo, div_s64, not s64_div
> 
> Changes in v3:
> - use s64_div for 64 bit integer division to fix compiles on 32 bit archs
> 
> Changes in v2:
> - add a comment mentioning skipping samples on overrun
> 
>  drivers/iio/adc/ina2xx-adc.c | 41 +++++++++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 2621a34ee5c6..b55b8bd1427b 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  	/* data buffer needs space for channel data and timestap */
>  	unsigned short data[4 + sizeof(s64)/sizeof(short)];
>  	int bit, ret, i = 0;
> -	s64 time_a, time_b;
> +	s64 time;
>  	unsigned int alert;
>  
> -	time_a = iio_get_time_ns(indio_dev);
> +	time = iio_get_time_ns(indio_dev);
>  
>  	/*
>  	 * Because the timer thread and the chip conversion clock
> @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  		data[i++] = val;
>  	}
>  
> -	time_b = iio_get_time_ns(indio_dev);
> +	iio_push_to_buffers_with_timestamp(indio_dev, data, time);
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
> -
> -	return (unsigned long)(time_b - time_a) / 1000;
> +	return 0;
>  };
>  
>  static int ina2xx_capture_thread(void *data)
> @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
>  	struct iio_dev *indio_dev = data;
>  	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>  	int sampling_us = SAMPLING_PERIOD(chip);
> -	int buffer_us, delay_us;
> +	int ret;
> +	struct timespec64 next, now, delta;
> +	s64 delay_us;
>  
>  	/*
>  	 * Poll a bit faster than the chip internal Fs, in case
> @@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
>  	if (!chip->allow_async_readout)
>  		sampling_us -= 200;
>  
> +	ktime_get_ts64(&next);
> +
>  	do {
> -		buffer_us = ina2xx_work_buffer(indio_dev);
> -		if (buffer_us < 0)
> -			return buffer_us;
> +		ret = ina2xx_work_buffer(indio_dev);
> +		if (ret < 0)
> +			return ret;
>  
> -		if (sampling_us > buffer_us) {
> -			delay_us = sampling_us - buffer_us;
> -			usleep_range(delay_us, (delay_us * 3) >> 1);
> -		}
> +		ktime_get_ts64(&now);
> +
> +		/*
> +		 * Advance the timestamp for the next poll by one sampling
> +		 * interval, and sleep for the remainder (next - now)
> +		 * In case "next" has already passed, the interval is added
> +		 * multiple times, i.e. samples are dropped.
> +		 */
> +		do {
> +			timespec64_add_ns(&next, 1000 * sampling_us);
> +			delta = timespec64_sub(next, now);
> +			delay_us = div_s64(timespec64_to_ns(&delta), 1000);
> +		} while (delay_us <= 0);
> +
> +		usleep_range(delay_us, (delay_us * 3) >> 1);
>  
>  	} while (!kthread_should_stop());
>