On Mon, 26 Nov 2018 15:43:37 -0800
Martin Kelly <martin@xxxxxxxxxxxxxxxx> wrote:
> On 11/25/18 5:59 AM, Jonathan Cameron wrote:
> > On Sun, 18 Nov 2018 16:53:46 -0800
> > Martin Kelly <martin@xxxxxxxxxxxxxxxx> wrote:
> >
> >> From: Martin Kelly <martin@xxxxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxxx>
> > 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
> >
>
> Yes, you are right that in the current driver, it's rather fuzzy when
> the "correct" timestamp to snap is. Separately, I'm working on adding
> interrupt support to this driver, so perhaps I should queue this up
> after that change. With interrupts (based on a data ready signal), I
> believe this change will be correct.
Agreed. That is the time to make this change as then it'll be obvious
why.
Thanks,
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;
> >
>
