Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

[Nishanth Menon <nm@ti.com>]

Sonasath, Moiz wrote:
> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG.
> Otherwise some data bytes can be lost while transferring them from the
> memory to the I2C interface.
> 
> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting
> if there is NACK | AL, set the appropriate error flags, ack the pending
> interrupts and return from the ISR.
> 
> Signed-off-by: Moiz Sonasath<m-sonasath@ti.com>
> Signed-off-by: Vikram Pandita<vikram.pandita@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   24 +++++++++++++++++++++++-
>  1 files changed, 23 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 05b5e4c..8deaf87 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  			break;
>  		}
>  
> +		err = 0;
> +complete:
cant we rename this label?
>  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
>  
> -		err = 0;
>  		if (stat & OMAP_I2C_STAT_NACK) {
>  			err |= OMAP_I2C_STAT_NACK;
>  			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> @@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  							"data to send\n");
>  					break;
>  				}
> +
> +				/*
> +				 * OMAP3430 Errata 1.153: When an XRDY/XDR
> +				 * is hit, wait for XUDF before writing data
> +				 * to DATA_REG. Otherwise some data bytes can
> +				 * be lost while transferring them from the
> +				 * memory to the I2C interface.
> +				 */
> +
> +				if (cpu_is_omap34xx()) {
> +						while (!(stat & OMAP_I2C_STAT_XUDF)) {
> +							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
> +								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
generic comment - code nesting is getting overwhelming - we may like to 
refactor the isr function at a later date I think..
> +								err |= OMAP_I2C_STAT_XUDF;
why set the err if we wantedly force this to happen?
> +								goto complete;

> +							}
> +							cpu_relax();
> +							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> +						}
this is an infinite while loop + it tries to handle error cases - 
essentially do another isr routine inside itself.


How about:
Apply [1] and the following?
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8d201..e3f21af 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
                 }
                 if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
                         u8 num_bytes = 1;
+                       if (cpu_is_omap34xx() &&
+                               !(stat & OMAP_I2C_STAT_XUDF)){
+                               cpu_relax();
+                               continue;
+                       }
+
                         if (dev->fifo_size) {
                                 if (stat & OMAP_I2C_STAT_XRDY)
                                         num_bytes = dev->fifo_size;


Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/