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

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

[Martin Kelly]

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

[PATCH 2/2] iio: bmi160: use all devm functions in probe

[Martin Kelly]

From: Martin Kelly <martin@martingkelly.com>

Currently, we're using the devm version of some but not all functions.
Switch to the devm version of iio_triggered_buffer_setup and
iio_device_register to simplify the code a bit and decrease the chance of
bugs.

Signed-off-by: Martin Kelly <martin@martingkelly.com>
---
 drivers/iio/imu/bmi160/bmi160_core.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
index 4d9d59d9e3a9..5e4efaed5e88 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -577,18 +577,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmi160_info;
 
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-					 bmi160_trigger_handler, NULL);
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      bmi160_trigger_handler, NULL);
 	if (ret < 0)
 		goto uninit;
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret < 0)
-		goto buffer_cleanup;
+		goto uninit;
 
 	return 0;
-buffer_cleanup:
-	iio_triggered_buffer_cleanup(indio_dev);
 uninit:
 	bmi160_chip_uninit(data);
 	return ret;
@@ -600,8 +598,6 @@ void bmi160_core_remove(struct device *dev)
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct bmi160_data *data = iio_priv(indio_dev);
 
-	iio_device_unregister(indio_dev);
-	iio_triggered_buffer_cleanup(indio_dev);
 	bmi160_chip_uninit(data);
 }
 EXPORT_SYMBOL_GPL(bmi160_core_remove);
-- 
2.11.0

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

[Daniel Baluta]

On Mon, Nov 19, 2018 at 2:54 AM 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>

Makes sense to me.

Acked-by: Daniel Baluta <daniel.baluta@gmail.com>

Would be nice to see some datapoints and compare the differences.

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

Re: [PATCH 2/2] iio: bmi160: use all devm functions in probe

[Daniel Baluta]

On Mon, Nov 19, 2018 at 2:55 AM Martin Kelly <martin@martingkelly.com> wrote:
>
> From: Martin Kelly <martin@martingkelly.com>
>
> Currently, we're using the devm version of some but not all functions.
> Switch to the devm version of iio_triggered_buffer_setup and
> iio_device_register to simplify the code a bit and decrease the chance of
> bugs.

Yes, it makes sense.

Acked-by: Daniel Baluta <daniel.baluta@gmail.com>

>
> Signed-off-by: Martin Kelly <martin@martingkelly.com>
> ---
>  drivers/iio/imu/bmi160/bmi160_core.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index 4d9d59d9e3a9..5e4efaed5e88 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -577,18 +577,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>         indio_dev->modes = INDIO_DIRECT_MODE;
>         indio_dev->info = &bmi160_info;
>
> -       ret = iio_triggered_buffer_setup(indio_dev, NULL,
> -                                        bmi160_trigger_handler, NULL);
> +       ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +                                             bmi160_trigger_handler, NULL);
>         if (ret < 0)
>                 goto uninit;
>
> -       ret = iio_device_register(indio_dev);
> +       ret = devm_iio_device_register(dev, indio_dev);
>         if (ret < 0)
> -               goto buffer_cleanup;
> +               goto uninit;
>
>         return 0;
> -buffer_cleanup:
> -       iio_triggered_buffer_cleanup(indio_dev);
>  uninit:
>         bmi160_chip_uninit(data);
>         return ret;
> @@ -600,8 +598,6 @@ void bmi160_core_remove(struct device *dev)
>         struct iio_dev *indio_dev = dev_get_drvdata(dev);
>         struct bmi160_data *data = iio_priv(indio_dev);
>
> -       iio_device_unregister(indio_dev);
> -       iio_triggered_buffer_cleanup(indio_dev);
>         bmi160_chip_uninit(data);
>  }
>  EXPORT_SYMBOL_GPL(bmi160_core_remove);
> --
> 2.11.0
>

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

[Martin Kelly]

On 11/19/18 12:45 AM, Daniel Baluta wrote:
> On Mon, Nov 19, 2018 at 2:54 AM 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>
> 
> Makes sense to me.
> 
> Acked-by: Daniel Baluta <daniel.baluta@gmail.com>
> 
> Would be nice to see some datapoints and compare the differences.
> 

I agree. I'm a bit short of time this week because of Thanksgiving, but
I'll try to do a bit of timing testing before and after the patch next week.

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

[Martin Kelly]

On 11/19/18 12:45 AM, Daniel Baluta wrote:
> On Mon, Nov 19, 2018 at 2:54 AM 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>
> 
> Makes sense to me.
> 
> Acked-by: Daniel Baluta <daniel.baluta@gmail.com>
> 
> Would be nice to see some datapoints and compare the differences.
> 

I agree. I'm a bit short of time this week because of Thanksgiving, but
I'll try to do a bit of timing testing before and after the patch next week.

Re: [PATCH 2/2] iio: bmi160: use all devm functions in probe

[Jonathan Cameron]

On Mon, 19 Nov 2018 10:48:20 +0200
Daniel Baluta <daniel.baluta@intel.com> wrote:

> On Mon, Nov 19, 2018 at 2:55 AM Martin Kelly <martin@martingkelly.com> wrote:
> >
> > From: Martin Kelly <martin@martingkelly.com>
> >
> > Currently, we're using the devm version of some but not all functions.
> > Switch to the devm version of iio_triggered_buffer_setup and
> > iio_device_register to simplify the code a bit and decrease the chance of
> > bugs.  
> 
> Yes, it makes sense.
> 
> Acked-by: Daniel Baluta <daniel.baluta@gmail.com>
Here we disagree :).  This results in the chip being uninitialized
via bmi160_chip_uninit before we remove the user space interfaces.
So the combination that is there right now was very deliberate
and is correct - that's not to say it can't be improved upon.

So if you do want to do this you need to ensure that operation
automatically occurs in the correct point in the remove and error
flows.

devm_add_action_or_reset is your friend here as it'll let you call
that uninit from the automatic unwinding path and hence at the
correct point.

Thanks,

Jonathan

> 
> >
> > Signed-off-by: Martin Kelly <martin@martingkelly.com>
> > ---
> >  drivers/iio/imu/bmi160/bmi160_core.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> > index 4d9d59d9e3a9..5e4efaed5e88 100644
> > --- a/drivers/iio/imu/bmi160/bmi160_core.c
> > +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> > @@ -577,18 +577,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >         indio_dev->info = &bmi160_info;
> >
> > -       ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > -                                        bmi160_trigger_handler, NULL);
> > +       ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > +                                             bmi160_trigger_handler, NULL);
> >         if (ret < 0)
> >                 goto uninit;
> >
> > -       ret = iio_device_register(indio_dev);
> > +       ret = devm_iio_device_register(dev, indio_dev);
> >         if (ret < 0)
> > -               goto buffer_cleanup;
> > +               goto uninit;
> >
> >         return 0;
> > -buffer_cleanup:
> > -       iio_triggered_buffer_cleanup(indio_dev);
> >  uninit:
> >         bmi160_chip_uninit(data);
> >         return ret;
> > @@ -600,8 +598,6 @@ void bmi160_core_remove(struct device *dev)
> >         struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >         struct bmi160_data *data = iio_priv(indio_dev);
> >
> > -       iio_device_unregister(indio_dev);
> > -       iio_triggered_buffer_cleanup(indio_dev);
> >         bmi160_chip_uninit(data);
> >  }
> >  EXPORT_SYMBOL_GPL(bmi160_core_remove);
> > --
> > 2.11.0
> >  

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

[Jonathan Cameron]

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;

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

[Martin Kelly]

On 11/25/18 5:59 AM, Jonathan Cameron wrote:
> 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
> 

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.

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

Re: [PATCH 2/2] iio: bmi160: use all devm functions in probe

[Martin Kelly]

On 11/25/18 5:51 AM, Jonathan Cameron wrote:
> On Mon, 19 Nov 2018 10:48:20 +0200
> Daniel Baluta <daniel.baluta@intel.com> wrote:
> 
>> On Mon, Nov 19, 2018 at 2:55 AM Martin Kelly <martin@martingkelly.com> wrote:
>>>
>>> From: Martin Kelly <martin@martingkelly.com>
>>>
>>> Currently, we're using the devm version of some but not all functions.
>>> Switch to the devm version of iio_triggered_buffer_setup and
>>> iio_device_register to simplify the code a bit and decrease the chance of
>>> bugs.
>>
>> Yes, it makes sense.
>>
>> Acked-by: Daniel Baluta <daniel.baluta@gmail.com>
> Here we disagree :).  This results in the chip being uninitialized
> via bmi160_chip_uninit before we remove the user space interfaces.
> So the combination that is there right now was very deliberate
> and is correct - that's not to say it can't be improved upon.
> 
> So if you do want to do this you need to ensure that operation
> automatically occurs in the correct point in the remove and error
> flows.
> 
> devm_add_action_or_reset is your friend here as it'll let you call
> that uninit from the automatic unwinding path and hence at the
> correct point.
> 
> Thanks,
> 
> Jonathan
> 

Thank you and good catch. I'll send a v2 using devm_add_action_or_reset.

>>
>>>
>>> Signed-off-by: Martin Kelly <martin@martingkelly.com>
>>> ---
>>>   drivers/iio/imu/bmi160/bmi160_core.c | 12 ++++--------
>>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
>>> index 4d9d59d9e3a9..5e4efaed5e88 100644
>>> --- a/drivers/iio/imu/bmi160/bmi160_core.c
>>> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
>>> @@ -577,18 +577,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>>>          indio_dev->modes = INDIO_DIRECT_MODE;
>>>          indio_dev->info = &bmi160_info;
>>>
>>> -       ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> -                                        bmi160_trigger_handler, NULL);
>>> +       ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
>>> +                                             bmi160_trigger_handler, NULL);
>>>          if (ret < 0)
>>>                  goto uninit;
>>>
>>> -       ret = iio_device_register(indio_dev);
>>> +       ret = devm_iio_device_register(dev, indio_dev);
>>>          if (ret < 0)
>>> -               goto buffer_cleanup;
>>> +               goto uninit;
>>>
>>>          return 0;
>>> -buffer_cleanup:
>>> -       iio_triggered_buffer_cleanup(indio_dev);
>>>   uninit:
>>>          bmi160_chip_uninit(data);
>>>          return ret;
>>> @@ -600,8 +598,6 @@ void bmi160_core_remove(struct device *dev)
>>>          struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>>          struct bmi160_data *data = iio_priv(indio_dev);
>>>
>>> -       iio_device_unregister(indio_dev);
>>> -       iio_triggered_buffer_cleanup(indio_dev);
>>>          bmi160_chip_uninit(data);
>>>   }
>>>   EXPORT_SYMBOL_GPL(bmi160_core_remove);
>>> --
>>> 2.11.0
>>>   
> 

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

[Jonathan Cameron]

On Mon, 26 Nov 2018 15:43:37 -0800
Martin Kelly <martin@martingkelly.com> wrote:

> On 11/25/18 5:59 AM, Jonathan Cameron wrote:
> > 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
> >   
> 
> 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;  
> >   
>