On Fri, Mar 18, 2016 at 09:46:30AM +0100, Jan Glauber wrote:

From: David Daney <redacted>

Use High Level Controller when possible.

Can you give me a one line description what this Controller is? I'd
assume it can do simple write-then-read messages with less setup?

i2c-octeon was reacting badly to bus contention: when in
direct-access mode (for transfers > 8 bytes, which cannot use the
high-level controller) some !ACK or arbitration-loss states were
not causing the current transfer to be aborted, and the bus released.

So, what does this patch do? Enable HLC for transfers < 8 byte? And for
all other transfers we still suffer from the same problem?

Such information should be here, too. It helps reviewing when I already
have the big picture.

There's one place in i2c protocol that !ACK is an acceptable
response: in the final byte of a read cycle.  In this case the
destination is not saying that the transfer failed, just that it
doesn't want more data.

Ehrm, no? For reads, the MASTER is saying it doesn't need any more data.
And an I2C eeprom can legally NACK a write, e.g. when it is still
processing the previous write. Also, NACK is a valid response after the
address phase, meaning there is no device listening.

Does the implementation cover the above cases?

This enables correct behavior of ACK on final byte of non-final read
msgs too.

The patch is huge and very hard to review. Maybe it needs to be split
up. Brainstorming example: a) move functions like octeon_i2c_set_clock()
upwards, b) change them if needed, c) implement HLC functions, d) add
switching logic to use HLC or non-HLC functions...

But first we need to be clear on the big picture view.

Thanks,

   Wolfram