[LinuxPPS] LinuxPPS: fixes

[Rodolfo Giometti]

Hello,

after yours suggestions I modified my code. Please see the patch
below.

However some issues are still not modified. Here my reasons (lines
with ">" are yours suggestions):

> Your way to hook into lp and 8250 is pretty gross. It should at least be
> possible to deactivate it via the kernel command line, but it would be
> a lot nicer to have pps_lp and pps_8250 modules which you can load. Also
> what happens if you've multiple lp ports? How do you control which to
> grab?

Serial support has "setserial" and I used it to enable/disable PPS
support at runtime. Here the patch for setserial:

--- setserial.c.old	2007-02-17 08:47:45.000000000 +0100
+++ setserial.c	2007-02-17 08:57:26.000000000 +0100
@@ -127,6 +127,7 @@
 	CMD_FLAG,	"hup_notify",	ASYNC_HUP_NOTIFY, ASYNC_HUP_NOTIFY, 0, FLAG_CAN_INVERT,
 	CMD_FLAG,	"skip_test",	ASYNC_SKIP_TEST,ASYNC_SKIP_TEST,2, FLAG_CAN_INVERT,
 	CMD_FLAG,	"auto_irq",	ASYNC_AUTO_IRQ,	ASYNC_AUTO_IRQ,	2, FLAG_CAN_INVERT,
+	CMD_FLAG,	"hardpps",	ASYNC_HARDPPS_CD, ASYNC_HARDPPS_CD, 2, FLAG_CAN_INVERT,
 	CMD_FLAG,	"split_termios", ASYNC_SPLIT_TERMIOS, ASYNC_SPLIT_TERMIOS, 2, FLAG_CAN_INVERT,
 	CMD_FLAG,	"session_lockout", ASYNC_SESSION_LOCKOUT, ASYNC_SESSION_LOCKOUT, 2, FLAG_CAN_INVERT,
 	CMD_FLAG,	"pgrp_lockout", ASYNC_PGRP_LOCKOUT, ASYNC_PGRP_LOCKOUT, 2, FLAG_CAN_INVERT,
@@ -725,6 +726,7 @@
 	fprintf(stderr, "\t^ fourport\tconfigure the port as an AST Fourport\n");
 	fprintf(stderr, "\t  autoconfig\tautomatically configure the serial port\n");
 	fprintf(stderr, "\t^ auto_irq\ttry to determine irq during autoconfiguration\n");
+	fprintf(stderr, "\t^ hardpps\tmanage PPS signal when CD changes status\n");
 	fprintf(stderr, "\t^ skip_test\tskip UART test during autoconfiguration\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "\t^ sak\t\tset the break key as the Secure Attention Key\n");

For parallel support I just cleaned up the code but I didn't find
something similar to setserial so I decided to leave the code as is
since at interrupt time we just register a timestamp without touching
the parallel port functionality.

> don't implement your own dbg() stuff, use dprintk and friends

My own dbg() stuff are for specific debugging string for PPS code. If
I use pr_dbg & friends I also enable several (and unwanted) kernel
debugging lines.

> drop the inlines, gcc will do the right thing.

I like specify them. :) Hope this is not a problem...

> > +             for (i = 0; i < LINUXPPS_MAX_SOURCES; i++)
> > +                     if (!__linuxpps_is_allocated(i))
> > +                             break;
> > +             if (i >= LINUXPPS_MAX_SOURCES) {
> > +                     err("no free source ids");
> > +                     return -ENOMEM;
> > +             }
>
> Why no dynamically allocated array?

It's just easier...

> > +void linuxpps_event(int source, int event, void *data)
> > +{
> > +     struct timespec ts;
> > +
> > +     /* In this function we shouldn't need locking at all since each PPS
> > +      * source arrives once per second and due the per-PPS source data
> > +      * array... */
>
> I wouldn't bet on that.

I see. However it is not a problem at all since if the device is a PPS
source we have just one interrupt at second, if not we just register a
timestamp never used.

> > +/* The main struct */
> > +struct linuxpps_s {
> > +     struct linuxpps_source_info_s *info;            /* PSS source info */
> > +
> > +     pps_params_t params;                            /* PPS's current params */
> > +
> > +     volatile pps_seq_t assert_sequence;             /* PPS' assert event seq # */
> > +     volatile pps_seq_t clear_sequence;              /* PPS' clear event seq # */
> > +     volatile pps_timeu_t assert_tu;
> > +     volatile pps_timeu_t clear_tu;
>
> I think you can drop the volatiles, there was a discussion some time ago
> that they mostly waste of words.

As for inlines I like specify volatiles... :P

> > +/* Private functions */
> > +
> > +static int netlink_msg(int socket, struct pps_netlink_msg *nlpps)
>
> Function in .h?

That's why these functions must be used into userland from NTPD and it
doesn't consider specific libraries to link with... it's the same
trick used by PPSkit (another PPS support).

> > +#define LINUXPPS_MAX_NAME_LEN           32
> > +struct pps_netlink_msg {
> > +     int cmd;                          /* the command to execute */
> > +     int source;
> > +     char name[LINUXPPS_MAX_NAME_LEN]; /* symbolic name */
> > +     char path[LINUXPPS_MAX_NAME_LEN]; /* path of the connected device */
> > +     int consumer;                     /* selected kernel consumer */
> > +     pps_params_t params;
> > +     int mode;                         /* edge */
> > +     int tsformat;                     /* format of time stamps */
> > +     pps_info_t info;
> > +     struct timespec timeout;
> > +     int ret;
> > +};
>
> Have you thought about 32/64bit issues?

I have no 64 bits hardware to test it... I just used PPC, MIPS and ARM
at 32 bits.

Thanks a lot for your suggestions!

Ciao,

Rodolfo

P.S. After your replies (and some tests) I'll repost my patch for
inclusion.

-- 

GNU/Linux Solutions                  e-mail:    giometti at enneenne.com
Linux Device Driver                             giometti at gnudd.com
Embedded Systems                     		giometti at linux.it
UNIX programming                     phone:     +39 349 2432127
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ntp-pps-2.6.20.diff
Type: text/x-diff
Size: 62918 bytes
Desc: not available
Url : http://ml.enneenne.com/pipermail/linuxpps/attachments/20070218/2bd5a85f/ntp-pps-2.6.20-0001.bin