2013/11/29 Alexey Brodkin Alexey.Brodkin@synopsys.com:
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.
Yes, that's why I said the root cause is U-Boot's I2C model. The address should never be rebuilt/reformated inside the I2C driver. The better solution is to update the i2c_read/i2c_write to:
int i2c_read(struct i2c_adapter *adap, u8 dev, uint *addr, int alen, uchar *buf, int len)
or
int i2c_read(struct i2c_adapter *adap, u8 dev, uchar *buf, int len)
i.e., drop the use of uint 'addr'
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).
Yes, it looks like a special case which use BIT0 of device address for page selection. Which means we can't directly pass the device address to i2c_read/i2c_write. But it's still o.k to directly the 'offset' as what we did before.
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 */ ==================
From source code above you see that virtually it addresses multiple 256 byte EEPROMs.
Regards, Alexey