[U-Boot] [PATCH 1/3] ARM: I2C: I2C Multi byte address support
Heiko Schocher
hs at denx.de
Fri Jan 13 13:06:11 CET 2012
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 at ti.com>
> Signed-off-by: Hebbar, Gururaja <gururaja.hebbar at ti.com>
> Signed-off-by: Patil, Rachna <rachna at 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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
More information about the U-Boot
mailing list