Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38833 )
Change subject: sb600spi: Add spireadmode ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@376 PS1, Line 376: \n
Maybe replace this newline with `...` and a space, and then ... […]
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@388 PS1, Line 388: Setting SPI clock to %s
... this could be shortened to "failed.\n", and ...
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@391 PS1, Line 391: Setting SPI clock to %s
... this as well.
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@397 PS1, Line 397: msg_pdbg("Setting SPI read mode to %s (%i).\n", spireadmodes[mode], mode);
Same as before.
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@435 PS1, Line 435: free(spispeed); : return 1;
nit: This path does not free(spireadmode), but it might be null. […]
Ack
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@451 PS1, Line 451: (amd_gen < CHIPSET_BOLTON))
Does this mean that generations before Bolton do not support configuring spireadmode at all?
I'm not sure. The code already here only set spi mode for >= Bolton which is why I restricted this option to >= Bolton.
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@509 PS1, Line 509: if (spispeed_idx >= 0 && set_speed(dev, spispeed_idx) != 0) { : return 1; : } : return 0;
This is a bit hard to parse. How about: […]
Agree. Except it would return 0 (success) if spispeed_idx < 0.