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

[Alexey Brodkin]

On Tue, 2013-12-03 at 08:55 +0800, Kuo-Jung Su wrote:
> The comment bellow clearly explain the issue here.
> 
> soft_i2c.c: line 351 ~ 367:
> 
> #ifdef CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW
>     /*
>      * EEPROM chips that implement "address overflow" are ones
>      * like Catalyst 24WC04/08/16 which has 9/10/11 bits of
>      * address and the extra bits end up in the "chip address"
>      * bit slots. This makes a 24WC08 (1Kbyte) chip look like
>      * four 256 byte chips.
>      *
>      * Note that we consider the length of the address field to
>      * still be one byte because the extra address bits are
>      * hidden in the chip address.
>      */
>     chip |= ((addr >> (alen * 8)) & CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW);
> 
>     PRINTD("i2c_read: fix addr_overflow: chip %02X addr %02X\n",
>         chip, addr);
> #endif

Indeed comment is pretty clear.
But IMHO this code is very generic (how is it bound to any specific
device driver?) and because of this I believe it should be in common I2C
sources but not in device-specific ones.

Otherwise do we need to copy-paste this snippet to all I2C drivers?

I do like the code above for modification of slave address ("chip") -
for me it looks very clear and CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW makes
perfect sense.

So why don't we try to push this in generic "eeprom_{read|write}"?

> Yes,the address/offset tricks are device specific (not only EEPROM, it
> also applies to Audio Codecs..etc.)

Saying "device specific" I meant not "I2C driver specific" but "attached
I2C slave specific".

As you correctly stated this kind of tricky addressing is used by
EEPROMs, audio codecs etc.

So when we need to work with EEPPROM with "ADDR_OVERFLOW" I expect to
enable CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW in configuration.

But imagine if the same I2C bus has another slave which expects "normal"
I2C addressing. So how then you're going to configure your I2C driver so
it correctly works with both slaves?

My vision of resolution is like this: I2C driver always work in "normal
addressing" mode - just uses "chip" and "address" values as they are,
but in higher level code like in ours "cmd_eeprom" we may modify both
"chip" and "offset" values for each particular type of attached I2C
device.

> But this code was there over a decade. And if everything works just
> fine, why bother ?

Well as it turned out not everything worked that fine. As I discovered
"dw_i2c" didn't work because of missing address re-calculation.

Indeed I may agree with your previous patch:
=====
if (i2c_write(dev_addr, offset, alen - 1, buffer, len))
=====
and implement address modification in "dw_i2c" driver.

But still there're questions:
1. Which other drivers will require update? and who's going to check/fix
it there?
2. Why do we need all this address modification part in "cmd_eeprom.c"?
And if we don't need - who's going to clean this up?

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?

Regards,
Alexey