From mboxrd@z Thu Jan  1 00:00:00 1970
From: Timur Tabi <timur@freescale.com>
Date: Wed, 31 Jan 2007 09:58:29 -0600
Subject: [U-Boot-Users] Fix empty i2c reads/writes in fsl_i2c.c
In-Reply-To: <1170237859.26527.6.camel@gentoo-jocke.transmode.se>
References: <1170237859.26527.6.camel@gentoo-jocke.transmode.se>
Message-ID: <45C0BCA5.5010009@freescale.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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