Shawn Anastasio has posted comments on this change. ( https://review.coreboot.org/23057 )
Change subject: buspirate_spi: Add support for variable serial speeds ......................................................................
Patch Set 4:
(4 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@452 PS4, Line 452: /* Ignore requests to set the serial speed to the default value */
What about keeping the default? e.g. […]
In the case of a v3 with serialspeed=115200, the intended behavior would occur, but the warning will still be printed.
I think I'll just remove the warning entirely.
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).
Will fix.
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 […]
I'll fix that.
You raise a fair point about code that may go unused, but in this case since the added code is a single print statement, I don't think the potential for project-damaging behavior is significant.
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. […]
Good catch, I'll fix that.
As for the visibility, do you mean I should change the log level (use msg_pinfo)? I chose msg_pdbg because a similar statement above which prints out the SPI speed also uses it.
-
Shawn Anastasio (Code Review)