OMAP: Keypad driver
PATCH 1/1 OMAP: Keypad driver
Please review the attached TI OMAP Keypad driver patch and consider it for -mm submission. Thanx.
---Komal Shah
http://komalshah.blogspot.com
Signature · 1 line
http://www.fastmail.fm - The way an email service should be
Quoted 2 lines · On Sat, 22 Jul 2006 01:26:14 -0700 "Komal Shah" <[email protected]>…
Please review the attached TI OMAP Keypad driver patch and consider it for -mm submission.
The code looks clean.
- Could use setup_timer().
- Check the return value from device_create_file() (and any and all such similar functions), handle failure appropriately.
- I don't think the tasklet stopping code is correct. tasklet_disable() will prevent the callback from being called. The del_timer_sync() will kill off the timer. But the just-killed timer handler might have left the tasklet scheduled, and although it will not call the handler, the high-level tasklet code can still execute, and will then start playing with now-freed data. A final tasklet_kill() should fix that up.
- Perhaps the probe function is requesting the IRQ too early? The IRQ handler can perhaps be called while the hardware is still being set up.
- Use INTF_TRIGGER_FALLING (etc), not the now-deprecated SA_TRIGGER_FALLING.
- The changelog needs work. What's an OMAP? ;)
- Finally, by what authority does [email protected] submit Nokia and TI's code?? Please review Section 11 of Documentation/SubmittingPatches, seek and obtain signoffs from the other authors and then add your own, thanks.
Quoted 21 lines · --- Andrew Morton
The code looks clean.
- Could use
setup_timer().- Check the return value from
device_create_file()(and any and all such
similar functions), handle failure appropriately.- I don't think the tasklet stopping code is correct.
tasklet_disable()
will prevent the callback from being called. Thedel_timer_sync()
will
kill off the timer. But the just-killed timer handler might have
left
the tasklet scheduled, and although it will not call the handler,
the
high-level tasklet code can still execute, and will then start
playing
with now-freed data. A finaltasklet_kill()should fix that up.
Quoted 4 lines
- Perhaps the probe function is requesting the IRQ too early? The IRQ
handler can perhaps be called while the hardware is still being set
up.
Ok. I will move it before
omap_writew(0, OMAP_MPUIO_BASE + OMAP_MPUIO_KBD_MASKIT);
Quoted 3 lines
- Use
INTF_TRIGGER_FALLING(etc), not the now-deprecatedSA_TRIGGER_FALLING.
Thanx for the review. I will update the driver and submit the patch.
Quoted 2 lines
- The changelog needs work. What's an OMAP? ;)
Ok. May changelog needs URL entry like http://linux.omap.com _or_
http://www.ti.com/omap :)
Quoted 7 lines
- Finally, by what authority does [email protected] submit Nokia
and TI's code?? Please review Section 11 ofDocumentation/SubmittingPatches, seek and obtain signoffs from the
other
authors and then add your own, thanks.
I have CCed all the authors mentioned in the top of the driver file, and I thought they will just reply with Signed-off-by: line. I never added my name in Copyright, even when I did some re-organization of omap keypad driver, which I thought was not major change :)
---Komal Shah
http://komalshah.blogspot.com/
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com