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

[Patil, Rachna]

Hi Heiko 

On Fri, Jan 13, 2012 at 17:36:11, Heiko Schocher wrote:
> 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.

I will clean this up.

> 
> > 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.

Yes, I am using this to define a bitmask, but this applies to changes done in this patch.

> 
> > +#define I2C_STAT_TIMEO	10
> 
> Could you explain, for what purpose this new timeout is?

I have interchanged the meaning of the above two definitions.
I2C_STAT_TIMEO would reserve the purpose of I2C_TIMEOUT.
To avoid confusion I will retain the original definition of I2C_TIMEOUT
And add the bit definition to I2C_STAT_TIMEO, so that would be

#define I2C_TIMEOUT	10
#define I2C_STAT_TIMEO  (1 << 31)

> 
> > -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?

I kept this delay big keeping in mind that some devices may need more time to respond.
However I have tested by reducing the delay and is still works.
I will reduce this to 9000.

> 
> > +
> > +	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?

These values are based on trial and error methods and not from any datasheet.
This delay can be removed.

> 
> [...]
> >  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 ?

Else, it would return 1, which is already taken care of. 
This if loop is not required. I will be deleting this.

> 
> > +		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.

Yes, I will run a checkpatch and take care in v2.

> 
> [...]
> > -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?

Agree, this does not have anything to do with two byte addressing support.
I will retain the original value.

> 
> [...]
> 
> bye,
> Heiko
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> 

Regards,
Rachna.