Attention is currently required from: Xiang Wang, Stefan Reinauer, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49255 )
Change subject: bitbang-spi.c: support clock polarity and phase ......................................................................
Patch Set 6:
(9 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/49255/comment/c31b8225_907bab5e PS1, Line 8:
https://en.wikipedia.org/wiki/Serial_Peripheral_Interface […]
Hi Xiang, So I think what Angel and I are saying here is that the rational should be explained in more detail in the commit message for such a substantial change in behavior. Please take your time here to detail more precisely what you have in mind to help bridge the gap in what you are trying to archive vs the communities understanding of the current code.
File bitbang_spi.c:
https://review.coreboot.org/c/flashrom/+/49255/comment/7203c5fd_ce5fc956 PS6, Line 52: } : : master->set_sck(sck); : master->set_mosi(mosi); : } spurious changes?
https://review.coreboot.org/c/flashrom/+/49255/comment/0ff3ed08_74429927 PS6, Line 62: ditto
https://review.coreboot.org/c/flashrom/+/49255/comment/cf93e2aa_90e23082 PS6, Line 89: for (int i = 0; i < 8; i++) { Leave style changes to another commit and out of the meat of this one, it gets in the way of reviewing the more important details.
https://review.coreboot.org/c/flashrom/+/49255/comment/07bf21e9_27cb0d96 PS6, Line 90: ret = ret << 1; same here, just leave as `ret <<= 1;` in this commit
https://review.coreboot.org/c/flashrom/+/49255/comment/dfcab703_529f5e47 PS6, Line 107: for (int i = 0; i < 8; i++) { ditto
https://review.coreboot.org/c/flashrom/+/49255/comment/8f86772f_27328871 PS6, Line 119: val = val << 1; ditto
https://review.coreboot.org/c/flashrom/+/49255/comment/0d844a84_f607a509 PS6, Line 136: programmer_delay(master->half_period); : bitbang_spi_set_cs(master, 0); : programmer_delay(master->half_period); : this feels like its own little patch.
https://review.coreboot.org/c/flashrom/+/49255/comment/cb9f26fe_88e2c7cb PS6, Line 186: char *mode = extract_programmer_param("mode"); : if (mode) { : if (strlen(mode)) { : char *endptr; : int v = strtol(mode, &endptr, 0); : if (mode == endptr || v > 3 || v < 0) { : msg_perr("bitbang spi only work with mode 0-3\n"); : free(mode); : return ERROR_FLASHROM_BUG; : } : cpol = (v >> 1) & 1; : cpha = (v >> 0) & 1; : } : free(mode); : } static parse_mode function that takes `(&int, &int)` as a type ?
-
Edward O'Callaghan (Code Review)