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

[Heiko Schocher <hs@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 <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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany