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