Attention is currently required from: Xiang W, Edward O'Callaghan, Angel Pons. Nico Huber 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 26:
(8 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/49255/comment/fba2547c_d605eaa9 PS1, Line 8:
Sorry I misunderstood.
The change does two things: 1. implements a better mode 0, 2. adds the option of other modes. The former is most welcome. I also like to get the whole patch merged, under one condition: The commit message has to give a reasonable incentive to add the optional modes.
For instance, what chip needs it to be supported? and what other changes are in the queue to make flashrom support it? Currently all the common code assumes that the programmer either implements mode 0 or 3 and that all flash chips support both. If we need to support other chips, the common code would have to change too, right?
Commit Message:
https://review.coreboot.org/c/flashrom/+/49255/comment/af59a92c_a15486b2 PS26, Line 11: may not be This "may not be" is rather vague. Do we know a real-world use case?
Patchset:
PS26: Sorry, I'm very late to this review. There is still the early question about the reason for the change. Basically, do we have a real-world use case?
File bitbang_spi.c:
https://review.coreboot.org/c/flashrom/+/49255/comment/147d1383_28ee3080 PS26, Line 171: _set_mosi Removing the spurious MOSI settings seems fine, but it should be mentioned in the commit message (or happen in a preceding commit).
https://review.coreboot.org/c/flashrom/+/49255/comment/cad0abf5_d7823283 PS26, Line 34: data transmission Technically, it's the data sampling (or capturing, as Wikipedia puts it) that happens, not the transmission.
https://review.coreboot.org/c/flashrom/+/49255/comment/b4c52ed0_ba3e8580 PS26, Line 96: bitbang_spi_set_sck(master, !cpol); The first clock change always needs to happen with a delay after asserting CS#. Both cpha cases should start with a delay.
https://review.coreboot.org/c/flashrom/+/49255/comment/c2ff835a_2f5e6f13 PS26, Line 184: if (strlen(mode)) { So an empty string is not reported as error?
https://review.coreboot.org/c/flashrom/+/49255/comment/e5719916_50b36152 PS26, Line 187: mode == endptr This does not account for trailing garbage. It's better to check for the null termination, e.g.
if (*mode == '\0' || *endptr != '\0' || v > 3 || v < 0) {
(this includes checking for an empty string)