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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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