On Fri, 17 Feb 2023 13:59:16 +0200 Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote: > On 2/17/23 13:43, Jonathan Cameron wrote: > > On Fri, 17 Feb 2023 07:56:22 +0200 > > Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxxx/ > >>> [2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/ > >>> [3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@xxxxxxxxxx/ > >> > >> Yours, > >> -- Matti > >> > > >
