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

[Kuo-Jung Su]

2013/11/28 Alexey Brodkin <Alexey.Brodkin at synopsys.com>:
> On Thu, 2013-11-28 at 10:47 +0800, Kuo-Jung Su wrote:
>> From: Kuo-Jung Su <dantesu at faraday-tech.com>
>>
>> The local pointer of address (i.e., addr) only gets
>> referenced in SPI mode, and it won't be appropriate
>> to pass only 1 bytes addr[1] to i2c_read/i2c_write while
>> CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1.
>>
>> To avoid ambiguity, this patch would drop the use of
>> address pointer in I2C mode, and directly pass (dev_addr, offset)
>> to i2c_read/i2c_write.
>
> Unfortunately this patch breaks cases with
> "CONFIG_SYS_I2C_EEPROM_ADDR_LEN = 1" where address is limited to 1 byte
> - thus a need to pass "addr[0]" which is combined from real I2C device
> address and 256-byte word offset (i.e. MSB part of offset). And
> "addr[1]" only carries lower 8 bit of offset.
>
> So I would recommend to separate 2 invocations of "i2c_{read|write}":
> 1) for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN = 1"
> 2) for "CONFIG_SYS_I2C_EEPROM_ADDR_LEN > 1"
>
> -Alexey
>

Hi Alexey:

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.

Best Wishes
Kuo-Jung Su