Nico Huber has posted comments on this change. ( https://review.coreboot.org/23057 )
Change subject: buspirate_spi: Add support for variable serial speeds ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/#/c/23057/4/buspirate_spi.c File buspirate_spi.c:
https://review.coreboot.org/#/c/23057/4/buspirate_spi.c@41 PS4, Line 41: }; Nit, this is now the same struct as buspirate_spispeeds. You could unite them, e.g. as `struct buspirate_speed`.
https://review.coreboot.org/#/c/23057/4/buspirate_spi.c@452 PS4, Line 452: /* Ignore requests to set the serial speed to the default value */ What about keeping the default? e.g. using 115200 with v3+?
https://review.coreboot.org/#/c/23057/4/buspirate_spi.c@453 PS4, Line 453: msg_pdbg("Ignoring request to set serial speed to default value of %d\n", BP_DEFAULTBAUD); Line is too long (even for flashrom's 112 char limit).
https://review.coreboot.org/#/c/23057/4/buspirate_spi.c@456 PS4, Line 456: msg_pwarn("Bus Pirate hardware version older than 3.0 may not support increased serial speeds." Line is too long, and it looks like the output wouldn't fit a 80x25 terminal line either and missing space after the full-stop.
Btw. this is exactly why I don't like to add code that we don't know if anybody will ever run it: People don't test the code paths, nobody cares if they are correct... it usually means that a project drags around dead, sometimes broken code.
https://review.coreboot.org/#/c/23057/4/buspirate_spi.c@497 PS4, Line 497: msg_pdbg("Serial speed is %d baud\n", serialspeeds[serialspeed_index].speed); Move into the if body above, serialspeed_index may be off. I would also make the message visible by default.
-
Nico Huber (Code Review)