Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/26946 )
Change subject: bitbang_spi: Add functions to optimize xfers ......................................................................
Patch Set 2:
(2 comments)
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 […]
How strongly do you feel about this? ;-)
I'm about to push a new patchset and I haven't acted on this one. Partly because we get in a mess with verbs[1] and partly because I prefer the code with the explicit set here if only because the mapping onto the unoptimized primitives is super obvious.
So I will change it if you really want it changed but if it is marginal for you then I'll leave as it is!
[1] Your comment applies equally to what is currently called bitbang_spi_set_sck_get_mosi() so "clear" won't work because it partners with "set" which now has two meanings... maybe "raise" and "lower" would work.
https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@157 PS2, Line 157: bitbang_spi_set_sck(master, 0);
"For the first cycle, ..." that is actually redundant with the […]
Reading this back after a break I'm not quite sure whether "nothing to argue about" means I should leave the code as is or if you still want to explore CPOL=1, CPHA=1 further.
Either way to save you having to check the code then the patchset I'm about to push is still CPOL=0, CPHA=0.
-
Daniel Thompson (Code Review)