2013/12/10 Alexey Brodkin Alexey.Brodkin@synopsys.com:
On Mon, 2013-12-09 at 12:21 +0100, Heiko Schocher wrote:
I thought the v3 patch just rolls things back as patch comment states:
Changes for v3:
- It turns out that what we did before 2013-11-13 (i.e., cmd_eeprom: fix i2c_{read|write} usage if env is in I2C EEPROM) is still the best one, this patch simply rollback to it with coding style fix.
Without this roll back, I think, some boards are broken now in current mainline... so first we should go this step back.
I'd say it's very questionable. For example if you look at I2C driver that Kuo-Jung refers to ("soft_i2c") you'll find that with my patch applied (the one you've just rolled back) it will work flawlessly because:
- "chip" address there has double applied MSB bits of offset (the first
time it gets applied in "cmd_eeprom") 2. Only LSB byte of offset gets written in I2C device if addr_len = 1.
But if addr_len = 2, everything goes wrong. This is the real problem here...
This one makes IMHO much more sense because it excludes an extra "chip" address modification - so it's clear that it will be done by particular I2C driver so people won't be confused with their expectations.
BTW what I cannot understand is why "soft_i2c_read" has this "chip" modification part while "soft_i2c_write" doesn't? Is it done on purpose? And how it actually works at all then?
Good question ... maybe currently only used on i2c reads ?
Frankly I have only 1 supposition regarding this strange state of things in "soft_i2c_write". Because without any doubts the same modification of "chip" address is applicable to both "read" and "write" because it is an integral part of data addressing.
But due to discussed a lot in this thread "double chip address modification" (application of MSB bits of offset) proper support of "chip" address was never implemented in "soft_i2c_write". As you correctly mentioned - "real ancient code" works and nobody has problems with it. Well, just because we have current implementation of "eeprom_write" that does "chip" address modification "soft_i2c" may not have this feature in both "read" and "write".
So I'd prefer to go with previous version of patch sent by Kuo-Jung http://patchwork.ozlabs.org/patch/294733/
And indeed this will "break" functionality of currently incomplete implementation of "soft_i2c_write".
No, it will works just fine.
1. eeprom_read: The overflowed address was shifted to chip address by cmd_eeprom.c, and then the same operations would be repeated by soft_i2c.c, but it's no harm to do a OR operation twice on the same bits, so everything is all right.
2. eeprom_write: The overflowed address was shifted to chip address by cmd_eeprom.c, and get passed to soft_i2c.c without further modification. So everything is all right.
The 'CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW' in soft_i2c.c is actually a redundant code snippet. And it would be better to move/copied its comment to cmd_eeprom.c.
Which means if we don't rollback, none of the EEPROMs with addr_len > 1 will work on u-boot. And this rollback actually does no harm to the EEPROMs with addr_len = 1 or 2.
P.S: The designware_i2c.c should work just fine with this rollback, too....