From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Fri, 13 Jan 2012 13:06:11 +0100 Subject: [U-Boot] [PATCH 1/3] ARM: I2C: I2C Multi byte address support In-Reply-To: <1326448661-26147-1-git-send-email-rachna@ti.com> References: <1326448661-26147-1-git-send-email-rachna@ti.com> Message-ID: <4F101E33.2080900@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > Signed-off-by: Hebbar, Gururaja > Signed-off-by: Patil, Rachna > --- > 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 -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany