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
> Add to MAINTAINERS
> 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
No way... I can add a specific flag as for uart lines or a kernel
> - don't implement your own dbg() stuff, use dprintk and friends
> - drop the inlines, gcc will do the right thing.
> 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.
> 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 http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/