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

[PATCH v1 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>
---

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

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

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

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

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

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

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..65bd9e69faf2 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,22 @@ 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);
+
+		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 v1 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>
---

 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 65bd9e69faf2..0a6745e15a5d 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 v1 7/7] iio: adc: ina2xx: Actually align the loop with the conversion ready flag

[Stefan Brüns]

Currently, the logic just shifts the interval duration between the delay
and the polling, but never aligns to 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>

---

 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 0a6745e15a5d..728cc109a61c 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 v1 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag

[Jonathan Cameron]

On Fri, 8 Dec 2017 18:41:48 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

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

I haven't checked the code thoroughly so there may well be something stopping it
but have you checked the case where the only channel enabled is the timestamp?

Obviously it makes little sense, but IIRC there is nothing in the core preventing
that happening.

Jonathan
> ---
> 
>  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);

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

[Jonathan Cameron]

On Fri, 8 Dec 2017 18:41:50 +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.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
> 
>  drivers/iio/adc/ina2xx-adc.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 2621a34ee5c6..65bd9e69faf2 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,22 @@ 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);
> +
> +		do {
> +			timespec64_add_ns(&next, 1000 * sampling_us);
> +			delta = timespec64_sub(next, now);
> +			delay_us = timespec64_to_ns(&delta) / 1000;
> +		} while (delay_us <= 0);

Umm. I'm lost, what is the purpose of the above dance?
A comment perhaps.

> +
> +		usleep_range(delay_us, (delay_us * 3) >> 1);
>  
>  	} while (!kthread_should_stop());
>  

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

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 4145 bytes --]

On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote:
> On Fri, 8 Dec 2017 18:41:50 +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.
> > 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > ---
> > 
> >  drivers/iio/adc/ina2xx-adc.c | 35 +++++++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > index 2621a34ee5c6..65bd9e69faf2 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,22 @@ 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);
> > +
> > +		do {
> > +			timespec64_add_ns(&next, 1000 * sampling_us);
> > +			delta = timespec64_sub(next, now);
> > +			delay_us = timespec64_to_ns(&delta) / 1000;
> > +		} while (delay_us <= 0);
> 
> Umm. I'm lost, what is the purpose of the above dance?
> A comment perhaps.

next is the timestamp for the next read to happen, now is the current time. 
Obviously we have to sleep for the remainder.

Each sampling interval the "next" timestamp is pushed back by sampling_us. 
Normally this happens exactly once per read, i.e. we schedule the reads to 
happen exactly each sampling interval.

The sampling inteval is *only* added multiple times if it is faster than the 
bus can deliver the data (at 100 kBits/s, each register read takes about 400 
us, so sampling faster than every ~1 ms is not possible.

The old code measured the time spent for reading the registers and slept for 
the remainder of the interval. This way the sampling drifts, as there is some 
time not accounted for - usleep_range, function call overhead, kthread 
interrupted.

Using a timestamp avoids the drift. It also allows simple readjustment of the 
"next" sampling time when polling the status register.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote:
> On Fri, 8 Dec 2017 18:41:48 +0100
> 
> Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> > 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>
> 
> I haven't checked the code thoroughly so there may well be something
> stopping it but have you checked the case where the only channel enabled is
> the timestamp?
> 
> Obviously it makes little sense, but IIRC there is nothing in the core
> preventing that happening.

The timestamp is completely unrelated to the status register, so I fail to 
understand your question. Can you please clarify?

This only removes a redundant read.

All channel combinations (w/ and w/o timestamp) work, but combinations not 
including the power register use less bus time now.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Jonathan Cameron]

On Sun, 10 Dec 2017 21:53:42 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote:
> > On Fri, 8 Dec 2017 18:41:48 +0100
> > 
> > Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:  
> > > 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>  
> > 
> > I haven't checked the code thoroughly so there may well be something
> > stopping it but have you checked the case where the only channel enabled is
> > the timestamp?
> > 
> > Obviously it makes little sense, but IIRC there is nothing in the core
> > preventing that happening.  
> 
> The timestamp is completely unrelated to the status register, so I fail to 
> understand your question. Can you please clarify?

If you only have a timestamp, the trigger will still fire (I think)
but you'll do no reading at all from the device.  If configured in this,
admittedly odd, way you should just get a stream of timestamps with no
data.

> 
> This only removes a redundant read.
The question is whether it is redundant if we have no non timestamp
registers enabled.

I'll be honest, whilst I can't immediately spot any protection against
this in the core (and it definitely used to be possible), I'm not totally
sure it now is and don't have a system to hand to test against.

We had some debate a long time back on whether it made sense to have
only timestamps and I think we concluded it did as you might in theory
only care about the timing and not the data in some obscure cases.

Jonathan
 
> 
> All channel combinations (w/ and w/o timestamp) work, but combinations not 
> including the power register use less bus time now.
> 
> Kind regards,
> 
> Stefan
> 

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

[Jonathan Cameron]

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

> On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote:
> > On Fri, 8 Dec 2017 18:41:50 +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.
> > > 
> > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > ---
> > > 
> > >  drivers/iio/adc/ina2xx-adc.c | 35 +++++++++++++++++++++--------------
> > >  1 file changed, 21 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > > index 2621a34ee5c6..65bd9e69faf2 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,22 @@ 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);
> > > +
> > > +		do {
> > > +			timespec64_add_ns(&next, 1000 * sampling_us);
> > > +			delta = timespec64_sub(next, now);
> > > +			delay_us = timespec64_to_ns(&delta) / 1000;
> > > +		} while (delay_us <= 0);  
> > 
> > Umm. I'm lost, what is the purpose of the above dance?
> > A comment perhaps.  
> 
> next is the timestamp for the next read to happen, now is the current time. 
> Obviously we have to sleep for the remainder.
> 
> Each sampling interval the "next" timestamp is pushed back by sampling_us. 
> Normally this happens exactly once per read, i.e. we schedule the reads to 
> happen exactly each sampling interval.
> 
> The sampling inteval is *only* added multiple times if it is faster than the 
> bus can deliver the data (at 100 kBits/s, each register read takes about 400 
> us, so sampling faster than every ~1 ms is not possible.

So this is deliberately skipping a sample if this happens?  It was this
element that I wasn't understanding previously. 
Add a comment in the code to explain this and I'm happy. It's horrible,
but not much we can do if things are simply going too fast.

Thanks for the info.

Jonathan
> 
> The old code measured the time spent for reading the registers and slept for 
> the remainder of the interval. This way the sampling drifts, as there is some 
> time not accounted for - usleep_range, function call overhead, kthread 
> interrupted.
> 
> Using a timestamp avoids the drift. It also allows simple readjustment of the 
> "next" sampling time when polling the status register.
> 
> Kind regards,
> 
> Stefan
> 

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

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

On Tuesday, December 12, 2017 9:15:30 PM CET Jonathan Cameron wrote:
> On Sun, 10 Dec 2017 21:53:42 +0100
> 
> Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> > On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote:
> > > On Fri, 8 Dec 2017 18:41:48 +0100
> > > 
> > > Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> > > > 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/64
> > > > 7053
> > > > /2378282
> > > > 
> > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > 
> > > I haven't checked the code thoroughly so there may well be something
> > > stopping it but have you checked the case where the only channel enabled
> > > is
> > > the timestamp?
> > > 
> > > Obviously it makes little sense, but IIRC there is nothing in the core
> > > preventing that happening.
> > 
> > The timestamp is completely unrelated to the status register, so I fail to
> > understand your question. Can you please clarify?
> 
> If you only have a timestamp, the trigger will still fire (I think)
> but you'll do no reading at all from the device.  If configured in this,
> admittedly odd, way you should just get a stream of timestamps with no
> data.

If there are reads depends on the mode - if running asynchronously, it will 
just stream out 64 bits of timestamp each interval. In synchronous mode, the 
driver will read the status register (low bits of bus voltage register for 
INA219, msk register for INA226), which implicitly clears the CNVR flag.
 
> > This only removes a redundant read.
> 
> The question is whether it is redundant if we have no non timestamp
> registers enabled.

According to the documentation, INA219 and 226 had to be treated differently. 
As it turned out, both actually behave the same way regarding the CNVR flag, 
so we just poll the status register, which for both devices clears the flag.

Regards,

Stefan


-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Jonathan Cameron]

On Fri, 8 Dec 2017 18:41:46 +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>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> ---
> 
>  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 v1 2/7] iio: adc: ina2xx: Clarify size requirement for data buffer

[Jonathan Cameron]

On Fri, 8 Dec 2017 18:41:47 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> 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>
Applied and pushed out as testing for the autobuilders to play with it.
Thanks,

Jonathan
> ---
> 
>  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;

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

[Jonathan Cameron]

On Wed, 13 Dec 2017 00:48:40 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> On Tuesday, December 12, 2017 9:15:30 PM CET Jonathan Cameron wrote:
> > On Sun, 10 Dec 2017 21:53:42 +0100
> > 
> > Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:  
> > > On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote:  
> > > > On Fri, 8 Dec 2017 18:41:48 +0100
> > > > 
> > > > Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:  
> > > > > 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/64
> > > > > 7053
> > > > > /2378282
> > > > > 
> > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>  
> > > > 
> > > > I haven't checked the code thoroughly so there may well be something
> > > > stopping it but have you checked the case where the only channel enabled
> > > > is
> > > > the timestamp?
> > > > 
> > > > Obviously it makes little sense, but IIRC there is nothing in the core
> > > > preventing that happening.  
> > > 
> > > The timestamp is completely unrelated to the status register, so I fail to
> > > understand your question. Can you please clarify?  
> > 
> > If you only have a timestamp, the trigger will still fire (I think)
> > but you'll do no reading at all from the device.  If configured in this,
> > admittedly odd, way you should just get a stream of timestamps with no
> > data.  
> 
> If there are reads depends on the mode - if running asynchronously, it will 
> just stream out 64 bits of timestamp each interval. In synchronous mode, the 
> driver will read the status register (low bits of bus voltage register for 
> INA219, msk register for INA226), which implicitly clears the CNVR flag.
>  
> > > This only removes a redundant read.  
> > 
> > The question is whether it is redundant if we have no non timestamp
> > registers enabled.  
> 
> According to the documentation, INA219 and 226 had to be treated differently. 
> As it turned out, both actually behave the same way regarding the CNVR flag, 
> so we just poll the status register, which for both devices clears the flag.
Ah, fine then. I thought we were talking about having to read a channel
not just the register we are anyway polling for status.

Applied to the togreg branch of iio.git.

Thanks,

Jonathan
> 
> Regards,
> 
> Stefan
> 
> 

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

[Jonathan Cameron]

On Fri, 8 Dec 2017 18:41:49 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> 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>
Applied
> ---
> 
>  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());
>  

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

[Jonathan Cameron]

On Tue, 12 Dec 2017 20:21:14 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun, 10 Dec 2017 21:47:37 +0100
> Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> 
> > On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote:  
> > > On Fri, 8 Dec 2017 18:41:50 +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.
> > > > 
> > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > > ---
> > > > 
> > > >  drivers/iio/adc/ina2xx-adc.c | 35 +++++++++++++++++++++--------------
> > > >  1 file changed, 21 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > > > index 2621a34ee5c6..65bd9e69faf2 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,22 @@ 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);
> > > > +
> > > > +		do {
> > > > +			timespec64_add_ns(&next, 1000 * sampling_us);
> > > > +			delta = timespec64_sub(next, now);
> > > > +			delay_us = timespec64_to_ns(&delta) / 1000;
> > > > +		} while (delay_us <= 0);    
> > > 
> > > Umm. I'm lost, what is the purpose of the above dance?
> > > A comment perhaps.    
> > 
> > next is the timestamp for the next read to happen, now is the current time. 
> > Obviously we have to sleep for the remainder.
> > 
> > Each sampling interval the "next" timestamp is pushed back by sampling_us. 
> > Normally this happens exactly once per read, i.e. we schedule the reads to 
> > happen exactly each sampling interval.
> > 
> > The sampling inteval is *only* added multiple times if it is faster than the 
> > bus can deliver the data (at 100 kBits/s, each register read takes about 400 
> > us, so sampling faster than every ~1 ms is not possible.  
> 
> So this is deliberately skipping a sample if this happens?  It was this
> element that I wasn't understanding previously. 
> Add a comment in the code to explain this and I'm happy. It's horrible,
> but not much we can do if things are simply going too fast.
I still want to see a comment in the code making it clear what is
happening in that loop.  So for now I'm going to stop here in applying
this series.

Jonathan

> 
> Thanks for the info.
> 
> Jonathan
> > 
> > The old code measured the time spent for reading the registers and slept for 
> > the remainder of the interval. This way the sampling drifts, as there is some 
> > time not accounted for - usleep_range, function call overhead, kthread 
> > interrupted.
> > 
> > Using a timestamp avoids the drift. It also allows simple readjustment of the 
> > "next" sampling time when polling the status register.
> > 
> > Kind regards,
> > 
> > Stefan
> >   
> 
> --
> 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