[U-Boot] [PATCH v2 4/4] cmd_eeprom: bug fix for i2c read/write

[Alexey Brodkin]

Hi Kuo-Jung,

On Fri, 2013-11-29 at 08:59 +0800, Kuo-Jung Su wrote:
> No it's a common misunderstanding, I also made the same mistake before.
> Please check my bug fix for Faraday FTI2C010 I2C driver:
> 
> http://patchwork.ozlabs.org/patch/294732/
> 
> The address should be rebuild inside the i2c driver in u-boot's I2C model.
> 
> Another example could be found at drivers/i2c/soft_i2c.c, line 382 - 389
> 
> static int soft_i2c_read (...)
> {
>     ......
>     shift = (alen-1) * 8;
>     while(alen-- > 0) {
>         if(write_byte(addr >> shift)) {
>             PRINTD("i2c_read, address not <ACK>ed\n");
>             return(1);
>         }
>         shift -= 8;
>     }
>     ......
> }
> 
> However the root cause is the u-boot i2c driver model, the address should
> never be passed to i2c_read/i2c_write in uint format.
> 
> Please take a peak at how ecos defineds the relative i2c routines:
> 
> externC cyg_uint32  cyg_i2c_transaction_tx(const cyg_i2c_device*,
> cyg_bool, const cyg_uint8*, cyg_uint32, cyg_bool);
> externC cyg_uint32  cyg_i2c_transaction_rx(const cyg_i2c_device*,
> cyg_bool, cyg_uint8*, cyg_uint32, cyg_bool, cyg_bool);
> 
> In this way, the eeprom address would be forced to rebuild in address
> pointer (i.e., addr[]) as what we did in SPI mode.

Unfortunately I still cannot agree with you.
In my opinion I2C driver has nothing to do with current situation.

It's purely how manufacturers of EEPROM use I2C.
For example I we use "PCF8594C-2" EEPROM.
Here's a datasheet -
http://www.nxp.com/documents/data_sheet/PCF8594C_2.pdf

Its capacity is 512 bytes. And as you may see from "Fig 3. Device
addressing." Each IC houses 2 virtual EEPROMs (each housing 256 bytes of
data).

And if you accept this philosophy you'll understand why "I2C chip
address" is modified and why following approach is used in
"cmd_eeprom.c" for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN == 1" (which says
that EEPROM has address length of 1 byte):
==================
addr[0] = offset >> 8;	/* block number */
addr[1] = blk_off;	/* block offset */
addr[0] |= dev_addr;	/* insert device address */
==================