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 1:
(5 comments)
Carl-Daniel pointed out on IRC that there already was a patch floating around:
https://mail.coreboot.org/pipermail/flashrom/2012-November/010102.html
He was also discussing pre-v3 buspirates. I guess we should filter them out unless we can test the patch with one.
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c File buspirate_spi.c:
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@41 PS1, Line 41: const int divisor; Looks like the divisor is simply `4,000,000 / baud - 1`. Though, as I've just learned, most buspirates come with an integrated serial UART to USB chip. So that raises the question if older buspirates support the BAUD switch at all and if it makes sense to use any- thing but the 2M?
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@200 PS1, Line 200: ret = buspirate_sendrecv(bp_commbuf, 1, 0); If you are going to overwrite `ret` below, you should check it here too and bail out in case.
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@201 PS1, Line 201: if ((ret = serialport_config(sp_fd, 115200)))
nit: extra parens
IIRC, gcc wants those parens (because of the assignment).
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@202 PS1, Line 202: goto out_shutdown; Actually, no goto needed here.
I'm also wondering why we should do this at all. We are not going to transfer any more data and the baud is reset to 115200 whenever we open the UART.
https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@352 PS1, Line 352: msg_pdbg("Detected Bus Pirate hardware %s\n", bp_commbuf); Here the hardware version is stored in `bp_commbuf` can we use that to decide which baud rate to chose? I'd prefer that over another option for the user to guess.
-
Nico Huber (Code Review)