[U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c

[U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c

[Joakim Tjernlund]

This is against the freescale tree at opensource.freescale.com
What is the status of this three? is it going into u-boot any time soon?

 Jocke

[U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c

[Timur Tabi]

Joakim Tjernlund wrote:
> This is against the freescale tree at opensource.freescale.com
> What is the status of this three? is it going into u-boot any time soon?

Yes.

> @@ -191,15 +192,17 @@ __i2c_read(u8 *data, int length)
>  int
>  i2c_read(u8 dev, uint addr, int alen, u8 *data, int length)
>  {
> -	int i = 0;
> +	int i = -1; /* signal error */
>  	u8 *a = (u8*)&addr;
>  
>  	if (i2c_wait4bus() >= 0
>  	    && i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
> -	    && __i2c_write(&a[4 - alen], alen) == alen
> -	    && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0) {
> +	    && __i2c_write(&a[4 - alen], alen) == alen)
> +		i = 0; /* No error so far */
> +
> +	if (length
> +	    && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0)
>  		i = __i2c_read(data, length);
> -	}

This is a good idea, but I see a bug in the code.  Say the first block succeeds, 
and sets i to 0.  Then the "i2c_write_addr(dev, I2C_READ_BIT, 1)" fails.  It 
will never call "__i2c_read(data, length)", and so it will never set the value 
of i.  Then it will return, indicating success.

Frankly, I think the original code is just an abuse of short-circuit boolean 
operations and should be completely rewritten.  Something like this:

if (i2c_wait4bus() < 0)
	return -1;

if (i2c_write_addr(dev, I2C_WRITE_BIT, 0) == 0))
	return -1;

if (__i2c_write(&a[4 - alen], alen) != alen))
	return -1;

if (length) {
	if (i2c_write_addr(dev, I2C_READ_BIT, 1) == 0))
		return -1;

	if (__i2c_read(data, length) != length)
		return -1;
}

writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);

return 0;

This version is a lot easier to read and understand, I think.

> +	/* For unknow reason the controller will ACK when

Typo.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c

[Joakim Tjernlund]

> -----Original Message-----
> From: Timur Tabi [mailto:timur at freescale.com] 
> Sent: den 31 januari 2007 16:58
> To: joakim.tjernlund at transmode.se
> Cc: Kim Phillips; U-BOOT
> Subject: Re: [U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c
> 
> Joakim Tjernlund wrote:
> > This is against the freescale tree at opensource.freescale.com
> > What is the status of this three? is it going into u-boot 
> any time soon?
> 
> Yes.
> 
> > @@ -191,15 +192,17 @@ __i2c_read(u8 *data, int length)
> >  int
> >  i2c_read(u8 dev, uint addr, int alen, u8 *data, int length)
> >  {
> > -	int i = 0;
> > +	int i = -1; /* signal error */
> >  	u8 *a = (u8*)&addr;
> >  
> >  	if (i2c_wait4bus() >= 0
> >  	    && i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
> > -	    && __i2c_write(&a[4 - alen], alen) == alen
> > -	    && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0) {
> > +	    && __i2c_write(&a[4 - alen], alen) == alen)
> > +		i = 0; /* No error so far */
> > +
> > +	if (length
> > +	    && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0)
> >  		i = __i2c_read(data, length);
> > -	}
> 
> This is a good idea, but I see a bug in the code.  Say the 
> first block succeeds, 
> and sets i to 0.  Then the "i2c_write_addr(dev, I2C_READ_BIT, 
> 1)" fails.  It 
> will never call "__i2c_read(data, length)", and so it will 
> never set the value 
> of i.  Then it will return, indicating success.

No, i will be 0 and length will be !=0 so no success.

> 
> Frankly, I think the original code is just an abuse of 
> short-circuit boolean 
> operations and should be completely rewritten.  Something like this:
> 
> if (i2c_wait4bus() < 0)
> 	return -1;
> 
> if (i2c_write_addr(dev, I2C_WRITE_BIT, 0) == 0))
> 	return -1;
> 
> if (__i2c_write(&a[4 - alen], alen) != alen))
> 	return -1;
> 
> if (length) {
> 	if (i2c_write_addr(dev, I2C_READ_BIT, 1) == 0))
> 		return -1;
> 
> 	if (__i2c_read(data, length) != length)
> 		return -1;
> }
> 
> writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
> 
> return 0;
> 
> This version is a lot easier to read and understand, I think.

Yes, I agree. The orginal freescale code looked something like that,
but I am not going to rewrite it back to that form. Someone did
change it to this form.

> 
> > +	/* For unknow reason the controller will ACK when
> 
> Typo.

Yeah, mind fixing that if the other parts are OK?

 Jocke

[U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c

[Timur Tabi]

Joakim Tjernlund wrote:

>> will never call "__i2c_read(data, length)", and so it will 
>> never set the value 
>> of i.  Then it will return, indicating success.
> 
> No, i will be 0 and length will be !=0 so no success.

Oh yeah, right.

> Yes, I agree. The orginal freescale code looked something like that,
> but I am not going to rewrite it back to that form. Someone did
> change it to this form.

Really?  That's weird.  Oh well.  In that case, your patch is okay.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale