Re: [PATCH v5 1/2] iio: cm36651: Add CM36651 proximity/light sensor

[Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> 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