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 6: Code-Review+1
(4 comments)
Tried to find some documentation on the matter which raised more questions. Code looks fine, so far.
https://review.coreboot.org/#/c/23057/4/buspirate_spi.c File buspirate_spi.c:
https://review.coreboot.org/#/c/23057/4/buspirate_spi.c@497 PS4, Line 497: bp_commbuf[0] = 0x00;
Good catch, I'll fix that. […]
Yes, I meant msg_pinfo() but was in doubt anyway. msg_pdbg() is just fine.
https://review.coreboot.org/#/c/23057/6/buspirate_spi.c File buspirate_spi.c:
https://review.coreboot.org/#/c/23057/6/buspirate_spi.c@443 PS6, Line 443: 5.5 Where is this written? This page talks about 5.2:
http://dangerousprototypes.com/docs/Bus_Pirate_menu_options_guide#B_Set_PC_s...
https://review.coreboot.org/#/c/23057/6/buspirate_spi.c@451 PS6, Line 451: " Continue at your own risk.\n"); I did some research, v2 can work with the same firmware and uses the same microcontroller and USB converter as v3. So that should be fine. Maybe remove the warning for v2 but keep it at default 115200 because we can't test it?
https://review.coreboot.org/#/c/23057/6/buspirate_spi.c@473 PS6, Line 473: sleep(1); Is this specified somewhere? It seems incredibly long.
-
Nico Huber (Code Review)