Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux

From: Rodolfo Giometti
Date: Fri Feb 16 2007 - 15:57:54 EST

On Fri, Feb 16, 2007 at 08:56:18PM +0100, Jan Dittmer wrote:

> Drop the linux prefix. It's in the linux kernel after all.


> > +PROCFS support
> > +--------------
> New features shouldn't introduce new /proc stuff.

It's a must? I can leave procfs for backward compatibility with old



> 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

I think it's not possible... however the Russell's suggestions should
go in that direction.

> what happens if you've multiple lp ports? How do you control which to
> grab?

No way... I can add a specific flag as for uart lines or a kernel
module parameter.

> - don't implement your own dbg() stuff, use dprintk and friends
> - drop the inlines, gcc will do the right thing.

Ok. Ok.

> Perhaps just implement empty defines for the none pps cases and get
> rid of the ifdefs? But this should really be controllabe via
> sysfs or such.

Mmm... let me think about howto implement that...

> help text


> help text and difference to CLIENT_LP?


> Why no dynamically allocated array?

It's easier! :P

Also it's very difficult having more that 3 or 4 PPS sources in a

> I wouldn't bet on that.

Why not? =:-o

Also locking instructions may add extra code and delay the timestamp

> Doesn't match filename

I'm going to fix it.

> > +++ b/drivers/pps/procfs.c
> I'd drop that completely.


> You read the comment above your line?

No, sorry. I'm going to choose another id number... or can I keep 17?

> These should use dprintk and friends


> Isn't something like 4 more reasonable (lp + 8250 + ktimer?)

It should be enought...

> I think you can drop the volatiles, there was a discussion some time ago
> that they mostly waste of words.

I see...

> This one looks pretty fishy. After the check you normally want
> to use it, don't you? And then you already lost the guarantee.

You are right...

> > +#define to_class_dev(obj) container_of((obj), struct class_device, kobj)
> pretty generic name.

I should change it?

> Have you thought about 32/64bit issues?

No. I have no 64 bits machine to test the code...

> Function in .h?

I'm going to check it.

Thanks for your suggestions,



GNU/Linux Solutions e-mail: giometti@xxxxxxxxxxxx
Linux Device Driver giometti@xxxxxxxxx
Embedded Systems giometti@xxxxxxxx
UNIX programming phone: +39 349 2432127
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at