Moiz, Nishant, Jagadeesh, Looks like this patch has some serious problems.
On Mon, 22 Jun 2009, Sonasath, Moiz wrote: > This patch includes the workarround for I2C Errata 1.153: When an > XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG > > Signed-off-by: Jagadeesh Bhaskar Pakaravoor <[email protected]> > Signed-off by: Nishant Kamat <[email protected]> > Signed-off-by: Moiz Sonasath<[email protected]> > --- > drivers/i2c/busses/i2c-omap.c | 54 +++++++++++++++++++++++++++++++++++++--- > 1 files changed, 50 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index ece0125..e84836b 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -632,6 +632,37 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id) > #define omap_i2c_rev1_isr NULL > #endif > > +/* I2C 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. > + */ > + > +static int omap_i2c_wait_for_xudf(struct omap_i2c_dev *dev) > +{ > + u16 xudf; > + int counter = 500; Shouldn't the timeout threshold depend on the I2C bus speed and transmit FIFO threshold? > + > + /* We are in interrupt context. Wait for XUDF for max 7 msec */ There's no way an interrupt service routine should loop for up to 7 milliseconds. Why does it need to wait this long? Shouldn't this patch zero the transmit FIFO threshold to minimize the amount of time between XRDY/XDR and XUDF? > + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); > + while (!(xudf & OMAP_I2C_STAT_XUDF) && counter--) { Why is a separate I2C_STAT loop used here? Shouldn't this patch just use the existing I2C_STAT loop in the ISR and use a state variable to indicate the current state of the ISR? > + if (xudf & (OMAP_I2C_STAT_ROVR | OMAP_I2C_STAT_NACK | > + OMAP_I2C_STAT_AL)) > + return -EINVAL; > + udelay(10); udelay() in an ISR is a big red flag. Why is this needed? > + xudf = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); > + } > + > + if (!counter) { > + /* Clear Tx FIFO */ > + omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, > + OMAP_I2C_BUF_TXFIF_CLR); > + return -ETIMEDOUT; > + } > + > + return 0; > +} > + > static irqreturn_t > omap_i2c_isr(int this_irq, void *dev_id) > { > @@ -639,6 +670,7 @@ omap_i2c_isr(int this_irq, void *dev_id) > u16 bits; > u16 stat, w; > int err, count = 0; > + int error; > > if (dev->idle) > return IRQ_NONE; > @@ -647,7 +679,7 @@ omap_i2c_isr(int this_irq, void *dev_id) > while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) { > dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat); > if (count++ == 100) { > - dev_warn(dev->dev, "Too much work in one IRQ\n"); > + dev_dbg(dev->dev, "Too much work in one IRQ\n"); > break; > } > > @@ -715,11 +747,22 @@ omap_i2c_isr(int this_irq, void *dev_id) > OMAP_I2C_BUFSTAT_REG); > } > while (num_bytes) { > - num_bytes--; > w = 0; > if (dev->buf_len) { > + if (cpu_is_omap34xx()) { > + /* OMAP3430 Errata 1.153 */ What about this I2C IP block on previous chip versions? Is this problem also present on 2430, which also has FIFO transmit capability? > + error = > omap_i2c_wait_for_xudf(dev); > + if (error) { > + omap_i2c_ack_stat(dev, > stat & > + > (OMAP_I2C_STAT_XRDY | > + > OMAP_I2C_STAT_XDR)); > + dev_err(dev->dev, > "Transmit error\n"); > + > omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_XUDF); > + > + return IRQ_HANDLED; > + } > + } > w = *dev->buf++; > - dev->buf_len--; > /* Data reg from 2430 is 8 bit wide */ > if (!cpu_is_omap2430() && > !cpu_is_omap34xx()) { > @@ -728,6 +771,10 @@ omap_i2c_isr(int this_irq, void *dev_id) > dev->buf_len--; > } > } > + omap_i2c_write_reg(dev, > + OMAP_I2C_DATA_REG, w); > + num_bytes--; > + dev->buf_len--; > } else { > if (stat & OMAP_I2C_STAT_XRDY) > dev_err(dev->dev, > @@ -739,7 +786,6 @@ omap_i2c_isr(int this_irq, void *dev_id) > "data to send\n"); > break; > } > - omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w); > } > omap_i2c_ack_stat(dev, > stat & (OMAP_I2C_STAT_XRDY | > OMAP_I2C_STAT_XDR)); - Paul -- 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
