Re: [PATCH 1/2] iio: bmi160: snap timestamp closer to event

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 18 Nov 2018 16:53:46 -0800
Martin Kelly <martin@martingkelly.com> wrote:

> From: Martin Kelly <martin@martingkelly.com>
> 
> Currently, we snap the timestamp after reading from the buffer and
> processing the event. Technically, we can get a slightly more accurate
> timestamp by snapping it prior to reading the data, since the data was
> already generated prior to entering the trigger handler. This is not going
> to make a huge difference, but we might as well improve slightly.
> 
> Signed-off-by: Martin Kelly <martin@martingkelly.com>
Now this is always an interesting one.   We are running that handler
off the back of a trigger that isn't a dataready signal.  As such
the start of the function doesn't necessarily correspond to the
time that is closest to when the data is captured, it corresponds
to the time that is closest to when we asked for it (which is
not the most relevant number when processing the data)

Now, here we are talking on either side of the actual hardware
reads, so my suspicion is that it'll be just as inaccurate, but
in the other direction.

Also note that the compiler is probably within it's rights to
reorder that entirely if it can tell there are no side effects
(which it probably can't here... so this change will actually
do what you intend.)

So upshot is I'm currently unconvinced either way on whether this
is a useful change or not!

Jonathan


> ---
>  drivers/iio/imu/bmi160/bmi160_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index c85659ca9507..4d9d59d9e3a9 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -384,6 +384,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
> +	s64 ts = iio_get_time_ns(indio_dev);
>  	struct bmi160_data *data = iio_priv(indio_dev);
>  	__le16 buf[16];
>  	/* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */
> @@ -399,8 +400,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
>  		buf[j++] = sample;
>  	}
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> -					   iio_get_time_ns(indio_dev));
> +	iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
>  done:
>  	iio_trigger_notify_done(indio_dev->trig);
>  	return IRQ_HANDLED;