[PATCH] I2C: Fix OMAP I2C status register handling in IRQ processing

[PATCH] I2C: Fix OMAP I2C status register handling in IRQ processing

[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 <seth.forshee@gmail.com>
---

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(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 4777466..a16d513 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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;
-- 
1.5.2.5

Re: [PATCH] I2C: Fix OMAP I2C status register handling in IRQ processing

[Felipe Balbi]

On Wed, 12 Mar 2008 12:56:10 -0500, Seth Forshee <seth.forshee@gmail.com>
wrote:
> 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 <seth.forshee@gmail.com>
> ---
> 
> 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(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c
b/drivers/i2c/busses/i2c-omap.c
> index 4777466..a16d513 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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.

> 
>  	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))

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

> +			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;
> --
> 1.5.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Best Regards,

Felipe Balbi
http://felipebalbi.com
me@felipebalbi.com

Re: [PATCH] I2C: Fix OMAP I2C status register handling in IRQ processing

[Seth Forshee]

On Wed, Mar 12, 2008 at 01:14:32PM -0500, Felipe Balbi wrote:
> > -	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.

> > +		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

Re: [PATCH] I2C: Fix OMAP I2C status register handling in IRQ processing

[Seth Forshee]

On Wed, Mar 12, 2008 at 12:56:10PM -0500, Seth Forshee wrote:
> 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

> 
> Signed-off-by: Seth Forshee <seth.forshee@gmail.com>
> ---
> 
> 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(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 4777466..a16d513 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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;
> -- 
> 1.5.2.5
> 

Re: [PATCH] I2C: Fix OMAP I2C status register handling in IRQ processing

[Tony Lindgren]

* Seth Forshee <seth.forshee@gmail.com> [080318 05:39]:
> On Wed, Mar 12, 2008 at 12:56:10PM -0500, Seth Forshee wrote:
> > 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.

Pushing today.

Tony

> 
> Cheers,
> Seth
> 
> > 
> > Signed-off-by: Seth Forshee <seth.forshee@gmail.com>
> > ---
> > 
> > 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(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index 4777466..a16d513 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -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;
> > -- 
> > 1.5.2.5
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html