[PATCH v2] OMAP: I2C: Fix the interrupt clearing in OMAP4

[PATCH v2] OMAP: I2C: Fix the interrupt clearing in OMAP4

[Shubhrajyoti D]

For OMAP4 Interrupt enable register is a legacy register.
To clear the interrupts we were writing 0 to it. However on OMAP4
we were writing 1 to  IRQENABLE_CLR which clears only the arbitration
lost interrupt. The patch intends to fix the same by writing 1
to all the bits.

Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 2dfb631..bf4376f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -167,6 +167,10 @@ enum {
 #define SYSC_IDLEMODE_SMART		0x2
 #define SYSC_CLOCKACTIVITY_FCLK		0x2
 
+
+/* Mask to clear all the interrupts */
+#define OMAP_I2C_IRQENABLE_CLR_ALL	0x6FFF
+
 /* Errata definitions */
 #define I2C_OMAP_ERRATA_I207		(1 << 0)
 #define I2C_OMAP3_1P153			(1 << 1)
@@ -308,8 +312,17 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
 	pdata = pdev->dev.platform_data;
 
 	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
+
+	/*
+	 * The Interrupt enable register is a legacy register for OMAP4.
+	 * However to clear all the interrupts we could write 0 to Interrupt
+	 * enable reg  or  should write 1 to all the bits of the
+	 * I2C_IRQENABLE_CLR register.
+	 */
+
 	if (dev->rev >= OMAP_I2C_REV_ON_4430)
-		omap_i2c_write_reg(dev, OMAP_I2C_IRQENABLE_CLR, 1);
+		omap_i2c_write_reg(dev, OMAP_I2C_IRQENABLE_CLR,
+					OMAP_I2C_IRQENABLE_CLR_ALL);
 	else
 		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
 
-- 
1.7.1

Re: [PATCH v2] OMAP: I2C: Fix the interrupt clearing in OMAP4

[Kevin Hilman]

Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org> writes:

> For OMAP4 Interrupt enable register is a legacy register.

I don't see anything in the docs mentioning this is legacy.  In fact,
that register is used extensivly throughout the driver, even for OMAP4.

I think the CLR/SET registers were added to aid atomically
setting/clearing specific interrupts, but when disabling all, I don't
see why I2C_IE cannot be used.

For that reason, any reason why the 4430-specific check cannot simply be
removed to fix this interrupt clearing bug?

> To clear the interrupts we were writing 0 to it. 

This patch still writes 0 to it, so I'm not seeing the point of this comment.

> However on OMAP4 we were writing 1 to IRQENABLE_CLR which clears only
> the arbitration lost interrupt. The patch intends to fix the same by
> writing 1 to all the bits.

This is the bug, and should come first in the changelog so readers know
what the problem is.

> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>

I believe this patch was originally from a fix by Vikram Pandita.  Even
if you've now changed the implementation, please credit the original
author (and Cc them) in the changelog.  It's common practice (and common
courtesy) to say something like "Based on an a patch originally from
Author <email>".  Thanks.

Also, this patch doesn't apply to mainline or linux-omap master.  Can
you please update?

Thanks,

Kevin

Re: [PATCH v2] OMAP: I2C: Fix the interrupt clearing in OMAP4

[Shubhrajyoti]

On Tuesday 22 November 2011 12:05 AM, Kevin Hilman wrote:
> Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org> writes:
>
>> For OMAP4 Interrupt enable register is a legacy register.
> I don't see anything in the docs mentioning this is legacy.  In fact,
> that register is used extensivly throughout the driver, even for OMAP4.
>
> I think the CLR/SET registers were added to aid atomically
> setting/clearing specific interrupts, but when disabling all, I don't
> see why I2C_IE cannot be used.
>
> For that reason, any reason why the 4430-specific check cannot simply be
> removed to fix this interrupt clearing bug?
I think IE could be used as well since the CLR is there I thought of
using it.

>> To clear the interrupts we were writing 0 to it. 
> This patch still writes 0 to it, so I'm not seeing the point of this comment.
>
>> However on OMAP4 we were writing 1 to IRQENABLE_CLR which clears only
>> the arbitration lost interrupt. The patch intends to fix the same by
>> writing 1 to all the bits.
> This is the bug, and should come first in the changelog so readers know
> what the problem is.
>
Yes will make this the first thing .
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> I believe this patch was originally from a fix by Vikram Pandita.  Even
> if you've now changed the implementation, please credit the original
> author (and Cc them) in the changelog.  It's common practice (and common
> courtesy) to say something like "Based on an a patch originally from
> Author <email>".  Thanks.
Yes missed  to copy the second version will update.
> Also, this patch doesn't apply to mainline or linux-omap master.  Can
> you please update?
It is was based on linus tree. Will update.
> Thanks,
>
> Kevin