Patch Set 3:

Patch Set 3: Code-Review-1

(2 comments)

Patch Set 3: Code-Review-1

If you change the code flow, please test (preferably >1000 times).
Unless there is documentation that states that after reading `CONF`
it will keep that value, we don't know that for sure. I know that
the code really makes it look like this is the way the hardware
works, but IMHO, it happens too often that some tool tells us to
fix something and we break it instead.

Definitely agree. I'd update the commit message as well.

The difference may be very subtle, but if the cursed sand (hardware) actually care about that register being read twice, things can break. So, even if it seems to be very minor, let's keep the original behavior.

So I did a bit of digging and found the technical reference manual for this chip [0]. There is a state machine on page 281 that describes the control flow for these transitions, and if we are trying to move to the ACCESS state (which is the name of this function) then the fall through from INIT_MEM -> CONF looks intentional, since that is the only way to get to ACCESS. There is another function move_to_config_state() in this file that has a similar (but explicit) fall through, so judging from that I believe this fall through is intentional as well.

Ah, documentation, nice :) Though, it's documenting the hardware state
machine. I think, it actually shows that it doesn't matter if we break.
The `break` would make us read the hardware state again. The diagram
shows us that it would only leave the `Config` state on an explicit `Go`
from us. So re-reading the hardware state should provide the same value,
and we'd give the `Go`.

[0]: http://rockchip.fr/Rockchip%20RK3288%20TRM%20V1.0%20Part%201-System%20and%20System%20Control.pdf

Patch set 4:Code-Review +2

View Change

To view, visit change 34296. To unsubscribe, or for help writing mail filters, visit settings.