[U-Boot] [PATCH 1/3] ARM: I2C: I2C Multi byte address support

Existing OMAP I2C driver does not support address length greater than one. Hence this patch is to add support for 2 byte address read/write.
Signed-off-by: Philip, Avinash avinashphilip@ti.com Signed-off-by: Hebbar, Gururaja gururaja.hebbar@ti.com Signed-off-by: Patil, Rachna rachna@ti.com --- drivers/i2c/omap24xx_i2c.c | 477 ++++++++++++++++++++++++++++---------------- drivers/i2c/omap24xx_i2c.h | 2 + 2 files changed, 306 insertions(+), 173 deletions(-)
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index 4ae237a..88e26b2 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -29,10 +29,11 @@
DECLARE_GLOBAL_DATA_PTR;
-#define I2C_TIMEOUT 1000 +#define I2C_TIMEOUT (1 << 31) +#define I2C_STAT_TIMEO 10
-static void wait_for_bb(void); -static u16 wait_for_pin(void); +static u32 wait_for_bb(void); +static u32 wait_for_status_mask(u16 mask); static void flush_fifo(void);
static struct i2c *i2c_base = (struct i2c *)I2C_DEFAULT_BASE; @@ -45,7 +46,6 @@ void i2c_init(int speed, int slaveadd) int psc, fsscll, fssclh; int hsscll = 0, hssclh = 0; u32 scll, sclh; - int timeout = I2C_TIMEOUT;
/* Only handle standard, fast and high speeds */ if ((speed != OMAP_I2C_STANDARD) && @@ -107,24 +107,14 @@ void i2c_init(int speed, int slaveadd) sclh = (unsigned int)fssclh; }
+ if (gd->flags & GD_FLG_RELOC) + bus_initialized[current_bus] = 1; + 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) && timeout--) { - if (timeout <= 0) { - printf("ERROR: Timeout in soft-reset\n"); - return; - } - udelay(1000); - } - - writew(0, &i2c_base->con); writew(psc, &i2c_base->psc); writew(scll, &i2c_base->scll); writew(sclh, &i2c_base->sclh); @@ -140,81 +130,6 @@ void i2c_init(int speed, int slaveadd) flush_fifo(); writew(0xFFFF, &i2c_base->stat); writew(0, &i2c_base->cnt); - - if (gd->flags & GD_FLG_RELOC) - bus_initialized[current_bus] = 1; -} - -static int i2c_read_byte(u8 devaddr, u8 regoffset, u8 *value) -{ - int i2c_error = 0; - u16 status; - - /* wait until bus not busy */ - wait_for_bb(); - - /* one byte only */ - writew(1, &i2c_base->cnt); - /* set slave address */ - writew(devaddr, &i2c_base->sa); - /* no stop bit needed here */ - writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | - I2C_CON_TRX, &i2c_base->con); - - /* send register offset */ - while (1) { - status = wait_for_pin(); - if (status == 0 || status & I2C_STAT_NACK) { - i2c_error = 1; - goto read_exit; - } - if (status & I2C_STAT_XRDY) { - /* Important: have to use byte access */ - writeb(regoffset, &i2c_base->data); - writew(I2C_STAT_XRDY, &i2c_base->stat); - } - if (status & I2C_STAT_ARDY) { - writew(I2C_STAT_ARDY, &i2c_base->stat); - break; - } - } - - /* set slave address */ - writew(devaddr, &i2c_base->sa); - /* read one byte from slave */ - writew(1, &i2c_base->cnt); - /* need stop bit here */ - writew(I2C_CON_EN | I2C_CON_MST | - I2C_CON_STT | I2C_CON_STP, - &i2c_base->con); - - /* receive data */ - while (1) { - status = wait_for_pin(); - if (status == 0 || status & I2C_STAT_NACK) { - i2c_error = 1; - goto read_exit; - } - if (status & I2C_STAT_RRDY) { -#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ - defined(CONFIG_OMAP44XX) - *value = readb(&i2c_base->data); -#else - *value = readw(&i2c_base->data); -#endif - writew(I2C_STAT_RRDY, &i2c_base->stat); - } - if (status & I2C_STAT_ARDY) { - writew(I2C_STAT_ARDY, &i2c_base->stat); - break; - } - } - -read_exit: - flush_fifo(); - writew(0xFFFF, &i2c_base->stat); - writew(0, &i2c_base->cnt); - return i2c_error; }
static void flush_fifo(void) @@ -241,32 +156,43 @@ static void flush_fifo(void)
int i2c_probe(uchar chip) { - u16 status; + u32 status; int res = 1; /* default = fail */
if (chip == readw(&i2c_base->oa)) return res;
/* wait until bus not busy */ - wait_for_bb(); + status = wait_for_bb(); + /* exit on BUS busy */ + if (status & I2C_TIMEOUT) + return res;
/* try to write one byte */ writew(1, &i2c_base->cnt); /* set slave address */ writew(chip, &i2c_base->sa); /* stop bit needed here */ - writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX | - I2C_CON_STP, &i2c_base->con); - - status = wait_for_pin(); - - /* check for ACK (!NAK) */ - if (!(status & I2C_STAT_NACK)) - res = 0; - - /* abort transfer (force idle state) */ - writew(0, &i2c_base->con); - + writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT + | I2C_CON_STP, &i2c_base->con); + /* enough delay for the NACK bit set */ + udelay(50000); + + if (!(readw(&i2c_base->stat) & I2C_STAT_NACK)) { + res = 0; /* success case */ + flush_fifo(); + writew(0xFFFF, &i2c_base->stat); + } else { + /* failure, clear sources*/ + writew(0xFFFF, &i2c_base->stat); + /* finish up xfer */ + writew(readw(&i2c_base->con) | I2C_CON_STP, &i2c_base->con); + udelay(20000); + status = wait_for_bb(); + /* exit on BUS busy */ + if (status & I2C_TIMEOUT) + return res; + } flush_fifo(); /* don't allow any more data in... we don't want it. */ writew(0, &i2c_base->cnt); @@ -276,143 +202,348 @@ int i2c_probe(uchar chip)
int i2c_read(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); + int i2c_error = 0, i; + u32 status; + + if ((alen > 2) || (alen < 0)) { return 1; }
- if (addr + len > 256) { - printf("I2C read: address out of range\n"); + if (alen < 2) { + if (addr + len > 256) { + return 1; + } + } else if (addr + len > 0xFFFF) { return 1; }
- for (i = 0; i < len; i++) { - if (i2c_read_byte(chip, addr + i, &buffer[i])) { - printf("I2C read: I/O error\n"); - i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); - return 1; + /* wait until bus not busy */ + status = wait_for_bb(); + + /* exit on BUS busy */ + if (status & I2C_TIMEOUT) + return 1; + + writew((alen & 0xFF), &i2c_base->cnt); + /* set slave address */ + writew(chip, &i2c_base->sa); + /* Clear the Tx & Rx FIFOs */ + writew((readw(&i2c_base->buf) | I2C_RXFIFO_CLEAR | + I2C_TXFIFO_CLEAR), &i2c_base->buf); + /* no stop bit needed here */ + writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_TRX | + I2C_CON_STT, &i2c_base->con); + + /* wait for Transmit ready condition */ + status = wait_for_status_mask(I2C_STAT_XRDY | I2C_STAT_NACK); + + if (status & (I2C_STAT_NACK | I2C_TIMEOUT)) + i2c_error = 1; + + if (!i2c_error) { + if (status & I2C_STAT_XRDY) { + switch (alen) { + case 2: + /* Send address MSByte */ +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) + writew(((addr >> 8) & 0xFF), &i2c_base->data); + + /* Clearing XRDY event */ + writew((status & I2C_STAT_XRDY), + &i2c_base->stat); + /* wait for Transmit ready condition */ + status = wait_for_status_mask(I2C_STAT_XRDY | + I2C_STAT_NACK); + + if (status & (I2C_STAT_NACK | + I2C_TIMEOUT)) { + i2c_error = 1; + break; + } +#endif + case 1: +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) + /* Send address LSByte */ + writew((addr & 0xFF), &i2c_base->data); +#else + /* Send address Short word */ + writew((addr & 0xFFFF), &i2c_base->data); +#endif + /* Clearing XRDY event */ + writew((status & I2C_STAT_XRDY), + &i2c_base->stat); + /*wait for Transmit ready condition */ + status = wait_for_status_mask(I2C_STAT_ARDY | + I2C_STAT_NACK); + + if (status & (I2C_STAT_NACK | + I2C_TIMEOUT)) { + i2c_error = 1; + break; + } + } + } else + i2c_error = 1; + } + + /* Wait for ARDY to set */ + status = wait_for_status_mask(I2C_STAT_ARDY | I2C_STAT_NACK + | I2C_STAT_AL); + + if (!i2c_error) { + /* set slave address */ + writew(chip, &i2c_base->sa); + writew((len & 0xFF), &i2c_base->cnt); + /* Clear the Tx & Rx FIFOs */ + writew((readw(&i2c_base->buf) | I2C_RXFIFO_CLEAR | + I2C_TXFIFO_CLEAR), &i2c_base->buf); + /* need stop bit here */ + writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP, + &i2c_base->con); + + for (i = 0; i < len; i++) { + /* wait for Receive condition */ + status = wait_for_status_mask(I2C_STAT_RRDY | + I2C_STAT_NACK); + + if (status & (I2C_STAT_NACK | I2C_TIMEOUT)) { + i2c_error = 1; + break; + } + + if (status & I2C_STAT_RRDY) { +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) + buffer[i] = readb(&i2c_base->data); +#else + *((u16 *)&buffer[i]) = + readw(&i2c_base->data) & 0xFFFF; + i++; +#endif + writew((status & I2C_STAT_RRDY), + &i2c_base->stat); + udelay(1000); + } else { + i2c_error = 1; + } + } + } + /* Wait for ARDY to set */ + status = wait_for_status_mask(I2C_STAT_ARDY | I2C_STAT_NACK + | I2C_STAT_AL); + + if (i2c_error) { + writew(0, &i2c_base->con); + return 1; + } + + if (!i2c_error) { + 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); } }
+ writew(I2C_CON_EN, &i2c_base->con); + flush_fifo(); + writew(0xFFFF, &i2c_base->stat); + writew(0, &i2c_base->cnt); + return 0; }
int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) { - int i; - u16 status; - int i2c_error = 0; + int i, i2c_error = 0; + u32 status; + u16 writelen;
- if (alen > 1) { - printf("I2C write: addr len %d not supported\n", alen); + if (alen > 2) { return 1; }
- if (addr + len > 256) { - printf("I2C write: address 0x%x + 0x%x out of range\n", - addr, len); + if (alen < 2) { + if (addr + len > 256) { + return 1; + } + } else if (addr + len > 0xFFFF) { return 1; }
/* wait until bus not busy */ - wait_for_bb(); + status = wait_for_bb(); + + /* exiting on BUS busy */ + if (status & I2C_TIMEOUT) + return 1; + + writelen = (len & 0xFFFF) + alen; + + /* two bytes */ + writew((writelen & 0xFFFF), &i2c_base->cnt); + /* Clear the Tx & Rx FIFOs */ + writew((readw(&i2c_base->buf) | I2C_RXFIFO_CLEAR | + I2C_TXFIFO_CLEAR), &i2c_base->buf);
- /* start address phase - will write regoffset + len bytes data */ - /* TODO consider case when !CONFIG_OMAP243X/34XX/44XX */ - writew(alen + len, &i2c_base->cnt); /* set slave address */ writew(chip, &i2c_base->sa); /* stop bit needed here */ writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX | I2C_CON_STP, &i2c_base->con);
- /* Send address byte */ - status = wait_for_pin();
- if (status == 0 || status & I2C_STAT_NACK) { + /* wait for Transmit ready condition */ + status = wait_for_status_mask(I2C_STAT_XRDY | I2C_STAT_NACK); + + if (status & (I2C_STAT_NACK | I2C_TIMEOUT)) i2c_error = 1; - printf("error waiting for i2c address ACK (status=0x%x)\n", - status); - goto write_exit; + + if (!i2c_error) { + if (status & I2C_STAT_XRDY) { + switch (alen) { +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) + case 2: + /* send out MSB byte */ + writeb(((addr >> 8) & 0xFF), &i2c_base->data); +#else + writeb((addr & 0xFFFF), &i2c_base->data); + break; +#endif + /* Clearing XRDY event */ + writew((status & I2C_STAT_XRDY), + &i2c_base->stat); + /*waiting for Transmit ready * condition */ + status = wait_for_status_mask(I2C_STAT_XRDY | + I2C_STAT_NACK); + + if (status & (I2C_STAT_NACK | I2C_TIMEOUT)) { + i2c_error = 1; + break; + } + case 1: +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) + /* send out MSB byte */ + writeb((addr & 0xFF), &i2c_base->data); +#else + writew(((buffer[0] << 8) | (addr & 0xFF)), + &i2c_base->data); +#endif + } + + /* Clearing XRDY event */ + writew((status & I2C_STAT_XRDY), &i2c_base->stat); + } + + /* waiting for Transmit ready condition */ + status = wait_for_status_mask(I2C_STAT_XRDY | I2C_STAT_NACK); + + if (status & (I2C_STAT_NACK | I2C_TIMEOUT)) + i2c_error = 1; + + if (!i2c_error) { + for (i = ((alen > 1) ? 0 : 1); i < len; i++) { + if (status & I2C_STAT_XRDY) { +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) + writeb((buffer[i] & 0xFF), + &i2c_base->data); +#else + writew((((buffer[i] << 8) | + buffer[i + 1]) & 0xFFFF), + &i2c_base->data); + i++; +#endif + } else + i2c_error = 1; + /* Clearing XRDY event */ + writew((status & I2C_STAT_XRDY), + &i2c_base->stat); + /* waiting for XRDY condition */ + status = wait_for_status_mask( + I2C_STAT_XRDY | + I2C_STAT_ARDY | + I2C_STAT_NACK); + if (status & (I2C_STAT_NACK | + I2C_TIMEOUT)) { + i2c_error = 1; + break; + } + if (status & I2C_STAT_ARDY) + break; + } + } }
- if (status & I2C_STAT_XRDY) { - writeb(addr & 0xFF, &i2c_base->data); - writew(I2C_STAT_XRDY, &i2c_base->stat); - } else { + status = wait_for_status_mask(I2C_STAT_ARDY | I2C_STAT_NACK | + I2C_STAT_AL); + + if (status & (I2C_STAT_NACK | I2C_TIMEOUT)) i2c_error = 1; - printf("i2c bus not ready for transmit (status=0x%x)\n", - status); - goto write_exit; + + if (i2c_error) { + writew(0, &i2c_base->con); + return 1; }
- /* address phase is over, now write data */ - for (i = 0; i < len; i++) { - status = wait_for_pin();
- if (status == 0 || status & I2C_STAT_NACK) { - i2c_error = 1; - printf("i2c error waiting for data ACK (status=0x%x)\n", - status); - goto write_exit; - } + if (!i2c_error) { + int eout = 200;
- if (status & I2C_STAT_XRDY) { - writeb(buffer[i], &i2c_base->data); - writew(I2C_STAT_XRDY, &i2c_base->stat); - } else { - i2c_error = 1; - printf("i2c bus not ready for Tx (i=%d)\n", i); - goto write_exit; + writew(I2C_CON_EN, &i2c_base->con); + while ((status = readw(&i2c_base->stat)) || + (readw(&i2c_base->con) & I2C_CON_MST)) { + udelay(1000); + /* have to read to clear intrrupt */ + writew(0xFFFF, &i2c_base->stat); + if (--eout == 0) + /* better leave with error than hang */ + break; } }
-write_exit: flush_fifo(); writew(0xFFFF, &i2c_base->stat); - return i2c_error; + writew(0, &i2c_base->cnt); + return 0; }
-static void wait_for_bb(void) +static u32 wait_for_bb(void) { - int timeout = I2C_TIMEOUT; - u16 stat; + int timeout = I2C_STAT_TIMEO; + u32 stat;
- writew(0xFFFF, &i2c_base->stat); /* clear current interrupts...*/ while ((stat = readw(&i2c_base->stat) & I2C_STAT_BB) && timeout--) { writew(stat, &i2c_base->stat); - udelay(1000); + udelay(50000); }
if (timeout <= 0) { printf("timed out in wait_for_bb: I2C_STAT=%x\n", readw(&i2c_base->stat)); + stat |= I2C_TIMEOUT; } writew(0xFFFF, &i2c_base->stat); /* clear delayed stuff*/ + return stat; }
-static u16 wait_for_pin(void) +static u32 wait_for_status_mask(u16 mask) { - u16 status; - int timeout = I2C_TIMEOUT; + u32 status; + int timeout = I2C_STAT_TIMEO;
do { udelay(1000); status = readw(&i2c_base->stat); - } while (!(status & - (I2C_STAT_ROVR | I2C_STAT_XUDF | I2C_STAT_XRDY | - I2C_STAT_RRDY | I2C_STAT_ARDY | I2C_STAT_NACK | - I2C_STAT_AL)) && timeout--); + } while (!(status & mask) && timeout--);
if (timeout <= 0) { printf("timed out in wait_for_pin: I2C_STAT=%x\n", readw(&i2c_base->stat)); writew(0xFFFF, &i2c_base->stat); - status = 0; + status |= I2C_TIMEOUT; } - return status; }
diff --git a/drivers/i2c/omap24xx_i2c.h b/drivers/i2c/omap24xx_i2c.h index 1f38c23..9a0b126 100644 --- a/drivers/i2c/omap24xx_i2c.h +++ b/drivers/i2c/omap24xx_i2c.h @@ -60,7 +60,9 @@ /* I2C Buffer Configuration Register (I2C_BUF): */
#define I2C_BUF_RDMA_EN (1 << 15) /* Receive DMA channel enable */ +#define I2C_RXFIFO_CLEAR (1 << 14) /* RX FIFO Clear */ #define I2C_BUF_XDMA_EN (1 << 7) /* Transmit DMA channel enable */ +#define I2C_TXFIFO_CLEAR (1 << 6) /* TX FIFO clear */
/* I2C Configuration Register (I2C_CON): */

Hello Patil,
Patil, Rachna wrote:
Existing OMAP I2C driver does not support address length greater than one. Hence this patch is to add support for 2 byte address read/write.
Signed-off-by: Philip, Avinash avinashphilip@ti.com Signed-off-by: Hebbar, Gururaja gururaja.hebbar@ti.com Signed-off-by: Patil, Rachna rachna@ti.com
drivers/i2c/omap24xx_i2c.c | 477 ++++++++++++++++++++++++++++---------------- drivers/i2c/omap24xx_i2c.h | 2 + 2 files changed, 306 insertions(+), 173 deletions(-)
Please check this patch with tools/checkpatch.pl, it throws some "WARNING: braces {} are not necessary for single statement blocks" warnings, please fix.
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index 4ae237a..88e26b2 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -29,10 +29,11 @@
DECLARE_GLOBAL_DATA_PTR;
-#define I2C_TIMEOUT 1000 +#define I2C_TIMEOUT (1 << 31)
? Why this? What has this to do with 2 address byte support?
Maybe you change the usage of this define from timeout to a bitmask? If so, please do this in a seperate patch.
+#define I2C_STAT_TIMEO 10
Could you explain, for what purpose this new timeout is?
-static void wait_for_bb(void); -static u16 wait_for_pin(void); +static u32 wait_for_bb(void); +static u32 wait_for_status_mask(u16 mask); static void flush_fifo(void);
static struct i2c *i2c_base = (struct i2c *)I2C_DEFAULT_BASE;
[...]
int i2c_probe(uchar chip) {
- writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT
| I2C_CON_STP, &i2c_base->con);
- /* enough delay for the NACK bit set */
- udelay(50000);
Huh... big delay? Is this really needed?
- if (!(readw(&i2c_base->stat) & I2C_STAT_NACK)) {
res = 0; /* success case */
flush_fifo();
writew(0xFFFF, &i2c_base->stat);
- } else {
/* failure, clear sources*/
writew(0xFFFF, &i2c_base->stat);
/* finish up xfer */
writew(readw(&i2c_base->con) | I2C_CON_STP, &i2c_base->con);
udelay(20000);
here too... are this values from a datasheet?
If I see this right, we have when probing a not used address, 70000 us timeout ...
Did you some timing tests between the old and the new version of this driver?
[...]
int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) {
[...]
- if (i2c_error) {
writew(0, &i2c_base->con);
return 1;
- }
- if (!i2c_error) {
else ?
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);
} }
writew(I2C_CON_EN, &i2c_base->con);
flush_fifo();
writew(0xFFFF, &i2c_base->stat);
writew(0, &i2c_base->cnt);
return 0;
}
int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) {
- int i;
- u16 status;
- int i2c_error = 0;
- int i, i2c_error = 0;
- u32 status;
- u16 writelen;
- if (alen > 1) {
printf("I2C write: addr len %d not supported\n", alen);
- if (alen > 2) { return 1; }
- if (addr + len > 256) {
printf("I2C write: address 0x%x + 0x%x out of range\n",
addr, len);
- if (alen < 2) {
if (addr + len > 256) {
return 1;
}
curly brackets not needed.
[...]
-static void wait_for_bb(void) +static u32 wait_for_bb(void) {
- int timeout = I2C_TIMEOUT;
- u16 stat;
- int timeout = I2C_STAT_TIMEO;
- u32 stat;
- writew(0xFFFF, &i2c_base->stat); /* clear current interrupts...*/ while ((stat = readw(&i2c_base->stat) & I2C_STAT_BB) && timeout--) { writew(stat, &i2c_base->stat);
udelay(1000);
udelay(50000);
Why such a new bigger timeout is needed? What has this to do with 2 byte address support?
[...]
bye, Heiko

Hi Heiko
On Fri, Jan 13, 2012 at 17:36:11, Heiko Schocher wrote:
Hello Patil,
Patil, Rachna wrote:
Existing OMAP I2C driver does not support address length greater than one. Hence this patch is to add support for 2 byte address read/write.
Signed-off-by: Philip, Avinash avinashphilip@ti.com Signed-off-by: Hebbar, Gururaja gururaja.hebbar@ti.com Signed-off-by: Patil, Rachna rachna@ti.com
drivers/i2c/omap24xx_i2c.c | 477 ++++++++++++++++++++++++++++---------------- drivers/i2c/omap24xx_i2c.h | 2 + 2 files changed, 306 insertions(+), 173 deletions(-)
Please check this patch with tools/checkpatch.pl, it throws some "WARNING: braces {} are not necessary for single statement blocks" warnings, please fix.
I will clean this up.
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index 4ae237a..88e26b2 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -29,10 +29,11 @@
DECLARE_GLOBAL_DATA_PTR;
-#define I2C_TIMEOUT 1000 +#define I2C_TIMEOUT (1 << 31)
? Why this? What has this to do with 2 address byte support?
Maybe you change the usage of this define from timeout to a bitmask? If so, please do this in a seperate patch.
Yes, I am using this to define a bitmask, but this applies to changes done in this patch.
+#define I2C_STAT_TIMEO 10
Could you explain, for what purpose this new timeout is?
I have interchanged the meaning of the above two definitions. I2C_STAT_TIMEO would reserve the purpose of I2C_TIMEOUT. To avoid confusion I will retain the original definition of I2C_TIMEOUT And add the bit definition to I2C_STAT_TIMEO, so that would be
#define I2C_TIMEOUT 10 #define I2C_STAT_TIMEO (1 << 31)
-static void wait_for_bb(void); -static u16 wait_for_pin(void); +static u32 wait_for_bb(void); +static u32 wait_for_status_mask(u16 mask); static void flush_fifo(void);
static struct i2c *i2c_base = (struct i2c *)I2C_DEFAULT_BASE;
[...]
int i2c_probe(uchar chip) {
- writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT
| I2C_CON_STP, &i2c_base->con);
- /* enough delay for the NACK bit set */
- udelay(50000);
Huh... big delay? Is this really needed?
I kept this delay big keeping in mind that some devices may need more time to respond. However I have tested by reducing the delay and is still works. I will reduce this to 9000.
- if (!(readw(&i2c_base->stat) & I2C_STAT_NACK)) {
res = 0; /* success case */
flush_fifo();
writew(0xFFFF, &i2c_base->stat);
- } else {
/* failure, clear sources*/
writew(0xFFFF, &i2c_base->stat);
/* finish up xfer */
writew(readw(&i2c_base->con) | I2C_CON_STP, &i2c_base->con);
udelay(20000);
here too... are this values from a datasheet?
If I see this right, we have when probing a not used address, 70000 us timeout ...
Did you some timing tests between the old and the new version of this driver?
These values are based on trial and error methods and not from any datasheet. This delay can be removed.
[...]
int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) {
[...]
- if (i2c_error) {
writew(0, &i2c_base->con);
return 1;
- }
- if (!i2c_error) {
else ?
Else, it would return 1, which is already taken care of. This if loop is not required. I will be deleting this.
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);
} }
writew(I2C_CON_EN, &i2c_base->con);
flush_fifo();
writew(0xFFFF, &i2c_base->stat);
writew(0, &i2c_base->cnt);
return 0;
}
int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) {
- int i;
- u16 status;
- int i2c_error = 0;
- int i, i2c_error = 0;
- u32 status;
- u16 writelen;
- if (alen > 1) {
printf("I2C write: addr len %d not supported\n", alen);
- if (alen > 2) { return 1; }
- if (addr + len > 256) {
printf("I2C write: address 0x%x + 0x%x out of range\n",
addr, len);
- if (alen < 2) {
if (addr + len > 256) {
return 1;
}
curly brackets not needed.
Yes, I will run a checkpatch and take care in v2.
[...]
-static void wait_for_bb(void) +static u32 wait_for_bb(void) {
- int timeout = I2C_TIMEOUT;
- u16 stat;
- int timeout = I2C_STAT_TIMEO;
- u32 stat;
- writew(0xFFFF, &i2c_base->stat); /* clear current interrupts...*/ while ((stat = readw(&i2c_base->stat) & I2C_STAT_BB) && timeout--) { writew(stat, &i2c_base->stat);
udelay(1000);
udelay(50000);
Why such a new bigger timeout is needed? What has this to do with 2 byte address support?
Agree, this does not have anything to do with two byte addressing support. I will retain the original value.
[...]
bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Regards, Rachna.
participants (2)
-
Heiko Schocher
-
Patil, Rachna