Re: [coreboot] New interface for I2C in coreboot

[Julius Werner]

I think the idea behind this change was mostly that the old API was
too inflexible and some I2C device drivers could not be properly
written with it. Take a look at line 139 in this file from the first
patch: http://review.coreboot.org/#/c/7751/3/src/drivers/i2c/tpm/tpm.c@139
. What this really does is trying to do all of the normal parts of an
I2C read transaction manually, because TPMs can add so long delays at
any point that the normal I2C controller drivers would already hit
their timeout. With the old API this was only possible with a crude
hack of setting the address length to 0 (so i2c_read() would skip the
register address write completely). Doing a "raw read" with the new
API does the same thing much clearer, and it's far more obvious to
controller driver authors that they need to implement this (otherwise,
it's easy to just assume alen == 0 is illegal until someone tries to
use the TPM driver with it, which I think is what hit us on Tegra).
The new API also makes the rules about when to use a repeated start vs
a complete stop and start much clearer (which should not make a
difference for the device, but in practice sometimes does).

I think Werner's original issue is easy to solve: you can either just
construct struct i2c_seg structures and call i2c_transfer() directly,
or implement a new i2c_write() wrapper similar to i2c_writeb() (just
with a variable data length). Maybe even just make i2c_writeb() a
one-line wrapper macro on top of the new function... all fine by me, I
think the only reason we didn't make more convenience functions is
because we didn't need them at the time.

The implementation problems Patrick mentioned are unfortunately true
for some controllers which attempt to be more clever than they
should... however, the thing is that we need to somehow implement raw
I2C reads anyway (to support the alen == 0 case in the old API and
make things like that TPM driver work). In the end we probably need to
implement less primitives for the new API ("raw write" and "raw read")
than for the old one ("full write", "full read", "raw write" and "raw
read"). If that means we end up not using the
do-everything-in-one-transaction "feature" of some controllers in
favor of more controllable low-level accesses, I don't see a problem
with that.