Change in flashrom[master]: [UNTESTED] bitbang_spi.c: Support changing clock polarity and phase
Attention is currently required from: Xiang W, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49271 )
Change subject: [UNTESTED] bitbang_spi.c: Support changing clock polarity and phase ......................................................................
Patch Set 1:
(8 comments)
Patchset:
PS1:
This implementation is wrong
Not more wrong than yours, however. And it seems easier to fix.
PS1: I had a look at it. But before anyone continues, please ask yourself if this is useful or just a programming excercise. I don't mind the latter as long as everything is double reviewed and double tested.
File bitbang_spi.c:
https://review.coreboot.org/c/flashrom/+/49271/comment/e64ce9eb_775863bb PS1, Line 35: val ^ cpol This would have to be done in all _set_sck functions?
`val` is misleading; how about `idle` and then set `!idle ^ cpol`?
https://review.coreboot.org/c/flashrom/+/49271/comment/ed0d0fd2_06c3212d PS1, Line 71: cpha
Tested on my raspberry pi zero with W25Q128.V […]
This aligns with the SCK setting in bitbang_spi_write_byte(). Same reasoning applies here.
https://review.coreboot.org/c/flashrom/+/49271/comment/0ffa64ff_7533c3ef PS1, Line 73: 0 This would be `cpha` too. We want the same edge of the start of any bit.
https://review.coreboot.org/c/flashrom/+/49271/comment/1201e10b_a92f65a8 PS1, Line 91: bitbang_spi_set_mosi_set_sck(master, (val >> i) & 1, cpha); So, for CPHA=0 we want to keep SCK in its idle state...
0 ^ CPOL == CPOL => ACK
For CPHA=1 we want to toggle it out of its idle state
1 ^ CPOL = !CPOL => ACK
https://review.coreboot.org/c/flashrom/+/49271/comment/a1f781e6_2a0303c2 PS1, Line 93: bitbang_spi_set_sck(master, !cpha); Naturally, this works too then.
https://review.coreboot.org/c/flashrom/+/49271/comment/a22c1ce0_0928bc27 PS1, Line 120: bitbang_spi_set_sck(master, 0); This is only necessary for CPHA=0 and would align nicely with the delay below in a single if branch.
-
Nico Huber (Code Review)