On Wed, Jul 21, 2010 at 4:22 PM, Nishanth Menon menon.nishanth@gmail.com wrote:
Steve, just minor nitpick below:
On 07/21/2010 02:25 PM, Steve Sakoman wrote:
In addition to modifying the init routine to follow the TRM recommendations, this patch adds a retry count to two loops to avoid the possibility of infinite loops. It also corrects error message typos in the i2c_write routine.
Signed-off-by: Steve Sakomansteve@sakoman.com
arch/arm/include/asm/arch-omap3/i2c.h | 4 ++- drivers/i2c/omap24xx_i2c.c | 37 ++++++++++++++++++++++---------- drivers/i2c/omap24xx_i2c.h | 4 +++ 3 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/arch/arm/include/asm/arch-omap3/i2c.h b/arch/arm/include/asm/arch-omap3/i2c.h index 7a4a73a..d2e7488 100644 --- a/arch/arm/include/asm/arch-omap3/i2c.h +++ b/arch/arm/include/asm/arch-omap3/i2c.h @@ -34,7 +34,9 @@ struct i2c { unsigned short stat; /* 0x08 */ unsigned short res3; unsigned short iv; /* 0x0C */
- unsigned short res4[3];
- unsigned short res4;
- unsigned short syss; /* 0x10 */
- unsigned short res4a;
unsigned short buf; /* 0x14 */ unsigned short res5; unsigned short cnt; /* 0x18 */ diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index 3256133..a91fe55 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -41,6 +41,7 @@ void i2c_init (int speed, int slaveadd) int psc, fsscll, fssclh; int hsscll = 0, hssclh = 0; u32 scll, sclh;
- int reset_timeout = 10;
I am guessing we can life with a u8, we timeout in 10ms.. would asking for a #define I2C_TIMEOUT_RESET is too much here?
I can't imagine why we would ever want to make this a config option!
In normal operation the loop is only traversed one time. I only added the timeout to prevent the possibility of an infinite loop.
/* Only handle standard, fast and high speeds */ if ((speed != OMAP_I2C_STANDARD)&& @@ -102,15 +103,22 @@ void i2c_init (int speed, int slaveadd) sclh = (unsigned int)fssclh; }
- writew(0x2,&i2c_base->sysc); /* for ES2 after soft reset */
- udelay(1000);
- writew(0x0,&i2c_base->sysc); /* will probably self clear but */
if (readw (&i2c_base->con)& I2C_CON_EN) { writew (0,&i2c_base->con); udelay (50000); }
- writew(0x2,&i2c_base->sysc); /* for ES2 after soft reset */
- udelay(1000);
- writew(I2C_CON_EN,&i2c_base->con);
- while (!(readw(&i2c_base->syss)& I2C_SYSS_RDONE)&& reset_timeout--) {
- if (reset_timeout<= 0)
- printf("ERROR: Timeout in soft-reset\n");
- udelay(1000);
- }
- writew(0,&i2c_base->con);
writew(psc,&i2c_base->psc); writew(scll,&i2c_base->scll); writew(sclh,&i2c_base->sclh); @@ -134,6 +142,7 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value) { int i2c_error = 0; u16 status;
- u16 retries;
u8 sounds good here - this does not seem to take more than 10..
/* wait until bus not busy */ wait_for_bb (); @@ -159,15 +168,16 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value) }
if (!i2c_error) {
- /* free bus, otherwise we can't use a combined transction */
- writew (0,&i2c_base->con);
- while (readw (&i2c_base->stat) || (readw (&i2c_base->con)& I2C_CON_MST)) {
- retries = 10;
#define sounds nice here..
- while ((readw(&i2c_base->stat)& 0x14) ||
- (readw(&i2c_base->con)& I2C_CON_MST)) {
udelay (10000); /* Have to clear pending interrupt to clear I2C_STAT */ writew (0xFFFF,&i2c_base->stat);
- if (!retries--)
- break;
}
do we just proceed in case of retry timeout?
Yes. Again this timeout was added to prevent an infinite loop hang.
Perhaps an error message if the timeout is triggered?
- wait_for_bb ();
/* set slave address */ writew (devaddr,&i2c_base->sa); /* read one byte from slave */ @@ -190,11 +200,14 @@ static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value) }
if (!i2c_error) {
- retries = 10;
writew (I2C_CON_EN,&i2c_base->con); while (readw (&i2c_base->stat) || (readw (&i2c_base->con)& I2C_CON_MST)) { udelay (10000); writew (0xFFFF,&i2c_base->stat);
- if (!retries--)
- break;
}
same comment on retry failure here..
Same response -- would you like to see an error message if the timeout is triggered?
} } @@ -276,7 +289,7 @@ static void flush_fifo(void) * you get a bus error */ while(1){
- stat = readw(&i2c_base->stat);
- stat = readw(&i2c_base->stat)& I2C_STAT_RRDY;
if(stat == I2C_STAT_RRDY){ #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ defined(CONFIG_OMAP44XX) @@ -357,18 +370,18 @@ int i2c_write (uchar chip, uint addr, int alen, uchar * buffer, int len) int i;
if (alen> 1) {
- printf ("I2C read: addr len %d not supported\n", alen);
- printf("I2C write: addr len %d not supported\n", alen);
err.. do we need this change?
I think so -- otherwise the i2c_write function is printing messages claiming to come from i2c_read! Same for the other messages you flagged.
Regards,
Steve