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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux