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

[Heiko Schocher]

Hello Alexey,

Am 09.12.2013 11:35, schrieb Alexey Brodkin:
> Hi Heiko,
>
> On Mon, 2013-12-09 at 07:56 +0100, Heiko Schocher wrote:
>
>> Applied to u-boot.i2c.git, thanks!
>>
>
> I'm wondering if you've seen a discussion between me and Kuo-jung
> regarding this patch and consequences of it being applied.
>
> Do you mind to comment on questions we discussed there?

Sorry, seems I missed this ...

> My main concern is how to keep underlying I2C drivers working because as
> I wrote some of them will not function any more correctly.

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.

Next step, drivers who do not work, should be fixed if needed, or:

You wrote:
on Alexey Brodkin - Dec. 3, 2013, 7:42 a.m. wrote:
 > On Tue, 2013-12-03 at 08:55 +0800, Kuo-Jung Su wrote:
 >>  The comment bellow clearly explain the issue here.
 > [...]
 >>  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.

Hmm.. this is real ancient code ... seems, until now, nobody had problems
with it ... if you see problems now in dw_i2c driver, you have two
possibilites:

- fix the dw_i2c driver

- The best way would really be to rework this, as you stated:
   "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."

   This touches I think a lot of boards ... the problem would be, how to
   test this change...

Nevertheless, this should be done in a new thread ...

 > But still there're questions:
 > 1. Which other drivers will require update? and who's going to check/fix
 > it there?

Yes thats the problem. For the "old state" this should be done, if new
boards need it and fix the i2c driver.

If you want to clean up this, as you mentioned above, this touches maybe
a lot of current i2c drivers (drivers who do this address modification)

So, this must be done carefully, and you/we need some board maintainers
which can test it on their boards.

 > 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?

The first who have issues with it? Or you volunteer to send a cleanup
patch?

 > 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 ?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany