[U-Boot] [PATCH V5] ARM: OMAP: I2C: New read, write and probe functions

New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older OMAPs and derivatives as well. The only anticipated exception would be the OMAP2420, which shall require driver modification. - Rewritten i2c_read to operate correctly with all types of chips (old function could not read consistent data from some I2C slaves). - Optimised i2c_write. - New i2c_probe, performs write access vs read. The old probe could hang the system under certain conditions (e.g. unconfigured pads). - The read/write/probe functions try to identify unconfigured bus. - Status functions now read irqstatus_raw as per TRM guidelines (except for OMAP243X and OMAP34XX). - Driver now supports up to I2C5 (OMAP5).
Signed-off-by: Lubomir Popov lpopov@mm-sol.com --- V5 changes: - Replaced some printf() with puts(). - Minor formatting touches, checkpatch-clean. V4 changes: - New i2c_probe is built unconditionally, old code is removed. CONFIG_I2C_PROBE_WRITE is no longer needed. - Added a small delay to work around breakage in AM335X SPL. - Some whitespace and general formatting cleanup. V3 changes: - Removed old functions and conditional compilation. New functions are now built unconditionally for all SoCs using this driver. The only chip that should break is the OMAP2420 dinosaur. - Interrupts are enabled for OMAP243X and OMAP34XX only (where we don't have an irqstatus_raw register). V2 changes: - Probe tries to identify misconfigured pads as well. - Status is retrieved from irqstatus_raw rather than from stat. - Some minor style & format changes.
drivers/i2c/omap24xx_i2c.c | 490 +++++++++++++++++++++++++++------------------ 1 file changed, 299 insertions(+), 191 deletions(-)
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index 54e9b15..ef38d71 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -18,6 +18,20 @@ * * Adapted for OMAP2420 I2C, r-woodruff2@ti.com * + * Copyright (c) 2013 Lubomir Popov lpopov@mm-sol.com, MM Solutions + * New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 + * (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older + * OMAPs and derivatives as well. The only anticipated exception would + * be the OMAP2420, which shall require driver modification. + * - Rewritten i2c_read to operate correctly with all types of chips + * (old function could not read consistent data from some I2C slaves). + * - Optimized i2c_write. + * - New i2c_probe, performs write access vs read. The old probe could + * hang the system under certain conditions (e.g. unconfigured pads). + * - The read/write/probe functions try to identify unconfigured bus. + * - Status functions now read irqstatus_raw as per TRM guidelines + * (except for OMAP243X and OMAP34XX). + * - Driver now supports up to I2C5 (OMAP5). */
#include <common.h> @@ -31,8 +45,11 @@ DECLARE_GLOBAL_DATA_PTR;
#define I2C_TIMEOUT 1000
+/* Absolutely safe for status update at 100 kHz I2C: */ +#define I2C_WAIT 200 + static int wait_for_bb(void); -static u16 wait_for_pin(void); +static u16 wait_for_event(void); static void flush_fifo(void);
/* @@ -137,10 +154,14 @@ void i2c_init(int speed, int slaveadd) /* own address */ writew(slaveadd, &i2c_base->oa); writew(I2C_CON_EN, &i2c_base->con); - - /* have to enable intrrupts or OMAP i2c module doesn't work */ +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) + /* + * Have to enable interrupts for OMAP2/3, these IPs don't have + * an 'irqstatus_raw' register and we shall have to poll 'stat' + */ writew(I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE | - I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c_base->ie); + I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c_base->ie); +#endif udelay(1000); flush_fifo(); writew(0xFFFF, &i2c_base->stat); @@ -150,88 +171,6 @@ void i2c_init(int speed, int slaveadd) bus_initialized[current_bus] = 1; }
-static int i2c_read_byte(u8 devaddr, u16 regoffset, u8 alen, u8 *value) -{ - int i2c_error = 0; - u16 status; - int i = 2 - alen; - u8 tmpbuf[2] = {(regoffset) >> 8, regoffset & 0xff}; - u16 w; - - /* wait until bus not busy */ - if (wait_for_bb()) - return 1; - - /* one byte only */ - writew(alen, &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) { - w = tmpbuf[i++]; -#if !(defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ - defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) || \ - defined(CONFIG_OMAP54XX)) - w |= tmpbuf[i++] << 8; -#endif - writew(w, &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) || defined(CONFIG_AM33XX) || \ - defined(CONFIG_OMAP54XX) - *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) { u16 stat;
@@ -241,13 +180,7 @@ static void flush_fifo(void) while (1) { stat = readw(&i2c_base->stat); if (stat == I2C_STAT_RRDY) { -#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ - defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) || \ - defined(CONFIG_OMAP54XX) readb(&i2c_base->data); -#else - readw(&i2c_base->data); -#endif writew(I2C_STAT_RRDY, &i2c_base->stat); udelay(1000); } else @@ -255,6 +188,10 @@ static void flush_fifo(void) } }
+/* + * i2c_probe: Use write access. Allows to identify addresses that are + * write-only (like the config register of dual-port EEPROMs) + */ int i2c_probe(uchar chip) { u16 status; @@ -263,61 +200,81 @@ int i2c_probe(uchar chip) if (chip == readw(&i2c_base->oa)) return res;
- /* wait until bus not busy */ + /* Wait until bus is free */ if (wait_for_bb()) return res;
- /* try to read one byte */ - writew(1, &i2c_base->cnt); - /* set slave address */ + /* No data transfer, slave addr only */ + writew(0, &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_STP, &i2c_base->con); - - while (1) { - status = wait_for_pin(); - if (status == 0 || status & I2C_STAT_AL) { - res = 1; - goto probe_exit; - } - if (status & I2C_STAT_NACK) { - res = 1; - writew(0xff, &i2c_base->stat); - writew (readw (&i2c_base->con) | I2C_CON_STP, &i2c_base->con); - - if (wait_for_bb()) - res = 1; - - break; - } - if (status & I2C_STAT_ARDY) { - writew(I2C_STAT_ARDY, &i2c_base->stat); - break; - } - if (status & I2C_STAT_RRDY) { - res = 0; -#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ - defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) || \ - defined(CONFIG_OMAP54XX) - readb(&i2c_base->data); -#else - readw(&i2c_base->data); -#endif - writew(I2C_STAT_RRDY, &i2c_base->stat); - } + /* 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_event(); + + if ((status & ~I2C_STAT_XRDY) == 0 || (status & I2C_STAT_AL)) { + /* + * With current high-level command implementation, notifying + * the user shall flood the console with 127 messages. If + * silent exit is desired upon unconfigured bus, remove the + * following 'if' section: + */ + if (status == I2C_STAT_XRDY) + printf("i2c_probe: pads on bus %d probably not configured (status=0x%x)\n", + current_bus, status); + + goto pr_exit; }
-probe_exit: + /* Check for ACK (!NAK) */ + if (!(status & I2C_STAT_NACK)) { + res = 0; /* Device found */ + udelay(I2C_WAIT); /* Required by AM335X in SPL */ + /* Abort transfer (force idle state) */ + writew(I2C_CON_MST | I2C_CON_TRX, &i2c_base->con); /* Reset */ + udelay(1000); + writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_TRX | + I2C_CON_STP, &i2c_base->con); /* STP */ + } +pr_exit: flush_fifo(); - /* don't allow any more data in... we don't want it. */ - writew(0, &i2c_base->cnt); writew(0xFFFF, &i2c_base->stat); + writew(0, &i2c_base->cnt); return res; }
+/* + * i2c_read: Function now uses a single I2C read transaction with bulk transfer + * of the requested number of bytes (note that the 'i2c md' command + * limits this to 16 bytes anyway). If CONFIG_I2C_REPEATED_START is + * defined in the board config header, this transaction shall be with + * Repeated Start (Sr) between the address and data phases; otherwise + * Stop-Start (P-S) shall be used (some I2C chips do require a P-S). + * The address (reg offset) may be 0, 1 or 2 bytes long. + * Function now reads correctly from chips that return more than one + * byte of data per addressed register (like TI temperature sensors), + * or that do not need a register address at all (such as some clock + * distributors). + */ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) { - int i; + int i2c_error = 0; + u16 status; + + if (alen < 0) { + puts("I2C read: addr len < 0\n"); + return 1; + } + if (len < 0) { + puts("I2C read: data len < 0\n"); + return 1; + } + if (buffer == NULL) { + puts("I2C read: NULL pointer passed\n"); + return 1; + }
if (alen > 2) { printf("I2C read: addr len %d not supported\n", alen); @@ -329,24 +286,122 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) return 1; }
- for (i = 0; i < len; i++) { - if (i2c_read_byte(chip, addr + i, alen, &buffer[i])) { - puts("I2C read: I/O error\n"); - i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); - return 1; + /* Wait until bus not busy */ + if (wait_for_bb()) + return 1; + + /* Zero, one or two bytes reg address (offset) */ + writew(alen, &i2c_base->cnt); + /* Set slave address */ + writew(chip, &i2c_base->sa); + + if (alen) { + /* Must write reg offset first */ +#ifdef CONFIG_I2C_REPEATED_START + /* No stop bit, use Repeated Start (Sr) */ + writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | + I2C_CON_TRX, &i2c_base->con); +#else + /* Stop - Start (P-S) */ + writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP | + I2C_CON_TRX, &i2c_base->con); +#endif + /* Send register offset */ + while (1) { + status = wait_for_event(); + /* Try to identify bus that is not padconf'd for I2C */ + if (status == I2C_STAT_XRDY) { + i2c_error = 2; + printf("i2c_read (addr phase): pads on bus %d probably not configured (status=0x%x)\n", + current_bus, status); + goto rd_exit; + } + if (status == 0 || status & I2C_STAT_NACK) { + i2c_error = 1; + printf("i2c_read: error waiting for addr ACK (status=0x%x)\n", + status); + goto rd_exit; + } + if (alen) { + if (status & I2C_STAT_XRDY) { + alen--; + /* Do we have to use byte access? */ + writeb((addr >> (8 * alen)) & 0xff, + &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(chip, &i2c_base->sa); + /* Read len bytes from slave */ + writew(len, &i2c_base->cnt); + /* Need stop bit here */ + writew(I2C_CON_EN | I2C_CON_MST | + I2C_CON_STT | I2C_CON_STP, + &i2c_base->con);
- return 0; + /* Receive data */ + while (1) { + status = wait_for_event(); + /* + * Try to identify bus that is not padconf'd for I2C. This + * state could be left over from previous transactions if + * the address phase is skipped due to alen=0. + */ + if (status == I2C_STAT_XRDY) { + i2c_error = 2; + printf("i2c_read (data phase): pads on bus %d probably not configured (status=0x%x)\n", + current_bus, status); + goto rd_exit; + } + if (status == 0 || status & I2C_STAT_NACK) { + i2c_error = 1; + goto rd_exit; + } + if (status & I2C_STAT_RRDY) { + *buffer++ = readb(&i2c_base->data); + writew(I2C_STAT_RRDY, &i2c_base->stat); + } + if (status & I2C_STAT_ARDY) { + writew(I2C_STAT_ARDY, &i2c_base->stat); + break; + } + } + +rd_exit: + flush_fifo(); + writew(0xFFFF, &i2c_base->stat); + writew(0, &i2c_base->cnt); + return i2c_error; }
+/* i2c_write: Address (reg offset) may be 0, 1 or 2 bytes long. */ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) { int i; u16 status; int i2c_error = 0; - u16 w; - u8 tmpbuf[2] = {addr >> 8, addr & 0xff}; + + if (alen < 0) { + puts("I2C write: addr len < 0\n"); + return 1; + } + + if (len < 0) { + puts("I2C write: data len < 0\n"); + return 1; + } + + if (buffer == NULL) { + puts("I2C write: NULL pointer passed\n"); + return 1; + }
if (alen > 2) { printf("I2C write: addr len %d not supported\n", alen); @@ -355,92 +410,137 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
if (addr + len > (1 << 16)) { printf("I2C write: address 0x%x + 0x%x out of range\n", - addr, len); + addr, len); return 1; }
- /* wait until bus not busy */ + /* Wait until bus not busy */ if (wait_for_bb()) return 1;
- /* start address phase - will write regoffset + len bytes data */ - /* TODO consider case when !CONFIG_OMAP243X/34XX/44XX */ + /* Start address phase - will write regoffset + len bytes data */ writew(alen + len, &i2c_base->cnt); - /* set slave address */ + /* Set slave address */ writew(chip, &i2c_base->sa); - /* stop bit needed here */ + /* 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 and data */ - for (i = -alen; i < len; i++) { - status = wait_for_pin(); - + I2C_CON_STP, &i2c_base->con); + + while (alen) { + /* Must write reg offset (one or two bytes) */ + status = wait_for_event(); + /* Try to identify bus that is not padconf'd for I2C */ + if (status == I2C_STAT_XRDY) { + i2c_error = 2; + printf("i2c_write: pads on bus %d probably not configured (status=0x%x)\n", + current_bus, status); + goto wr_exit; + } 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; + printf("i2c_write: error waiting for addr ACK (status=0x%x)\n", + status); + goto wr_exit; } - if (status & I2C_STAT_XRDY) { - w = (i < 0) ? tmpbuf[2+i] : buffer[i]; -#if !(defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \ - defined(CONFIG_OMAP44XX) || defined(CONFIG_AM33XX) || \ - defined(CONFIG_OMAP54XX)) - w |= ((++i < 0) ? tmpbuf[2+i] : buffer[i]) << 8; -#endif - writew(w, &i2c_base->data); + alen--; + writeb((addr >> (8 * alen)) & 0xff, &i2c_base->data); + writew(I2C_STAT_XRDY, &i2c_base->stat); + } else { + i2c_error = 1; + printf("i2c_write: bus not ready for addr Tx (status=0x%x)\n", + status); + goto wr_exit; + } + } + /* Address phase is over, now write data */ + for (i = 0; i < len; i++) { + status = wait_for_event(); + if (status == 0 || status & I2C_STAT_NACK) { + i2c_error = 1; + printf("i2c_write: error waiting for data ACK (status=0x%x)\n", + status); + goto wr_exit; + } + 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; + printf("i2c_write: bus not ready for data Tx (i=%d)\n", + i); + goto wr_exit; } }
-write_exit: +wr_exit: flush_fifo(); writew(0xFFFF, &i2c_base->stat); + writew(0, &i2c_base->cnt); return i2c_error; }
+/* + * Wait for the bus to be free by checking the Bus Busy (BB) + * bit to become clear + */ static int wait_for_bb(void) { int timeout = I2C_TIMEOUT; u16 stat;
writew(0xFFFF, &i2c_base->stat); /* clear current interrupts...*/ +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) while ((stat = readw(&i2c_base->stat) & I2C_STAT_BB) && timeout--) { +#else + /* Read RAW status */ + while ((stat = readw(&i2c_base->irqstatus_raw) & + I2C_STAT_BB) && timeout--) { +#endif writew(stat, &i2c_base->stat); - udelay(1000); + udelay(I2C_WAIT); }
if (timeout <= 0) { - printf("timed out in wait_for_bb: I2C_STAT=%x\n", - readw(&i2c_base->stat)); + printf("Timed out in wait_for_bb: status=%04x\n", + stat); return 1; } writew(0xFFFF, &i2c_base->stat); /* clear delayed stuff*/ return 0; }
-static u16 wait_for_pin(void) +/* + * Wait for the I2C controller to complete current action + * and update status + */ +static u16 wait_for_event(void) { u16 status; int timeout = I2C_TIMEOUT;
do { - udelay(1000); + udelay(I2C_WAIT); +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) status = readw(&i2c_base->stat); +#else + /* Read RAW status */ + status = readw(&i2c_base->irqstatus_raw); +#endif } while (!(status & (I2C_STAT_ROVR | I2C_STAT_XUDF | I2C_STAT_XRDY | I2C_STAT_RRDY | I2C_STAT_ARDY | I2C_STAT_NACK | I2C_STAT_AL)) && timeout--);
if (timeout <= 0) { - printf("timed out in wait_for_pin: I2C_STAT=%x\n", - readw(&i2c_base->stat)); + printf("Timed out in wait_for_event: status=%04x\n", + status); + /* + * If status is still 0 here, probably the bus pads have + * not been configured for I2C, and/or pull-ups are missing. + */ + printf("Check if pads/pull-ups of bus %d are properly configured\n", + current_bus); writew(0xFFFF, &i2c_base->stat); status = 0; } @@ -450,28 +550,36 @@ static u16 wait_for_pin(void)
int i2c_set_bus_num(unsigned int bus) { - if ((bus < 0) || (bus >= I2C_BUS_MAX)) { - printf("Bad bus: %d\n", bus); + if (bus >= I2C_BUS_MAX) { + printf("Bad bus: %x\n", bus); return -1; }
-#if I2C_BUS_MAX == 4 - if (bus == 3) - i2c_base = (struct i2c *)I2C_BASE4; - else - if (bus == 2) + switch (bus) { + default: + bus = 0; /* Fall through */ + case 0: + i2c_base = (struct i2c *)I2C_BASE1; + break; + case 1: + i2c_base = (struct i2c *)I2C_BASE2; + break; +#if (I2C_BUS_MAX > 2) + case 2: i2c_base = (struct i2c *)I2C_BASE3; - else + break; +#if (I2C_BUS_MAX > 3) + case 3: + i2c_base = (struct i2c *)I2C_BASE4; + break; +#if (I2C_BUS_MAX > 4) + case 4: + i2c_base = (struct i2c *)I2C_BASE5; + break; #endif -#if I2C_BUS_MAX == 3 - if (bus == 2) - i2c_base = (struct i2c *)I2C_BASE3; - else #endif - if (bus == 1) - i2c_base = (struct i2c *)I2C_BASE2; - else - i2c_base = (struct i2c *)I2C_BASE1; +#endif + }
current_bus = bus;

Hello Lubomir,
Am 01.06.2013 18:44, schrieb Lubomir Popov:
New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older OMAPs and derivatives as well. The only anticipated exception would be the OMAP2420, which shall require driver modification.
- Rewritten i2c_read to operate correctly with all types of chips (old function could not read consistent data from some I2C slaves).
- Optimised i2c_write.
- New i2c_probe, performs write access vs read. The old probe could hang the system under certain conditions (e.g. unconfigured pads).
- The read/write/probe functions try to identify unconfigured bus.
- Status functions now read irqstatus_raw as per TRM guidelines (except for OMAP243X and OMAP34XX).
- Driver now supports up to I2C5 (OMAP5).
Signed-off-by: Lubomir Popov lpopov@mm-sol.com
V5 changes:
- Replaced some printf() with puts().
- Minor formatting touches, checkpatch-clean.
V4 changes:
- New i2c_probe is built unconditionally, old code is removed. CONFIG_I2C_PROBE_WRITE is no longer needed.
- Added a small delay to work around breakage in AM335X SPL.
- Some whitespace and general formatting cleanup.
V3 changes:
- Removed old functions and conditional compilation. New functions are now built unconditionally for all SoCs using this driver. The only chip that should break is the OMAP2420 dinosaur.
- Interrupts are enabled for OMAP243X and OMAP34XX only (where we don't have an irqstatus_raw register).
V2 changes:
- Probe tries to identify misconfigured pads as well.
- Status is retrieved from irqstatus_raw rather than from stat.
- Some minor style & format changes.
drivers/i2c/omap24xx_i2c.c | 490 +++++++++++++++++++++++++++------------------ 1 file changed, 299 insertions(+), 191 deletions(-)
Tested on 3 arm335x based boards, which one uses i2c in SPL code for getting ram parameters, so:
Tested-by: Heiko Schocher hs@denx.de
Just one comment: Your patch has 9 checkpatch warnings which are all lines (printf strings) over 80 chars ... some with lines > 110 characters ... I know, tom gave you a OK for this ... I am also unhappy with splitting a printf-string over 2 or more lines ... but we have this 80 characters rule ... Wolfgang, what do you think? Should we loosen this rule for printf-strings?
bye, Heiko

Hello Lubomir,
Am 01.06.2013 18:44, schrieb Lubomir Popov:
New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older OMAPs and derivatives as well. The only anticipated exception would be the OMAP2420, which shall require driver modification.
- Rewritten i2c_read to operate correctly with all types of chips (old function could not read consistent data from some I2C slaves).
- Optimised i2c_write.
- New i2c_probe, performs write access vs read. The old probe could hang the system under certain conditions (e.g. unconfigured pads).
- The read/write/probe functions try to identify unconfigured bus.
- Status functions now read irqstatus_raw as per TRM guidelines (except for OMAP243X and OMAP34XX).
- Driver now supports up to I2C5 (OMAP5).
Signed-off-by: Lubomir Popov lpopov@mm-sol.com
V5 changes:
- Replaced some printf() with puts().
- Minor formatting touches, checkpatch-clean.
V4 changes:
- New i2c_probe is built unconditionally, old code is removed. CONFIG_I2C_PROBE_WRITE is no longer needed.
- Added a small delay to work around breakage in AM335X SPL.
- Some whitespace and general formatting cleanup.
V3 changes:
- Removed old functions and conditional compilation. New functions are now built unconditionally for all SoCs using this driver. The only chip that should break is the OMAP2420 dinosaur.
- Interrupts are enabled for OMAP243X and OMAP34XX only (where we don't have an irqstatus_raw register).
V2 changes:
- Probe tries to identify misconfigured pads as well.
- Status is retrieved from irqstatus_raw rather than from stat.
- Some minor style & format changes.
drivers/i2c/omap24xx_i2c.c | 490 +++++++++++++++++++++++++++------------------ 1 file changed, 299 insertions(+), 191 deletions(-)
Tested on 3 arm335x based boards, which one uses i2c in SPL code for getting ram parameters, so:
Tested-by: Heiko Schocher hs@denx.de
Many thanks for testing, to Tom as well (he did it on the Beagleboards, but for one of the older versions, V3 I believe, right?). When it comes to versions, I see that V1 and V2 are still listed in patchwork, probably because of slightly different wording of the subject: http://patchwork.ozlabs.org/patch/233823/ http://patchwork.ozlabs.org/patch/246204/ Could you or Tom clean this up, please? Thanks.
Just one comment: Your patch has 9 checkpatch warnings which are all lines (printf strings) over 80 chars ... some with lines > 110 characters ... I know, tom gave you a OK for this ... I am also unhappy with splitting a printf-string over 2 or more lines ... but we have this 80 characters rule ... Wolfgang, what do you think? Should we loosen this rule for printf-strings?
Yes, I had the long strings splitted in the older versions, but then unrolled them back as per Tom's recommendation. IMHO, grep-ability is worth breaking this particular rule... But perhaps only for pure strings w/o format placeholders? I mean, strings could be splitted at the format parameters of printf/sprintf arguments.
bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Best regards, Lubo

Hello Lubomir,
Am 02.06.2013 13:42, schrieb Lubomir Popov:
Hello Lubomir,
Am 01.06.2013 18:44, schrieb Lubomir Popov:
New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older OMAPs and derivatives as well. The only anticipated exception would be the OMAP2420, which shall require driver modification.
[...]
drivers/i2c/omap24xx_i2c.c | 490 +++++++++++++++++++++++++++------------------ 1 file changed, 299 insertions(+), 191 deletions(-)
Tested on 3 arm335x based boards, which one uses i2c in SPL code for getting ram parameters, so:
Tested-by: Heiko Schocher hs@denx.de
Many thanks for testing, to Tom as well (he did it on the Beagleboards, but for one of the older versions, V3 I believe, right?). When it comes to versions, I see that V1 and V2 are still listed in patchwork, probably because of slightly different wording of the subject: http://patchwork.ozlabs.org/patch/233823/ http://patchwork.ozlabs.org/patch/246204/ Could you or Tom clean this up, please? Thanks.
Cleared the v3 v4 version, but missed the v2 and v1, Done.
Just one comment: Your patch has 9 checkpatch warnings which are all lines (printf strings) over 80 chars ... some with lines > 110 characters ... I know, tom gave you a OK for this ... I am also unhappy with splitting a printf-string over 2 or more lines ... but we have this 80 characters rule ... Wolfgang, what do you think? Should we loosen this rule for printf-strings?
Yes, I had the long strings splitted in the older versions, but then unrolled them back as per Tom's recommendation. IMHO, grep-ability is worth breaking this particular rule... But perhaps only for pure
Yep.
strings w/o format placeholders? I mean, strings could be splitted at the format parameters of printf/sprintf arguments.
bye, Heiko

Hello Lubomir,
Am 03.06.2013 07:13, schrieb Heiko Schocher:
Hello Lubomir,
Am 02.06.2013 13:42, schrieb Lubomir Popov:
Hello Lubomir,
Am 01.06.2013 18:44, schrieb Lubomir Popov:
New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older OMAPs and derivatives as well. The only anticipated exception would be the OMAP2420, which shall require driver modification.
[...]
drivers/i2c/omap24xx_i2c.c | 490 +++++++++++++++++++++++++++------------------ 1 file changed, 299 insertions(+), 191 deletions(-)
Tested on 3 arm335x based boards, which one uses i2c in SPL code for getting ram parameters, so:
Tested-by: Heiko Schocher hs@denx.de
Many thanks for testing, to Tom as well (he did it on the Beagleboards, but for one of the older versions, V3 I believe, right?). When it comes to versions, I see that V1 and V2 are still listed in patchwork, probably because of slightly different wording of the subject: http://patchwork.ozlabs.org/patch/233823/ http://patchwork.ozlabs.org/patch/246204/ Could you or Tom clean this up, please? Thanks.
Cleared the v3 v4 version, but missed the v2 and v1, Done.
just doing a "MAKEALL arm" with your patch applied and get this error:
Configuring for omap2420h4 board... omap24xx_i2c.c:497:17: error: 'struct i2c' has no member named 'irqstatus_raw' omap24xx_i2c.c:528:12: error: 'struct i2c' has no member named 'irqstatus_raw' arm-linux-gnueabi-size: './u-boot': No such file omap24xx_i2c.c: In function 'wait_for_bb': omap24xx_i2c.c:497:17: error: 'struct i2c' has no member named 'irqstatus_raw' omap24xx_i2c.c: In function 'wait_for_event': omap24xx_i2c.c:528:12: error: 'struct i2c' has no member named 'irqstatus_raw' make[1]: *** [omap24xx_i2c.o] Fehler 1 make: *** [drivers/i2c/libi2c.o] Fehler 2 make: *** Warte auf noch nicht beendete Prozesse...
Please fix this and run a "MAKEALL arm" before repost, thanks!
bye, Heiko

Hi Heiko,
On 04/06/13 07:26, Heiko Schocher wrote:
Hello Lubomir,
Am 03.06.2013 07:13, schrieb Heiko Schocher:
Hello Lubomir,
Am 02.06.2013 13:42, schrieb Lubomir Popov:
Hello Lubomir,
Am 01.06.2013 18:44, schrieb Lubomir Popov:
New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older OMAPs and derivatives as well. The only anticipated exception would be the OMAP2420, which shall require driver modification.
[...]
drivers/i2c/omap24xx_i2c.c | 490 +++++++++++++++++++++++++++------------------ 1 file changed, 299 insertions(+), 191 deletions(-)
Tested on 3 arm335x based boards, which one uses i2c in SPL code for getting ram parameters, so:
Tested-by: Heiko Schocher hs@denx.de
Many thanks for testing, to Tom as well (he did it on the Beagleboards, but for one of the older versions, V3 I believe, right?). When it comes to versions, I see that V1 and V2 are still listed in patchwork, probably because of slightly different wording of the subject: http://patchwork.ozlabs.org/patch/233823/ http://patchwork.ozlabs.org/patch/246204/ Could you or Tom clean this up, please? Thanks.
Cleared the v3 v4 version, but missed the v2 and v1, Done.
just doing a "MAKEALL arm" with your patch applied and get this error:
Configuring for omap2420h4 board... omap24xx_i2c.c:497:17: error: 'struct i2c' has no member named 'irqstatus_raw' omap24xx_i2c.c:528:12: error: 'struct i2c' has no member named 'irqstatus_raw' arm-linux-gnueabi-size: './u-boot': No such file omap24xx_i2c.c: In function 'wait_for_bb': omap24xx_i2c.c:497:17: error: 'struct i2c' has no member named 'irqstatus_raw' omap24xx_i2c.c: In function 'wait_for_event': omap24xx_i2c.c:528:12: error: 'struct i2c' has no member named 'irqstatus_raw' make[1]: *** [omap24xx_i2c.o] Fehler 1 make: *** [drivers/i2c/libi2c.o] Fehler 2 make: *** Warte auf noch nicht beendete Prozesse...
Please fix this and run a "MAKEALL arm" before repost, thanks!
We have agreed upon this breakage with Tom - the OMAP2420 shall not be supported anymore in the next release. At least I understood it that way (and have noted it accordingly in the comments).
I could of course make a very simple modification that would allow build without errors, but the driver shall not work on the 2420; in order to make it work, a more serious rework is required. I don't have however any 2420 hardware to test upon.
Tom?
bye, Heiko
Regards, Lubo

Hello Lubomir,
Am 04.06.2013 08:49, schrieb Lubomir Popov:
Hi Heiko,
On 04/06/13 07:26, Heiko Schocher wrote:
Hello Lubomir,
Am 03.06.2013 07:13, schrieb Heiko Schocher:
Hello Lubomir,
Am 02.06.2013 13:42, schrieb Lubomir Popov:
Hello Lubomir,
Am 01.06.2013 18:44, schrieb Lubomir Popov:
New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older OMAPs and derivatives as well. The only anticipated exception would be the OMAP2420, which shall require driver modification.
[...]
drivers/i2c/omap24xx_i2c.c | 490 +++++++++++++++++++++++++++------------------ 1 file changed, 299 insertions(+), 191 deletions(-)
Tested on 3 arm335x based boards, which one uses i2c in SPL code for getting ram parameters, so:
Tested-by: Heiko Schocher hs@denx.de
Many thanks for testing, to Tom as well (he did it on the Beagleboards, but for one of the older versions, V3 I believe, right?). When it comes to versions, I see that V1 and V2 are still listed in patchwork, probably because of slightly different wording of the subject: http://patchwork.ozlabs.org/patch/233823/ http://patchwork.ozlabs.org/patch/246204/ Could you or Tom clean this up, please? Thanks.
Cleared the v3 v4 version, but missed the v2 and v1, Done.
just doing a "MAKEALL arm" with your patch applied and get this error:
Configuring for omap2420h4 board... omap24xx_i2c.c:497:17: error: 'struct i2c' has no member named 'irqstatus_raw' omap24xx_i2c.c:528:12: error: 'struct i2c' has no member named 'irqstatus_raw' arm-linux-gnueabi-size: './u-boot': No such file omap24xx_i2c.c: In function 'wait_for_bb': omap24xx_i2c.c:497:17: error: 'struct i2c' has no member named 'irqstatus_raw' omap24xx_i2c.c: In function 'wait_for_event': omap24xx_i2c.c:528:12: error: 'struct i2c' has no member named 'irqstatus_raw' make[1]: *** [omap24xx_i2c.o] Fehler 1 make: *** [drivers/i2c/libi2c.o] Fehler 2 make: *** Warte auf noch nicht beendete Prozesse...
Please fix this and run a "MAKEALL arm" before repost, thanks!
We have agreed upon this breakage with Tom - the OMAP2420 shall not be supported anymore in the next release. At least I understood it that way (and have noted it accordingly in the comments).
Ah, ok ... so affected boards should fixed too ...
@Tom: Could you than pick up this patch?
I could of course make a very simple modification that would allow build without errors, but the driver shall not work on the 2420; in order to make it work, a more serious rework is required. I don't have however any 2420 hardware to test upon.
Tom?
bye, Heiko

On Tue, Jun 04, 2013 at 09:03:12AM +0200, Heiko Schocher wrote:
Hello Lubomir,
Am 04.06.2013 08:49, schrieb Lubomir Popov:
Hi Heiko,
On 04/06/13 07:26, Heiko Schocher wrote:
Hello Lubomir,
Am 03.06.2013 07:13, schrieb Heiko Schocher:
Hello Lubomir,
Am 02.06.2013 13:42, schrieb Lubomir Popov:
Hello Lubomir,
Am 01.06.2013 18:44, schrieb Lubomir Popov: > New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 > (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older > OMAPs and derivatives as well. The only anticipated exception would > be the OMAP2420, which shall require driver modification.
[...]
> drivers/i2c/omap24xx_i2c.c | 490 +++++++++++++++++++++++++++------------------ > 1 file changed, 299 insertions(+), 191 deletions(-)
Tested on 3 arm335x based boards, which one uses i2c in SPL code for getting ram parameters, so:
Tested-by: Heiko Schocher hs@denx.de
Many thanks for testing, to Tom as well (he did it on the Beagleboards, but for one of the older versions, V3 I believe, right?). When it comes to versions, I see that V1 and V2 are still listed in patchwork, probably because of slightly different wording of the subject: http://patchwork.ozlabs.org/patch/233823/ http://patchwork.ozlabs.org/patch/246204/ Could you or Tom clean this up, please? Thanks.
Cleared the v3 v4 version, but missed the v2 and v1, Done.
just doing a "MAKEALL arm" with your patch applied and get this error:
Configuring for omap2420h4 board... omap24xx_i2c.c:497:17: error: 'struct i2c' has no member named 'irqstatus_raw' omap24xx_i2c.c:528:12: error: 'struct i2c' has no member named 'irqstatus_raw' arm-linux-gnueabi-size: './u-boot': No such file omap24xx_i2c.c: In function 'wait_for_bb': omap24xx_i2c.c:497:17: error: 'struct i2c' has no member named 'irqstatus_raw' omap24xx_i2c.c: In function 'wait_for_event': omap24xx_i2c.c:528:12: error: 'struct i2c' has no member named 'irqstatus_raw' make[1]: *** [omap24xx_i2c.o] Fehler 1 make: *** [drivers/i2c/libi2c.o] Fehler 2 make: *** Warte auf noch nicht beendete Prozesse...
Please fix this and run a "MAKEALL arm" before repost, thanks!
We have agreed upon this breakage with Tom - the OMAP2420 shall not be supported anymore in the next release. At least I understood it that way (and have noted it accordingly in the comments).
Ah, ok ... so affected boards should fixed too ...
@Tom: Could you than pick up this patch?
Yes, I just need to post the patch removing omap2420h2 support.

On Sun, Jun 02, 2013 at 07:20:50AM +0200, Heiko Schocher wrote:
Hello Lubomir,
Am 01.06.2013 18:44, schrieb Lubomir Popov:
New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older OMAPs and derivatives as well. The only anticipated exception would be the OMAP2420, which shall require driver modification.
- Rewritten i2c_read to operate correctly with all types of chips (old function could not read consistent data from some I2C slaves).
- Optimised i2c_write.
- New i2c_probe, performs write access vs read. The old probe could hang the system under certain conditions (e.g. unconfigured pads).
- The read/write/probe functions try to identify unconfigured bus.
- Status functions now read irqstatus_raw as per TRM guidelines (except for OMAP243X and OMAP34XX).
- Driver now supports up to I2C5 (OMAP5).
Signed-off-by: Lubomir Popov lpopov@mm-sol.com
V5 changes:
- Replaced some printf() with puts().
- Minor formatting touches, checkpatch-clean.
V4 changes:
- New i2c_probe is built unconditionally, old code is removed. CONFIG_I2C_PROBE_WRITE is no longer needed.
- Added a small delay to work around breakage in AM335X SPL.
- Some whitespace and general formatting cleanup.
V3 changes:
- Removed old functions and conditional compilation. New functions are now built unconditionally for all SoCs using this driver. The only chip that should break is the OMAP2420 dinosaur.
- Interrupts are enabled for OMAP243X and OMAP34XX only (where we don't have an irqstatus_raw register).
V2 changes:
- Probe tries to identify misconfigured pads as well.
- Status is retrieved from irqstatus_raw rather than from stat.
- Some minor style & format changes.
drivers/i2c/omap24xx_i2c.c | 490 +++++++++++++++++++++++++++------------------ 1 file changed, 299 insertions(+), 191 deletions(-)
Tested on 3 arm335x based boards, which one uses i2c in SPL code for getting ram parameters, so:
Tested-by: Heiko Schocher hs@denx.de
Just one comment: Your patch has 9 checkpatch warnings which are all lines (printf strings) over 80 chars ... some with lines > 110 characters ... I know, tom gave you a OK for this ... I am also unhappy with splitting a printf-string over 2 or more lines ... but we have this 80 characters rule ... Wolfgang, what do you think? Should we loosen this rule for printf-strings?
We have loosened the rule for printf strings already, in order to make it easier for tracking down error messages. However, checkpatch needs tweaking at times for our print functions vs kernel print functions.

Hello Tom,
Am 02.06.2013 15:08, schrieb Tom Rini:
On Sun, Jun 02, 2013 at 07:20:50AM +0200, Heiko Schocher wrote:
Hello Lubomir,
Am 01.06.2013 18:44, schrieb Lubomir Popov:
New i2c_read, i2c_write and i2c_probe functions, tested on OMAP4 (4430/60/70), OMAP5 (5430) and AM335X (3359); should work on older OMAPs and derivatives as well. The only anticipated exception would be the OMAP2420, which shall require driver modification.
[...9
Just one comment: Your patch has 9 checkpatch warnings which are all lines (printf strings) over 80 chars ... some with lines > 110 characters ... I know, tom gave you a OK for this ... I am also unhappy with splitting a printf-string over 2 or more lines ... but we have this 80 characters rule ... Wolfgang, what do you think? Should we loosen this rule for printf-strings?
We have loosened the rule for printf strings already, in order to make it easier for tracking down error messages. However, checkpatch needs tweaking at times for our print functions vs kernel print functions.
Ok, great, missed that, thanks!
bye, Heiko
participants (3)
-
Heiko Schocher
-
Lubomir Popov
-
Tom Rini