Re: [PATCH 10/10] PPS: low level IRQ timestamps recording.

[john stultz]

On Thu, Oct 2, 2008 at 12:41 AM, Rodolfo Giometti <giometti@xxxxxxxx> 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@xxxxxxxx>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/