On 10/01/2013 11:26 PM, Jonathan Cameron wrote: > On 10/01/13 13:55, Beomho Seo wrote: >> >> This patch adds a new driver for Capella CM36651 proximity and RGB sensor. >> >> Signed-off-by: Beomho Seo <beomho.seo@xxxxxxxxxxx> > A couple of little points inline. In short, some unnecessary > variable initializations (bad idea as they may hide real bugs) > and you should use IRQF_DISABLED if you really do need to have > the interrupt disabled. IRQF_DISABLED actually does something else (or well did something else, these days it's a no-op and shouldn't be used). But yea, a flag that keeps the interrupt disabled would be nice. > > Also, sorry if I missed an explanation, but I can't see why you need > the enable and disable on the interrupt. As you explicitly turn the > interrupt on by writing to a device register why would an interrupt > occur at any other time? Convention is normally to not do the disabling > of the irq unless it is actually necessary. It complicates > the code for no gain. Yep, I don't see any reason why the code actually has to use enable_irq/disable_irq. Unless there is a problem with the chip that it tends to generate lots of spurious interrupts. In that case this should be documented in the code though. - Lars -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
