Angel Pons 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 ... (see next comment)
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 ...
https://review.coreboot.org/c/flashrom/+/38833/1/sb600spi.c@391 PS1, Line 391: Setting SPI clock to %s ... this as well.
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.
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.
Instead, one can keep the declarations at the beginning and only extract spireadmode after this block.
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?
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:
if (spispeed_idx < 0) return 1;
return set_speed(dev, spispeed_idx);