Nico Huber has posted comments on this change. ( https://review.coreboot.org/26946 )
Change subject: bitbang_spi: Add functions to optimize xfers ......................................................................
Patch Set 2: Code-Review+2
(3 comments)
You are right, you didn't change the clocking. It just looks confusing now. Btw. changing from CPOL=0 CPHA=0 to CPOL=1 CPHA=1 is no big deal, the only difference is the idle state of the clock.
https://review.coreboot.org/#/c/26946/2/bitbang_spi.c File bitbang_spi.c:
https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@47 PS2, Line 47: static void bitbang_spi_set_sck_set_mosi(const struct bitbang_spi_master * const master, int sck, int mosi) Now that I looked again, the `sck` parameter is not needed, its always called with the same value. Could be renamed to `bitbang_ spi_clear_sck_set_mosi()` if you want to get rid of it.
https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@114 PS2, Line 114: bitbang_spi_set_sck_set_mosi(master, 0, 0);
I don't really understand what you mean here. […]
You are right. All I saw was that we start at 0 and the first thing we do in the loop is set to 0 again, that seemed wrong. And I see now why, the code was wrong all the time. Your change just makes it more obvious.
https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@157 PS2, Line 157: bitbang_spi_set_sck(master, 0);
You mean change from a CPOL=0, CPHA=0 controller into a CPOL=1, CPHA=1 controller (using terminology […]
I didn't mean to change it to further optimize but to correct it.
I hope you didn't stare at the diagram on Wikipedia as long as I did, it is wrong as well (or at least doesn't reflect the descrip- tion).
Let's see what CPOL=0 CPHA=0 really means. Leading edge is 0 to 1, trailing edge is 1 to 0. Now it says "For CPHA=0, the "out" side changes the data on the trailing edge of the preceding clock cycle". But with this code, there is no preceding clock cycle. I'm not sure if that is valid. But it doesn't matter as all slave implementations just sample when they see the rising edge.
-
Nico Huber (Code Review)