Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Mehdi Djait]

Hi all,

DISCLAIMER: I'm new to kernel development.

I'm currently working on extending the kionix-kx022a driver to support
kionix-kx132. My question is about the timestamp pushed in the trigger
handler. The kionix-kx022a supports both FIFO and triggered buffer
mode, for my questions the triggered buffer mode is used.

Before asking the question: I tried to read every documentation
available, the kernel code and I found the Threads [1] [2] [3]

To better explain my question here are the two relevant setup functions:
A.  devm_iio_triggered_buffer_setup_ext(dev, idev,
                                        &iio_pollfunc_store_time, 
                                        kx022a_trigger_handler,
                                        IIO_BUFFER_DIRECTION_IN,
                                        &kx022a_buffer_ops,
                                        kx022a_fifo_attributes)

B. devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
                             &kx022a_irq_thread_handler,
                             IRQF_ONESHOT, name, idev);


And here are the relevant steps after an IRQ occurs :
1. IRQ context --> kx022a_irq_handler() --> gets the current timestamp
with "data->timestamp = iio_get_time_ns(idev);" and returns
IRQ_WAKE_THREAD

2. kx022a_irq_thread_handler() -> checks that the trigger is enabled
--> iio_trigger_poll_chained() --> handle_nested_irq(): which will only
call the bottom half of the pollfuncs

3. kx022a_trigger_handler() --> iio_push_to_buffers_with_timestamp(idev,
data->buffer, pf->timestamp)


My questions are:
Question 1: Is iio_push_to_buffers_with_timestamp(idev, data->buffer,
data->timestamp) instead of "pf->timestamp" better in the 
trigger_handler ? I was first concerned that it would be racy with the 
irq_handler, but the IRQF_ONESHOT flag is used, which means that the irq 
line is disabled until the threaded handler has been run, i.e. until
kx022a_trigger_handler runs and retruns IRQ_HANDLED (right?).

Question 2: If the change proposed in question 1 is wrong, would this
one be better iio_push_to_buffers_with_timestamp(idev, data->buffer,
iio_get_time_ns(idev)). There is some delay between the IRQ occuring
and trigger_handler being called but that is better than getting all 0
timestamps like suggested in [2]

I hope that I'm understating this correctly or at least not totally
off :) If yes, I will send a patch.

[1] https://lore.kernel.org/linux-iio/4FDB33CD.2090805@metafoo.de/
[2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/
[3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@Huawei.com/

--
Kind Regards
Mehdi Djait

Re: Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Matti Vaittinen]

Hi Mehdi,

On 2/16/23 22:22, Mehdi Djait wrote:
> Hi all,
> 
> DISCLAIMER: I'm new to kernel development.
> 
> I'm currently working on extending the kionix-kx022a driver to support
> kionix-kx132.

Thanks for working with this :) Support for the kx132, kx122 etc. is 
very welcome!

> My question is about the timestamp pushed in the trigger
> handler. The kionix-kx022a supports both FIFO and triggered buffer
> mode, for my questions the triggered buffer mode is used.
> 
> Before asking the question: I tried to read every documentation
> available, the kernel code and I found the Threads [1] [2] [3]
> 
> To better explain my question here are the two relevant setup functions:
> A.  devm_iio_triggered_buffer_setup_ext(dev, idev,
>                                          &iio_pollfunc_store_time,
>                                          kx022a_trigger_handler,
>                                          IIO_BUFFER_DIRECTION_IN,
>                                          &kx022a_buffer_ops,
>                                          kx022a_fifo_attributes)
> 
> B. devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
>                               &kx022a_irq_thread_handler,
>                               IRQF_ONESHOT, name, idev);
> 
> 
> And here are the relevant steps after an IRQ occurs :
> 1. IRQ context --> kx022a_irq_handler() --> gets the current timestamp
> with "data->timestamp = iio_get_time_ns(idev);" and returns
> IRQ_WAKE_THREAD
> 
> 2. kx022a_irq_thread_handler() -> checks that the trigger is enabled
> --> iio_trigger_poll_chained() --> handle_nested_irq(): which will only
> call the bottom half of the pollfuncs

I don't get the kx022a at my hands until next week to test this, but it 
seems to me your reasoning is right. iio_pollfunc_store_time() is 
probably not called. I just wonder why I didn't see zero timestamps when 
testing this. (OTOH, I had somewhat peculiar IRQ handling at first - 
maybe I broke this along the way).

> 3. kx022a_trigger_handler() --> iio_push_to_buffers_with_timestamp(idev,
> data->buffer, pf->timestamp)
> 
> 
> My questions are:
> Question 1: Is iio_push_to_buffers_with_timestamp(idev, data->buffer,
> data->timestamp) instead of "pf->timestamp" better in the
> trigger_handler ?

I don't see any "technical reasons" why it would be better. I think it 
is more standard looking though - but seems like it is plain wrong here 
as you pointed out.

> I was first concerned that it would be racy with the
> irq_handler, but the IRQF_ONESHOT flag is used, which means that the irq
> line is disabled until the threaded handler has been run, i.e. until
> kx022a_trigger_handler runs and retruns IRQ_HANDLED (right?).

Yes. This is the purpose of IRQF_ONESHOT. (Well, AFAICS the IRQs are 
re-enabled even if some other value is returned unless the IRQ_NONE is 
returned repeatedly).

> Question 2: If the change proposed in question 1 is wrong, would this
> one be better iio_push_to_buffers_with_timestamp(idev, data->buffer,
> iio_get_time_ns(idev)). There is some delay between the IRQ occuring
> and trigger_handler being called but that is better than getting all 0
> timestamps like suggested in [2]

Please, use the data->timestamp as you suggested.

> I hope that I'm understating this correctly or at least not totally
> off :) If yes, I will send a patch.

Thanks Mehdi! I think this was a great catch! Maybe - while at it - you 
could also send a patch adding a small kerneldoc to the 
iio_trigger_poll_chained() mentioning this particular issue. Yes, I 
guess it should be obvious just by reading the function name *_chained() 
- but I did fall on this trap (and according to your reference [2] so 
has someone else).

> [1] https://lore.kernel.org/linux-iio/4FDB33CD.2090805@metafoo.de/
> [2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/
> [3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@Huawei.com/

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Jonathan Cameron]

On Fri, 17 Feb 2023 07:56:22 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Mehdi,
> 
> On 2/16/23 22:22, Mehdi Djait wrote:
> > Hi all,
> > 
> > DISCLAIMER: I'm new to kernel development.
> > 
> > I'm currently working on extending the kionix-kx022a driver to support
> > kionix-kx132.  
> 
> Thanks for working with this :) Support for the kx132, kx122 etc. is 
> very welcome!
> 
> > My question is about the timestamp pushed in the trigger
> > handler. The kionix-kx022a supports both FIFO and triggered buffer
> > mode, for my questions the triggered buffer mode is used.
> > 
> > Before asking the question: I tried to read every documentation
> > available, the kernel code and I found the Threads [1] [2] [3]
> > 
> > To better explain my question here are the two relevant setup functions:
> > A.  devm_iio_triggered_buffer_setup_ext(dev, idev,
> >                                          &iio_pollfunc_store_time,
> >                                          kx022a_trigger_handler,
> >                                          IIO_BUFFER_DIRECTION_IN,
> >                                          &kx022a_buffer_ops,
> >                                          kx022a_fifo_attributes)
> > 
> > B. devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
> >                               &kx022a_irq_thread_handler,
> >                               IRQF_ONESHOT, name, idev);
> > 
> > 
> > And here are the relevant steps after an IRQ occurs :
> > 1. IRQ context --> kx022a_irq_handler() --> gets the current timestamp
> > with "data->timestamp = iio_get_time_ns(idev);" and returns
> > IRQ_WAKE_THREAD
> > 
> > 2. kx022a_irq_thread_handler() -> checks that the trigger is enabled  
> > --> iio_trigger_poll_chained() --> handle_nested_irq(): which will only  
> > call the bottom half of the pollfuncs  
> 
> I don't get the kx022a at my hands until next week to test this, but it 
> seems to me your reasoning is right. iio_pollfunc_store_time() is 
> probably not called. I just wonder why I didn't see zero timestamps when 
> testing this. (OTOH, I had somewhat peculiar IRQ handling at first - 
> maybe I broke this along the way).

This is a common problem.  So far we've always solved it in the driver
by using the pf->timestamp only if it's been set - otherwise fallback
to grabbing a new one to pass into iio_push_to_buffer_with_timestamp()
in the threaded handler.

It might be possible to solve in a generic fashion but it's a bit
fiddly so I don't think anyone has ever looked at it.

> 
> > 3. kx022a_trigger_handler() --> iio_push_to_buffers_with_timestamp(idev,
> > data->buffer, pf->timestamp)
> > 
> > 
> > My questions are:
> > Question 1: Is iio_push_to_buffers_with_timestamp(idev, data->buffer,
> > data->timestamp) instead of "pf->timestamp" better in the
> > trigger_handler ?  
> 
> I don't see any "technical reasons" why it would be better. I think it 
> is more standard looking though - but seems like it is plain wrong here 
> as you pointed out.

Agreed. That looks like a bug.

> 
> > I was first concerned that it would be racy with the
> > irq_handler, but the IRQF_ONESHOT flag is used, which means that the irq
> > line is disabled until the threaded handler has been run, i.e. until
> > kx022a_trigger_handler runs and retruns IRQ_HANDLED (right?).  
> 
> Yes. This is the purpose of IRQF_ONESHOT. (Well, AFAICS the IRQs are 
> re-enabled even if some other value is returned unless the IRQ_NONE is 
> returned repeatedly).
> 
> > Question 2: If the change proposed in question 1 is wrong, would this
> > one be better iio_push_to_buffers_with_timestamp(idev, data->buffer,
> > iio_get_time_ns(idev)). There is some delay between the IRQ occuring
> > and trigger_handler being called but that is better than getting all 0
> > timestamps like suggested in [2]  
> 
> Please, use the data->timestamp as you suggested.

I'd suggest a bit of both.  If you have a timestamp from the irq handler
use it. If it's not available then grab one locally in the threaded handler.

> 
> > I hope that I'm understating this correctly or at least not totally
> > off :) If yes, I will send a patch.  
> 
> Thanks Mehdi! I think this was a great catch! Maybe - while at it - you 
> could also send a patch adding a small kerneldoc to the 
> iio_trigger_poll_chained() mentioning this particular issue. Yes, I 
> guess it should be obvious just by reading the function name *_chained() 
> - but I did fall on this trap (and according to your reference [2] so 
> has someone else).
> 
> > [1] https://lore.kernel.org/linux-iio/4FDB33CD.2090805@metafoo.de/
> > [2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/
> > [3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@Huawei.com/  
> 
> Yours,
> 	-- Matti
> 

Re: Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Matti Vaittinen]

On 2/17/23 13:43, Jonathan Cameron wrote:
> On Fri, 17 Feb 2023 07:56:22 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hi Mehdi,
>>
>> On 2/16/23 22:22, Mehdi Djait wrote: >>> And here are the relevant steps after an IRQ occurs :
>>> 1. IRQ context --> kx022a_irq_handler() --> gets the current timestamp
>>> with "data->timestamp = iio_get_time_ns(idev);" and returns
>>> IRQ_WAKE_THREAD
>>>
>>> 2. kx022a_irq_thread_handler() -> checks that the trigger is enabled
>>> --> iio_trigger_poll_chained() --> handle_nested_irq(): which will only
>>> call the bottom half of the pollfuncs
>>
>> I don't get the kx022a at my hands until next week to test this, but it
>> seems to me your reasoning is right. iio_pollfunc_store_time() is
>> probably not called. I just wonder why I didn't see zero timestamps when
>> testing this. (OTOH, I had somewhat peculiar IRQ handling at first -
>> maybe I broke this along the way).
> 
> This is a common problem.  So far we've always solved it in the driver
> by using the pf->timestamp only if it's been set - otherwise fallback
> to grabbing a new one to pass into iio_push_to_buffer_with_timestamp()
> in the threaded handler.
> 
> It might be possible to solve in a generic fashion but it's a bit
> fiddly so I don't think anyone has ever looked at it.

I agree it's "fiddly" :) I played with a though of conditionally adding 
the timestamp in the iio_trigger_poll_chained() if the timestamp is zero 
there. This, however, would require clearing the timestamp when it is 
read - which gets "fiddly" soon. Hence I just suggested adding a note in 
kerneldoc.

>>
>>> Question 2: If the change proposed in question 1 is wrong, would this
>>> one be better iio_push_to_buffers_with_timestamp(idev, data->buffer,
>>> iio_get_time_ns(idev)). There is some delay between the IRQ occuring
>>> and trigger_handler being called but that is better than getting all 0
>>> timestamps like suggested in [2]
>>
>> Please, use the data->timestamp as you suggested.
> 
> I'd suggest a bit of both.  If you have a timestamp from the irq handler
> use it. If it's not available then grab one locally in the threaded handler.

Hm. I don't think we will end up in the kx022a threaded handler so that 
the data->timestamp is not populated in the IRQ handler. I am _far_ from 
an IIO expert - but I guess the only way would be that some other 
trigger invoked the threaded handler(?) Shouldn't the 
kx022a_validate_trigger() prevent this?

Please, follow Jonathan's guidance if he does not tell othervice. You 
clearly should not trust a random guy who obviously does not know how to 
write these drivers in the first place XD

>>
>>> I hope that I'm understating this correctly or at least not totally
>>> off :) If yes, I will send a patch.
>>
>> Thanks Mehdi! I think this was a great catch! Maybe - while at it - you
>> could also send a patch adding a small kerneldoc to the
>> iio_trigger_poll_chained() mentioning this particular issue. Yes, I
>> guess it should be obvious just by reading the function name *_chained()
>> - but I did fall on this trap (and according to your reference [2] so
>> has someone else).
>>
>>> [1] https://lore.kernel.org/linux-iio/4FDB33CD.2090805@metafoo.de/
>>> [2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/
>>> [3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@Huawei.com/
>>
>> Yours,
>> 	-- Matti
>>
> 

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Jonathan Cameron]

On Fri, 17 Feb 2023 13:59:16 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 2/17/23 13:43, Jonathan Cameron wrote:
> > On Fri, 17 Feb 2023 07:56:22 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> Hi Mehdi,
> >>
> >> On 2/16/23 22:22, Mehdi Djait wrote: >>> And here are the relevant steps after an IRQ occurs :  
> >>> 1. IRQ context --> kx022a_irq_handler() --> gets the current timestamp
> >>> with "data->timestamp = iio_get_time_ns(idev);" and returns
> >>> IRQ_WAKE_THREAD
> >>>
> >>> 2. kx022a_irq_thread_handler() -> checks that the trigger is enabled  
> >>> --> iio_trigger_poll_chained() --> handle_nested_irq(): which will only  
> >>> call the bottom half of the pollfuncs  
> >>
> >> I don't get the kx022a at my hands until next week to test this, but it
> >> seems to me your reasoning is right. iio_pollfunc_store_time() is
> >> probably not called. I just wonder why I didn't see zero timestamps when
> >> testing this. (OTOH, I had somewhat peculiar IRQ handling at first -
> >> maybe I broke this along the way).  
> > 
> > This is a common problem.  So far we've always solved it in the driver
> > by using the pf->timestamp only if it's been set - otherwise fallback
> > to grabbing a new one to pass into iio_push_to_buffer_with_timestamp()
> > in the threaded handler.
> > 
> > It might be possible to solve in a generic fashion but it's a bit
> > fiddly so I don't think anyone has ever looked at it.  
> 
> I agree it's "fiddly" :) I played with a though of conditionally adding 
> the timestamp in the iio_trigger_poll_chained() if the timestamp is zero 
> there. This, however, would require clearing the timestamp when it is 
> read - which gets "fiddly" soon. Hence I just suggested adding a note in 
> kerneldoc.
> 
> >>  
> >>> Question 2: If the change proposed in question 1 is wrong, would this
> >>> one be better iio_push_to_buffers_with_timestamp(idev, data->buffer,
> >>> iio_get_time_ns(idev)). There is some delay between the IRQ occuring
> >>> and trigger_handler being called but that is better than getting all 0
> >>> timestamps like suggested in [2]  
> >>
> >> Please, use the data->timestamp as you suggested.  
> > 
> > I'd suggest a bit of both.  If you have a timestamp from the irq handler
> > use it. If it's not available then grab one locally in the threaded handler.  
> 
> Hm. I don't think we will end up in the kx022a threaded handler so that 
> the data->timestamp is not populated in the IRQ handler. I am _far_ from 
> an IIO expert - but I guess the only way would be that some other 
> trigger invoked the threaded handler(?) Shouldn't the 
> kx022a_validate_trigger() prevent this?
Ah.  I'd missed this one restricted what triggers could be used.
We'll have to pay attention to this if that particular condition is ever
relaxed.
> 
> Please, follow Jonathan's guidance if he does not tell othervice. You 
> clearly should not trust a random guy who obviously does not know how to 
> write these drivers in the first place XD

You were right here :)

> 
> >>  
> >>> I hope that I'm understating this correctly or at least not totally
> >>> off :) If yes, I will send a patch.  
> >>
> >> Thanks Mehdi! I think this was a great catch! Maybe - while at it - you
> >> could also send a patch adding a small kerneldoc to the
> >> iio_trigger_poll_chained() mentioning this particular issue. Yes, I
> >> guess it should be obvious just by reading the function name *_chained()
> >> - but I did fall on this trap (and according to your reference [2] so
> >> has someone else).
> >>  
> >>> [1] https://lore.kernel.org/linux-iio/4FDB33CD.2090805@metafoo.de/
> >>> [2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/
> >>> [3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@Huawei.com/  
> >>
> >> Yours,
> >> 	-- Matti
> >>  
> >   
> 

Re: Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Mehdi Djait]

On Fri, Feb 17, 2023 at 02:28:28PM +0000, Jonathan Cameron wrote:
> On Fri, 17 Feb 2023 13:59:16 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
> > On 2/17/23 13:43, Jonathan Cameron wrote:
> > > On Fri, 17 Feb 2023 07:56:22 +0200
> > > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> > >   
> > >> Hi Mehdi,
> > >>
> > >> On 2/16/23 22:22, Mehdi Djait wrote: >>> And here are the relevant steps after an IRQ occurs :  
> > >>> 1. IRQ context --> kx022a_irq_handler() --> gets the current timestamp
> > >>> with "data->timestamp = iio_get_time_ns(idev);" and returns
> > >>> IRQ_WAKE_THREAD
> > >>>
> > >>> 2. kx022a_irq_thread_handler() -> checks that the trigger is enabled  
> > >>> --> iio_trigger_poll_chained() --> handle_nested_irq(): which will only  
> > >>> call the bottom half of the pollfuncs  
> > >>
> > >> I don't get the kx022a at my hands until next week to test this, but it
> > >> seems to me your reasoning is right. iio_pollfunc_store_time() is
> > >> probably not called. I just wonder why I didn't see zero timestamps when
> > >> testing this. (OTOH, I had somewhat peculiar IRQ handling at first -
> > >> maybe I broke this along the way).  
> > > 
> > > This is a common problem.  So far we've always solved it in the driver
> > > by using the pf->timestamp only if it's been set - otherwise fallback
> > > to grabbing a new one to pass into iio_push_to_buffer_with_timestamp()
> > > in the threaded handler.
> > > 
> > > It might be possible to solve in a generic fashion but it's a bit
> > > fiddly so I don't think anyone has ever looked at it.  
> > 
> > I agree it's "fiddly" :) I played with a though of conditionally adding 
> > the timestamp in the iio_trigger_poll_chained() if the timestamp is zero 
> > there. This, however, would require clearing the timestamp when it is 
> > read - which gets "fiddly" soon. Hence I just suggested adding a note in 
> > kerneldoc.
> > 
> > >>  
> > >>> Question 2: If the change proposed in question 1 is wrong, would this
> > >>> one be better iio_push_to_buffers_with_timestamp(idev, data->buffer,
> > >>> iio_get_time_ns(idev)). There is some delay between the IRQ occuring
> > >>> and trigger_handler being called but that is better than getting all 0
> > >>> timestamps like suggested in [2]  
> > >>
> > >> Please, use the data->timestamp as you suggested.  
> > > 
> > > I'd suggest a bit of both.  If you have a timestamp from the irq handler
> > > use it. If it's not available then grab one locally in the threaded handler.  
> > 
> > Hm. I don't think we will end up in the kx022a threaded handler so that 
> > the data->timestamp is not populated in the IRQ handler. I am _far_ from 
> > an IIO expert - but I guess the only way would be that some other 
> > trigger invoked the threaded handler(?) Shouldn't the 
> > kx022a_validate_trigger() prevent this?
> Ah.  I'd missed this one restricted what triggers could be used.
> We'll have to pay attention to this if that particular condition is ever
> relaxed.
> > 
> > Please, follow Jonathan's guidance if he does not tell othervice. You 
> > clearly should not trust a random guy who obviously does not know how to 
> > write these drivers in the first place XD
> 
> You were right here :)

So should I send a patch with data->timestamp as I suggested ? 

And should I write some documentaion to highlight the
difference between iio_trigger_poll and iio_trigger_poll_chained, i.e.,
where the functions expect to be called ?
Something similar to the /kernel/irq/irqdesc.c

> 
> > 
> > >>  
> > >>> I hope that I'm understating this correctly or at least not totally
> > >>> off :) If yes, I will send a patch.  
> > >>
> > >> Thanks Mehdi! I think this was a great catch! Maybe - while at it - you
> > >> could also send a patch adding a small kerneldoc to the
> > >> iio_trigger_poll_chained() mentioning this particular issue. Yes, I
> > >> guess it should be obvious just by reading the function name *_chained()
> > >> - but I did fall on this trap (and according to your reference [2] so
> > >> has someone else).
> > >>  
> > >>> [1] https://lore.kernel.org/linux-iio/4FDB33CD.2090805@metafoo.de/
> > >>> [2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/
> > >>> [3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@Huawei.com/  
> > >>
> > >> Yours,
> > >> 	-- Matti
> > >>  
> > >   
> > 
> 

Re: Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Jonathan Cameron]

On Fri, 17 Feb 2023 15:43:26 +0100
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:

> On Fri, Feb 17, 2023 at 02:28:28PM +0000, Jonathan Cameron wrote:
> > On Fri, 17 Feb 2023 13:59:16 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> > > On 2/17/23 13:43, Jonathan Cameron wrote:  
> > > > On Fri, 17 Feb 2023 07:56:22 +0200
> > > > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> > > >     
> > > >> Hi Mehdi,
> > > >>
> > > >> On 2/16/23 22:22, Mehdi Djait wrote: >>> And here are the relevant steps after an IRQ occurs :    
> > > >>> 1. IRQ context --> kx022a_irq_handler() --> gets the current timestamp
> > > >>> with "data->timestamp = iio_get_time_ns(idev);" and returns
> > > >>> IRQ_WAKE_THREAD
> > > >>>
> > > >>> 2. kx022a_irq_thread_handler() -> checks that the trigger is enabled    
> > > >>> --> iio_trigger_poll_chained() --> handle_nested_irq(): which will only    
> > > >>> call the bottom half of the pollfuncs    
> > > >>
> > > >> I don't get the kx022a at my hands until next week to test this, but it
> > > >> seems to me your reasoning is right. iio_pollfunc_store_time() is
> > > >> probably not called. I just wonder why I didn't see zero timestamps when
> > > >> testing this. (OTOH, I had somewhat peculiar IRQ handling at first -
> > > >> maybe I broke this along the way).    
> > > > 
> > > > This is a common problem.  So far we've always solved it in the driver
> > > > by using the pf->timestamp only if it's been set - otherwise fallback
> > > > to grabbing a new one to pass into iio_push_to_buffer_with_timestamp()
> > > > in the threaded handler.
> > > > 
> > > > It might be possible to solve in a generic fashion but it's a bit
> > > > fiddly so I don't think anyone has ever looked at it.    
> > > 
> > > I agree it's "fiddly" :) I played with a though of conditionally adding 
> > > the timestamp in the iio_trigger_poll_chained() if the timestamp is zero 
> > > there. This, however, would require clearing the timestamp when it is 
> > > read - which gets "fiddly" soon. Hence I just suggested adding a note in 
> > > kerneldoc.
> > >   
> > > >>    
> > > >>> Question 2: If the change proposed in question 1 is wrong, would this
> > > >>> one be better iio_push_to_buffers_with_timestamp(idev, data->buffer,
> > > >>> iio_get_time_ns(idev)). There is some delay between the IRQ occuring
> > > >>> and trigger_handler being called but that is better than getting all 0
> > > >>> timestamps like suggested in [2]    
> > > >>
> > > >> Please, use the data->timestamp as you suggested.    
> > > > 
> > > > I'd suggest a bit of both.  If you have a timestamp from the irq handler
> > > > use it. If it's not available then grab one locally in the threaded handler.    
> > > 
> > > Hm. I don't think we will end up in the kx022a threaded handler so that 
> > > the data->timestamp is not populated in the IRQ handler. I am _far_ from 
> > > an IIO expert - but I guess the only way would be that some other 
> > > trigger invoked the threaded handler(?) Shouldn't the 
> > > kx022a_validate_trigger() prevent this?  
> > Ah.  I'd missed this one restricted what triggers could be used.
> > We'll have to pay attention to this if that particular condition is ever
> > relaxed.  
> > > 
> > > Please, follow Jonathan's guidance if he does not tell othervice. You 
> > > clearly should not trust a random guy who obviously does not know how to 
> > > write these drivers in the first place XD  
> > 
> > You were right here :)  
> 
> So should I send a patch with data->timestamp as I suggested ? 

I think so.

> 
> And should I write some documentaion to highlight the
> difference between iio_trigger_poll and iio_trigger_poll_chained, i.e.,
> where the functions expect to be called ?
> Something similar to the /kernel/irq/irqdesc.c

Sure.  The chained naming is inconsistent, but I never got around to fixing it.
Probably should be something like iio_trigger_poll_nested()

Comment on expectations is good.

> 
> >   
> > >   
> > > >>    
> > > >>> I hope that I'm understating this correctly or at least not totally
> > > >>> off :) If yes, I will send a patch.    
> > > >>
> > > >> Thanks Mehdi! I think this was a great catch! Maybe - while at it - you
> > > >> could also send a patch adding a small kerneldoc to the
> > > >> iio_trigger_poll_chained() mentioning this particular issue. Yes, I
> > > >> guess it should be obvious just by reading the function name *_chained()
> > > >> - but I did fall on this trap (and according to your reference [2] so
> > > >> has someone else).
> > > >>    
> > > >>> [1] https://lore.kernel.org/linux-iio/4FDB33CD.2090805@metafoo.de/
> > > >>> [2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/
> > > >>> [3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@Huawei.com/    
> > > >>
> > > >> Yours,
> > > >> 	-- Matti
> > > >>    
> > > >     
> > >   
> >   

Re: Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Matti Vaittinen]

On 2/17/23 17:27, Jonathan Cameron wrote:
> On Fri, 17 Feb 2023 15:43:26 +0100
> Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> 
>> On Fri, Feb 17, 2023 at 02:28:28PM +0000, Jonathan Cameron wrote:
>>> On Fri, 17 Feb 2023 13:59:16 +0200
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> On 2/17/23 13:43, Jonathan Cameron wrote:
>>>>> On Fri, 17 Feb 2023 07:56:22 +0200
>>>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>>>      
>>>>>> Hi Mehdi,
>>>>>>
>>>>>> On 2/16/23 22:22, Mehdi Djait wrote: 

>>
>> So should I send a patch with data->timestamp as I suggested ?
> 
> I think so.

Please do! That would be very much appreciated :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Mehdi Djait]

On Fri, Feb 17, 2023 at 07:02:07PM +0200, Matti Vaittinen wrote:
> On 2/17/23 17:27, Jonathan Cameron wrote:
> > On Fri, 17 Feb 2023 15:43:26 +0100
> > Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> > 
> > > On Fri, Feb 17, 2023 at 02:28:28PM +0000, Jonathan Cameron wrote:
> > > > On Fri, 17 Feb 2023 13:59:16 +0200
> > > > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> > > > > On 2/17/23 13:43, Jonathan Cameron wrote:
> > > > > > On Fri, 17 Feb 2023 07:56:22 +0200
> > > > > > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> > > > > > > Hi Mehdi,
> > > > > > > 
> > > > > > > On 2/16/23 22:22, Mehdi Djait wrote:
> 
> > > 
> > > So should I send a patch with data->timestamp as I suggested ?
> > 
> > I think so.
> 
> Please do! That would be very much appreciated :)
> 

I will send the patch today. 

--
Kind Regards
Mehdi Djait

> Yours,
> 	-- Matti
> 
> -- 
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
> 
> ~~ When things go utterly wrong vim users can always type :help! ~~
>