[LinuxPPS] [PATCH]

[Cirilo Bernardo]

Thanks for all the comments; I'll work through these in the next week.
I just have a few questions to help me out:


On Feb 1, 2008 9:45 PM, Rodolfo Giometti <giometti at enneenne.com> wrote:
> On Fri, Feb 01, 2008 at 02:00:24AM +0000, Cirilo Bernardo wrote:
> > I am attaching a patch to add support for PPS interrupts on an ISA
> > bus.  This may be useful for some embedded boards.
>
> Thanks! :)
>
> > I hope attachments are OK - since I only have this 'gmail' account I
> > have no idea if cut/paste will mangle all the formatting.
>
> Please, use git-format-patch to format a patch for submission. Add a
> "title", a "comment" (if needed) and the Signed-off-by line.
>
> Now some comments on your patch.
>


How do I get a comment and "submitted by" entry into the diff?  I used:
git diff origin master > the_patch
I noticed that there was no way to identify who sent the patch etc,
but I am unfamiliar with git and am having trouble finding out how to
get it to do what I want.


[SNIP]

> > diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
> > index d8ec308..e70c44f 100644
> > --- a/drivers/pps/Makefile
> > +++ b/drivers/pps/Makefile
> > @@ -9,3 +9,8 @@ obj-y                         += clients/
> >  ifeq ($(CONFIG_PPS_DEBUG),y)
> >  EXTRA_CFLAGS += -DDEBUG
> >  endif
> > +
> > +ifeq ($(CONFIG_PPS_ISA),y)
> > +obj-m                                += pps_isa.o
> > +endif
>
> Why??? =:-o
>
> [1] Just use "tristate" instaed of "bool" into Kconfig and use the
> line:
>
>   obj-$(CONFIG_PPS_CLIENT_ISA) += pps_isa.o
>
> into pps/clients/Makefile.

Will the build system automatically set 'm' if this driver depends on
pps_core and pps_core is built as a module?

[SNIP]
> > + *   This program is free software; you can redistribute it and/or modify
> > + *   it under the terms of the GNU General Public License as published by
> > + *   the Free Software Foundation; either version 2 of the License, or
> > + *   (at your option) any later version.
>
> Please, use __only__ GNU General Public License version 2 as the whole
> kernel code does.
>

I will look at the kernel comments; I haven't really been keeping up
with developments as GPL-3 was introduced.

[SNIP]

> > +#define NDEVS 1
> > +#define DEVNAME "pps_isa"   /* Device name to pass to various routines */
> > +
> > +static unsigned int irq = 0;
>
> Set it to -1 and allow 0 as valid IRQ number.

OK - so I use -1 as the invalid value, but if the user specifies '0' I
still need to bail out because 0 means "softirq" and this is not a
dependable timer -- did I get that right?

[SNIP]

> > +
> > +     /* Register with LinuxPPS as a PPS source */
> > +     ret = pps_register_source(&pps_isa_info, PPS_CAPTUREASSERT);
>
> No PPS_OFFSETASSERT?
>

OK, I'll look into putting it in; I need to re-read code to understand
what is going on.

[SNIP]


> I think is better using isa_register_driver() & Co. to manage ISA
> devices. Please, refere to linux/drivers/i2c/busses/i2c-pca-isa.c

Thanks for the tip.  I spent many hours trying to find out how to
claim an IRQ without reserving a char device.

[SNIP]
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> You should fix all these errors. :)
>
> I'm sorry for such paranoia comments but otherwise we have no chances
> to get kernel inclusion. :)
>
> Ciao,
>
> Rodolfo
>

OK, I'll work through this - but I'll probably be slow since I have so
much other work.  I never complain about paranoia - I build reliable
unattended field instruments so paranoia is a virtue - that's probably
also why I have so little time to do anything other than work -
between documentation, testing, debugging there is no time left for
anything else.

 With regards to static module variables - once upon a time they were
encouraged to reduce "namespace pollution".  Does the current build
system assure that variables are only visible within the module unless
exported?  If that is the case I will remove all statics.

 The other driver I have needs some cleaning up and rethinking some
things. The problem with the GPS I have (Trimble LassenSQ) is that the
device generates an undisciplined PPS signal when there is no
satellite lock and the manufacturer does not specify a tolerance.
Obviously using such a signal is not good for an NTP reference clock
and in my situation NTP will have no basis for deciding which clock is
defective (system or GPS).  So my other driver is actually a fairly
specialized GPS driver but it also interfaces to LinuxPPS; the PPS is
only delivered to pps_core if there is a satellite lock and this way
NTP is immediately aware that the problem is with the GPS.  I guess
such a driver is better placed elsewhere since PPS is only a minor
function of the driver - any comments on that?

- Cirilo