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.
Ok. > > +PROCFS support > > +-------------- > > New features shouldn't introduce new /proc stuff. It's a must? I can leave procfs for backward compatibility with old utilities? > Add to MAINTAINERS Ok. > 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 Ok. > help text and difference to CLIENT_LP? Ok. > Why no dynamically allocated array? It's easier! :P Also it's very difficult having more that 3 or 4 PPS sources in a system. > I wouldn't bet on that. Why not? =:-o Also locking instructions may add extra code and delay the timestamp recording... > 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 Ok. > 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, Rodolfo -- GNU/Linux Solutions e-mail: [EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems [EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
