Skip to contentSkip to conversation

I2C: Fix OMAP I2C status register handling in IRQ processing

PATCH In review Seth Forshee 3 participants 5 messages Mar 12, 2008 – Mar 28, 2008

Subsystems (4)
I2C subsystem host drivers OMAP2+ support OMAP1 support OMAP I2C driver
lore
Share & tools
lore

Seth Forshee

author
#0 lore raw

PATCH 1/1 I2C: Fix OMAP I2C status register handling in IRQ processing

The IRQ handler in omap-i2c.c can sometimes clear status bits without actually processing them. In particular, error status bits will be ignored if any of the ARDY, RRDY, RDR, XRDY, or XDR bits are concurrently set.

Signed-off-by Seth Forshee <[email protected]>

---

More information:

I originally noticed this problem on a custom 2430 board I am working with. Whenever an i2c chip driver calls i2c_probe() the device would be found on both i2c busses at each of the possible addresses for the device. I discovered that NACK was being set in the status register but omap_i2c_xfer() still returned success because ARDY was also set.

This patch fixes this issue on my board and hasn't caused any problems (in my admittedly light i2c usage). I don't have immediate access to an SDP board however, so it has not been tested on that platform.

 drivers/i2c/busses/i2c-omap.c |   28 +++++++++++++---------------
 1 files changed, 13 insertions(+), 15 deletions(-)
drivers/i2c/busses/i2c-omap.c +13 -15
index 4777466..a16d513 100644
@@ -590,7 +590,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 bits;
 	u16 stat, w;
-	int count = 0;
+	int err, count = 0;
 
 	if (dev->idle)
 		return IRQ_NONE;

@@ -605,10 +605,19 @@ omap_i2c_isr(int this_irq, void *dev_id)
 
 		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
 
-		if (stat & OMAP_I2C_STAT_ARDY) {
-			omap_i2c_complete_cmd(dev, 0);
-			continue;
+		err = 0;
+		if (stat & OMAP_I2C_STAT_NACK) {
+			err |= OMAP_I2C_STAT_NACK;
+			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
+					   OMAP_I2C_CON_STP);
 		}
+		if (stat & OMAP_I2C_STAT_AL) {
+			dev_err(dev->dev, "Arbitration lost\n");
+			err |= OMAP_I2C_STAT_AL;
+		}
+		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
+					OMAP_I2C_STAT_AL))
+			omap_i2c_complete_cmd(dev, err);
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
 			if (dev->fifo_size) {

@@ -640,7 +649,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				}
 			}
 			omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
-			continue;
 		}
 		if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
 			u8 num_bytes = 1;

@@ -674,7 +682,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
 			omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-			continue;
 		}
 		if (stat & OMAP_I2C_STAT_ROVR) {
 			dev_err(dev->dev, "Receive overrun\n");

@@ -684,15 +691,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			dev_err(dev->dev, "Transmit overflow\n");
 			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
 		}
-		if (stat & OMAP_I2C_STAT_NACK) {
-			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_NACK);
-			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
-					   OMAP_I2C_CON_STP);
-		}
-		if (stat & OMAP_I2C_STAT_AL) {
-			dev_err(dev->dev, "Arbitration lost\n");
-			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_AL);
-		}
 	}
 
 	return count ? IRQ_HANDLED : IRQ_NONE;
Signature · 1 line
1.5.2.5
#1 lore raw
Quoted 16 lines · Seth Forshee

The IRQ handler in omap-i2c.c can sometimes clear status bits without actually processing them. In particular, error status bits will be ignored if any of the ARDY, RRDY, RDR, XRDY, or XDR bits are concurrently set.

Signed-off-by Seth Forshee <[email protected]>

---

More information:

I originally noticed this problem on a custom 2430 board I am working with. Whenever an i2c chip driver calls i2c_probe() the device would be found on both i2c busses at each of the possible addresses for the device. I discovered that NACK was being set in the status register but omap_i2c_xfer() still returned success because ARDY was also set.

Review

Quoted 8 lines

This patch fixes this issue on my board and hasn't caused any problems (in my admittedly light i2c usage). I don't have immediate access to an SDP board however, so it has not been tested on that platform.

 drivers/i2c/busses/i2c-omap.c |   28 +++++++++++++---------------
 1 files changed, 13 insertions(+), 15 deletions(-)
(quoted diff) +0 -0

b/drivers/i2c/busses/i2c-omap.c

drivers/i2c/busses/i2c-omap.c omap_i2c_isr(int this_irq, void *dev_id) · line 593

Quoted 9 lines

index 4777466..a16d513 100644

drivers/i2c/busses/i2c-omap.c +1 -1
@@ -590,7 +590,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 bits;
 	u16 stat, w;
-	int count = 0;
+	int err, count = 0;

int err = 0, count = 0; will be better and you avoid that err=0 before using err right below.

Quoted 12 lines
drivers/i2c/busses/i2c-omap.c +2 -3
 	if (dev->idle)
 		return IRQ_NONE;

@@ -605,10 +605,19 @@ omap_i2c_isr(int this_irq, void *dev_id)
 
 		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
 
-		if (stat & OMAP_I2C_STAT_ARDY) {
-			omap_i2c_complete_cmd(dev, 0);
-			continue;
+		err = 0;
+		if (stat & OMAP_I2C_STAT_NACK) {

drivers/i2c/busses/i2c-omap.c omap_i2c_isr(int this_irq, void *dev_id) · line 619

Quoted 10 lines
drivers/i2c/busses/i2c-omap.c +9 -0
+			err |= OMAP_I2C_STAT_NACK;
+			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
+					   OMAP_I2C_CON_STP);
 		}
+		if (stat & OMAP_I2C_STAT_AL) {
+			dev_err(dev->dev, "Arbitration lost\n");
+			err |= OMAP_I2C_STAT_AL;
+		}
+		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
+					OMAP_I2C_STAT_AL))

err should already hold them, how about
if (stat & err)

Quoted 45 lines
drivers/i2c/busses/i2c-omap.c +1 -1
+			omap_i2c_complete_cmd(dev, err);
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
 			if (dev->fifo_size) {

@@ -640,7 +649,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				}
 			}
 			omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_RRDY |
 OMAP_I2C_STAT_RDR));
-			continue;
 		}
 		if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
drivers/i2c/busses/i2c-omap.c +0 -1
 			u8 num_bytes = 1;

@@ -674,7 +682,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
 			omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY |
 OMAP_I2C_STAT_XDR));
-			continue;
 		}
 		if (stat & OMAP_I2C_STAT_ROVR) {
drivers/i2c/busses/i2c-omap.c +0 -10
 			dev_err(dev->dev, "Receive overrun\n");

@@ -684,15 +691,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			dev_err(dev->dev, "Transmit overflow\n");
 			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
 		}
-		if (stat & OMAP_I2C_STAT_NACK) {
-			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_NACK);
-			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
-					   OMAP_I2C_CON_STP);
-		}
-		if (stat & OMAP_I2C_STAT_AL) {
-			dev_err(dev->dev, "Arbitration lost\n");
-			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_AL);
-		}
 	}
 
 	return count ? IRQ_HANDLED : IRQ_NONE;
--

1.5.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 5 lines
Best Regards,

Felipe Balbi
http://felipebalbi.com
[email protected]
author
#2 lore raw
Quoted 5 lines · Felipe Balbi

drivers/i2c/busses/i2c-omap.c omap_i2c_isr(int this_irq, void *dev_id) · line 593

Quoted 2 lines
drivers/i2c/busses/i2c-omap.c +1 -1
-	int count = 0;
+	int err, count = 0;

int err = 0, count = 0; will be better and you avoid that err=0 before using err right below.

No, the err = 0 I added is in a while loop. It needs to be reinitialized for each iteration.

Quoted 5 lines

drivers/i2c/busses/i2c-omap.c omap_i2c_isr(int this_irq, void *dev_id) · line 619

Quoted 2 lines
drivers/i2c/busses/i2c-omap.c +2 -0
+		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
+					OMAP_I2C_STAT_AL))

err should already hold them, how about
if (stat & err)

err will not hold ARDY, so it would need to be

	if (stat & (err | OMAP_I2C_STAT_ARDY))

But I don't see the advantage. Comparing the assembly, what I submitted generates one less instruction than the above, so the difference appears to be trivial.

Cheers,
Seth

author
#3 lore raw
In reply to Seth Forshee
Quoted 4 lines · Seth Forshee

The IRQ handler in omap-i2c.c can sometimes clear status bits without actually processing them. In particular, error status bits will be ignored if any of the ARDY, RRDY, RDR, XRDY, or XDR bits are concurrently set.

Are there any more comments on the patch below? If there is a problem with style or implementation details I will fix them and submit a new patch, as long as the functionality isn't affected as with the previous comments. But it is possible for the error status bits to go unhandled and unreported (in fact it always happens for me with any address for which there is no slave present on a given bus), so I would think that it would be desirable to get a fix applied.

Cheers,
Seth

Quoted 90 lines
Signed-off-by Seth Forshee <[email protected]>

---

More information:

I originally noticed this problem on a custom 2430 board I am working with. Whenever an i2c chip driver calls i2c_probe() the device would be found on both i2c busses at each of the possible addresses for the device. I discovered that NACK was being set in the status register but omap_i2c_xfer() still returned success because ARDY was also set.

This patch fixes this issue on my board and hasn't caused any problems (in my admittedly light i2c usage). I don't have immediate access to an SDP board however, so it has not been tested on that platform.

 drivers/i2c/busses/i2c-omap.c |   28 +++++++++++++---------------
 1 files changed, 13 insertions(+), 15 deletions(-)
drivers/i2c/busses/i2c-omap.c +13 -15
index 4777466..a16d513 100644
@@ -590,7 +590,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 bits;
 	u16 stat, w;
-	int count = 0;
+	int err, count = 0;
 
 	if (dev->idle)
 		return IRQ_NONE;

@@ -605,10 +605,19 @@ omap_i2c_isr(int this_irq, void *dev_id)
 
 		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
 
-		if (stat & OMAP_I2C_STAT_ARDY) {
-			omap_i2c_complete_cmd(dev, 0);
-			continue;
+		err = 0;
+		if (stat & OMAP_I2C_STAT_NACK) {
+			err |= OMAP_I2C_STAT_NACK;
+			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
+					   OMAP_I2C_CON_STP);
 		}
+		if (stat & OMAP_I2C_STAT_AL) {
+			dev_err(dev->dev, "Arbitration lost\n");
+			err |= OMAP_I2C_STAT_AL;
+		}
+		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
+					OMAP_I2C_STAT_AL))
+			omap_i2c_complete_cmd(dev, err);
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
 			if (dev->fifo_size) {

@@ -640,7 +649,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				}
 			}
 			omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
-			continue;
 		}
 		if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
 			u8 num_bytes = 1;

@@ -674,7 +682,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
 			omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-			continue;
 		}
 		if (stat & OMAP_I2C_STAT_ROVR) {
 			dev_err(dev->dev, "Receive overrun\n");

@@ -684,15 +691,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			dev_err(dev->dev, "Transmit overflow\n");
 			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
 		}
-		if (stat & OMAP_I2C_STAT_NACK) {
-			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_NACK);
-			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
-					   OMAP_I2C_CON_STP);
-		}
-		if (stat & OMAP_I2C_STAT_AL) {
-			dev_err(dev->dev, "Arbitration lost\n");
-			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_AL);
-		}
 	}
 
 	return count ? IRQ_HANDLED : IRQ_NONE;
Signature · 1 line
1.5.2.5
maintainer
#4 lore raw
Quoted 6 lines · Seth Forshee
Quoted 4 lines · Seth Forshee

The IRQ handler in omap-i2c.c can sometimes clear status bits without actually processing them. In particular, error status bits will be ignored if any of the ARDY, RRDY, RDR, XRDY, or XDR bits are concurrently set.

Quoted 7 lines

Are there any more comments on the patch below? If there is a problem with style or implementation details I will fix them and submit a new patch, as long as the functionality isn't affected as with the previous comments. But it is possible for the error status bits to go unhandled and unreported (in fact it always happens for me with any address for which there is no slave present on a given bus), so I would think that it would be desirable to get a fix applied.

Pushing today.

Tony

Quoted 98 lines

Cheers,
Seth

Quoted 90 lines
Signed-off-by Seth Forshee <[email protected]>

---

More information:

I originally noticed this problem on a custom 2430 board I am working with. Whenever an i2c chip driver calls i2c_probe() the device would be found on both i2c busses at each of the possible addresses for the device. I discovered that NACK was being set in the status register but omap_i2c_xfer() still returned success because ARDY was also set.

This patch fixes this issue on my board and hasn't caused any problems (in my admittedly light i2c usage). I don't have immediate access to an SDP board however, so it has not been tested on that platform.

 drivers/i2c/busses/i2c-omap.c |   28 +++++++++++++---------------
 1 files changed, 13 insertions(+), 15 deletions(-)
drivers/i2c/busses/i2c-omap.c +13 -15
index 4777466..a16d513 100644
@@ -590,7 +590,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 bits;
 	u16 stat, w;
-	int count = 0;
+	int err, count = 0;
 
 	if (dev->idle)
 		return IRQ_NONE;

@@ -605,10 +605,19 @@ omap_i2c_isr(int this_irq, void *dev_id)
 
 		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
 
-		if (stat & OMAP_I2C_STAT_ARDY) {
-			omap_i2c_complete_cmd(dev, 0);
-			continue;
+		err = 0;
+		if (stat & OMAP_I2C_STAT_NACK) {
+			err |= OMAP_I2C_STAT_NACK;
+			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
+					   OMAP_I2C_CON_STP);
 		}
+		if (stat & OMAP_I2C_STAT_AL) {
+			dev_err(dev->dev, "Arbitration lost\n");
+			err |= OMAP_I2C_STAT_AL;
+		}
+		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
+					OMAP_I2C_STAT_AL))
+			omap_i2c_complete_cmd(dev, err);
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
 			if (dev->fifo_size) {

@@ -640,7 +649,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				}
 			}
 			omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
-			continue;
 		}
 		if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
 			u8 num_bytes = 1;

@@ -674,7 +682,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
 			omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-			continue;
 		}
 		if (stat & OMAP_I2C_STAT_ROVR) {
 			dev_err(dev->dev, "Receive overrun\n");

@@ -684,15 +691,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			dev_err(dev->dev, "Transmit overflow\n");
 			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
 		}
-		if (stat & OMAP_I2C_STAT_NACK) {
-			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_NACK);
-			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
-					   OMAP_I2C_CON_STP);
-		}
-		if (stat & OMAP_I2C_STAT_AL) {
-			dev_err(dev->dev, "Arbitration lost\n");
-			omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_AL);
-		}
 	}
 
 	return count ? IRQ_HANDLED : IRQ_NONE;
Signature · 1 line
1.5.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html