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

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

 



Sonasath, Moiz had written, on 07/15/2009 05:23 PM, the following:
Hello Nishant,

Comments inlined,

Regards
Moiz Sonasath


-----Original Message-----
From: Menon, Nishanth Sent: Wednesday, July 15, 2009 4:43 PM
To: Sonasath, Moiz
Cc: linux-omap@xxxxxxxxxxxxxxx; Kamat, Nishant; Paul Walmsley
Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153

Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following:
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
+							}
+							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.
[Moiz]
Yes, it is a busy wait loop. But AFAIK while we come to this part of
 > the code the only thing that can go wrong in the transfer is either
 > the device would go off suddenly (creating a NACK) or there is an
 > arbitration lost(AL). This loop takes care of these two error conditions.
 > Apart from these, if the hardware is functional, the XUDF bit should
 > be set as soon as the FIFO gets empty.
Correct me if I am missing something.
That is exactly what I was complaining about -> why not use the existing logic above in the code to take care of it as I indicated below? do you see a problem with the logic I send? it is lesser code and should IMHO take care of the same issue without the additional 3rd nested while loop
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;
+                       }
+

[Moiz]

In this alternate implementation if a NACK|AL is produced while waiting
> on the XUDF, it returns from the ISR (as per my patch [2/3])
> without ACKING the XRDY/XDR interrupts which would make it return
> to the ISR again and again which looks like a problem. To accommodate
>this implementation, we will have to ACK XRDY/XDR before returning from the ISR. ok, this is due to my [1] patch which should have handled this condition in the first place. I will rework my original [1] patch and resend it.

Regards,
Nishanth Menon
Ref:
[1] http://patchwork.kernel.org/patch/32332/
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux