Messages in this thread | | | Date | Wed, 8 Oct 2008 15:57:21 -0700 | From | "john stultz" <> | Subject | Re: [PATCH 10/10] PPS: low level IRQ timestamps recording. |
| |
On Thu, Oct 2, 2008 at 12:41 AM, Rodolfo Giometti <giometti@linux.it> wrote: > Add low level IRQ timestamps recording for x86 (32 and 64 bits) > platforms and enable UART clients in order to use it. > > This improves PPS precision. :) > > Signed-off-by: Rodolfo Giometti <giometti@linux.it>
Hey Rodolfo, First of all, kudos to your persistence on this patch set, it really has been a long time that you've been pushing this. Hopefully it won't take much longer. :)
I've never had much experience with PPS devices, so other then knowing a fair number of folks who are interested in using them, most of the code is outside of my realm of knowledge, so I've not had much to comment upon.
However, Thomas asked me to check and see how this interacted with the timekeeping subsystem, so I took another look at the current code. From a quick review, I really don't see any interactions. The code seems fairly well isolated.
One question I have is: Do you have any plans for integrating with the adjtimex() interface and its pps values?
My only other comment is on this last patch #10, and as you said in your original post, its deferrable. I'd agree that dropping this patch for now would be best, since adding a getnstimeofday() call, which may take a few microseconds on common hardware, to every interrupt would be a bad idea.
It seems having a special flag on the IRQ for timestamping would be better, and then we could only enable it for PPS connected interrupts. It may add a touch more jitter but I think it would allow for better performance while still reducing jitter.
So yea. Unless there are objections from the serial and parallel port maintainers, or someone who has more experience with PPS devices and might better critique the API proposed, I see no reason for holding this patch set back from my (limited) perspective.
thanks -john
| |