On Thursday 10 September 2009, Miguel Aguilar wrote: > Can anyone check this patch to make it ready to be pushed in the mainline?
I tried it and it seemed to behave. Re "ready to push", you should submit it ASAP to linux-input for review, to see if they think it's ready ... after addressing the following comments: - "struct davinci_kp" should be driver-internal, not part of the <mach/...> header used to interface platform_data - (number) is bad; just #define as the number, no parens - KEYPAD_BASE is unused (and shouldn't exist anyway) - Some of the keypad irq masks use spaces not tabs to line up; fix - Don't #undef DEBUG in the driver; that should normally be set conditionally via Makefile - Why do you expose non-static symbols in the driver? As a rule, drivers should not export symbols... - I see excess #includes - Those "#ifdef DEBUG then pr_info()" things should just be dev_debug() with no #ifdefs. - Your probe() logic can leak some of the memory you allocate ... and the "cannot allocate input device" message also has *two* failure modes, despite that text. (That case should fail with -ENOMEM; and I'd make most of those messages dev_debug not dev_err, on the grounds that once a board works they're all just wasted space and you want them to compile out.) - Use dev_err() not printk(KERN_ERR) - Use resource_size() to compute base_size. - The probe() should return the status code that's provided by the lower level functions, instead of always discarding that in favor of -EINVAL. Most failure modes are not related to bad arguments. - Surely your kp_remove() method should release the resources in the reverse of the order in which they were acquired? In particular, get rid of the IRQ before unregistering the input device, so you never need to worry about IRQs coming in on devices that have been unregistered, and thus triggering upcalls that the input subsystem will choke on. - Use platform_driver_probe() and __exit/__exit_p(); there's no point in keeping that code around in typical configs, it'd just waste memory. - Snug the module_init() decl up next to the function it identifies; ditto the module_exit() decl and the function which *it* identifies. _______________________________________________ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
