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