Attention is currently required from: Simon Buhrow, Alan Green, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 )
Change subject: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call ......................................................................
Patch Set 25:
(13 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/40477/comment/f392e74b_1cb2807d PS25, Line 7: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call This would be more accurate now:
ft2232_spi: Implement spi_send_multicommand()
https://review.coreboot.org/c/flashrom/+/40477/comment/b17dbb66_4a474a3c PS25, Line 10: seems to take always 2-3ms to respond. Please no line breaks after a sentence. Either it's one paragraph (then no line break) or it's two (then they should be separated by an empty line).
https://review.coreboot.org/c/flashrom/+/40477/comment/4896555b_ecc3d883 PS25, Line 20: I am using ftdi-2232H chip. That is why I put it at this place. : If anyone has a good overview about all programmers: : This could be implemented in spi_write_cmd() in case that it is : compatible to all programmers : or this principle could be transfered to other programmers which act : in a similar way. It's already implemented, spi_send_multicommand() is called where possible, so compatible programmers just need to implement it.
Patchset:
PS25: Hi Simon, thanks for the work and sorry for not responding for that long. Also sorry that people keep merging conflicting changes. I guess they just didn't know your work. You'll need at least one more rebase with fixups.
The implementation looks quite nice. Just a few details for edge cases left, I think. Don't get alarmed by the number of comments, half of them are merely about style nits.
In a follow-up, we can remove the old ft2232_spi_send_command() implementation, and replace the pointer with `default_spi_send_ command`.
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/comment/a45125c7_cf903df9 PS25, Line 278: struct spi_command *cmds) This is very oddly indented. Tabs and spaces but still not aligned to anything.
You can also remove the line break. Our limit is officially 112 chars.
https://review.coreboot.org/c/flashrom/+/40477/comment/216dcc54_dfd97ae2 PS25, Line 283: int i = 0,ret = 0, failed = 0; missing space after first comma.
https://review.coreboot.org/c/flashrom/+/40477/comment/52adbddd_487d0cad PS25, Line 294: /* Overflow check for buf Should start with a
/*
on a line of its own.
https://review.coreboot.org/c/flashrom/+/40477/comment/c2cdd203_870097c7 PS25, Line 295: 9 This is 12 now becaues the deassertion of CS# is also packed, AFAICS.
https://review.coreboot.org/c/flashrom/+/40477/comment/ca9164c4_4e2ceab4 PS25, Line 304: 0 & ~pinlvl; The respective expression in ft2232_spi_send_command() got a fix in the meantime, this should be
buf[i++] = ~ 0x08 & pinlvl; /* assert CS (3rd) bit only */
now.
https://review.coreboot.org/c/flashrom/+/40477/comment/ac45471e_0d9841e2 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 be cleaner by adding a function:
static bool ft2232_spi_command_fits(const struct spi_command *cmd, size_t buffer_size) { return 12 + cmd->writecnt + cmd->readcnt <= buffer_size; }
And call it with `(cmds + 1, ARRAY_SIZE(buf) - i)` here and with `(cmds, ARRAY_SIZE(buf) - i)` above.
If we don't check it here, we might run into a situation where spi_send_multicommand() fails while individual spi_send_command() calls would work.
also, missing space before {
https://review.coreboot.org/c/flashrom/+/40477/comment/70877e02_c659906d PS25, Line 326: optionally Either
/* An optional read command */
or
/* Optionally, a read command */
https://review.coreboot.org/c/flashrom/+/40477/comment/4e560b59_49b80d2a 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 would be false. So we can as well `break;` here, and wouldn't have to check `ret` below and also not in the loop condition.
Moreover, as sending the deassertion of CS# is packed into the first command now, there is no way for `failed` to differ from `ret`, so we can just drop `failed` and check `ret` below at the `return`.
https://review.coreboot.org/c/flashrom/+/40477/comment/f3d5a483_956983eb 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.
(I'm not sure if the comment is valid actually.)