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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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! ~~