Attention is currently required from: Alan Green, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić. Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 )
Change subject: ft2232_spi.c: Implement spi_send_multicommand() ......................................................................
Patch Set 29:
(7 comments)
Patchset:
PS29: PS 29 has not been HW tested yet.
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/comment/98247231_518a3413 PS25, Line 189: ft2232_spi_send_command
As mentioned earlier, this is already implemented: default_spi_send_command(). […]
Ack
https://review.coreboot.org/c/flashrom/+/40477/comment/1fefe787_68aaa8f4 PS25, Line 295: 9
oh yes... I made a mistake, I didn't notice continue statement. […]
Good idea Nico! It makes the code more readable! Thanks!
https://review.coreboot.org/c/flashrom/+/40477/comment/1676189d_2de0812d PS25, Line 317: if (!cmds->readcnt && ((cmds + 1)->writecnt || (cmds + 1)->readcnt)){
I think we should check here also if the next command still fits. Might […]
Checking if next command fits is a very good idea! I did as you proposed. However I´m not sure if the '12' is too far from it´s origin if it´s in an extra function. But this is more a question of taste I think.
More important: I think we need to reset 'i' after send_buf in case the next command does not fit into buffer?
https://review.coreboot.org/c/flashrom/+/40477/comment/4fa462c3_1727eafa PS25, Line 343: msg_perr("send_buf failed: %i\n", ret);
If `ret` is non-0, we'd skip the next block, and the loop condition […]
Done.
Very good overview Nico!
https://review.coreboot.org/c/flashrom/+/40477/comment/c7fd8af8_93455fd6 PS25, Line 348: * FIXME: This is unreliable. There's no guarantee that : * we read the response directly after sending the read : * command. We may be scheduled out etc.
Looks like you lost a space before each asterisk. […]
Done
https://review.coreboot.org/c/flashrom/+/40477/comment/39a45309_9e9d0786 PS25, Line 356: msg_perr("get_buf failed: %i\n", ret);
Sorry, forgot this one, we'd have to `break;` here too on error. Also […]
Done