Attention is currently required from: Simon Buhrow, Alan Green, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Samir Ibradžić.
Patch set 35:Code-Review +1
18 comments:
Commit Message:
Patch Set #8, Line 17: https://ibb.co/0c1J25d
I wouldn't provide them in a commit message
Done
Commit Message:
Patch Set #16, Line 25: in a similar way.
The infrastructure is already in place. You just have to implement […]
Done
Patchset:
> OK, I understand (I have/had got no idea of VM stuff). […]
Done
Patchset:
Thanks for paying attention again. […]
I'll take over then. It's only a few nits left after all.
File ft2232_spi.c:
Patch Set #8, Line 542: /* Return to get second op (Program or Erase) without resetting buf nor i*/
s Line 473
Ack
File ft2232_spi.c:
Patch Set #16, Line 172: .multicommand = default_spi_send_multicommand,
I got it. […]
Done
Patch Set #16, Line 497: bufsize = max(writecnt + 19, 261 + 19);
> What do you want to drop? Just the change from 260+9 to 261+19 or the whole max()-line? […]
Done
Patch Set #16, Line 562: if (writearr[0] == JEDEC_WREN) {
You are right, I was assuming JEDEC_WREN in any case. […]
Done
File ft2232_spi.c:
Patch Set #25, Line 189: ft2232_spi_send_command
Maybe we should do this rather early to have more test coverage.
Done in a subsequent commit.
File ft2232_spi.c:
Patch Set #33, Line 290: + cmd->readcnt
One tiny mistake left. I also didn't realize this before: The buffer `buf` […]
Done
File ft2232_spi.c:
I guess this space is not intentional. In any case, it should align […]
Done
Patch Set #34, Line 283: const size_t cmd_len = 3; /* same length for any ft2232 command */
I don't think it's necessary, as the literal value is never used outside of this function. […]
I agree with Angel, it should be kept as close as possible to the code it's used in.
Patch Set #34, Line 290: + cmd->writecnt + cmd->readcnt
The buffer in question does not include the `readcnt` part.
Patch Set #34, Line 300: int i = 0, ret = 0;
Shouldn't `i` be `unsigned int` or `size_t`? […]
Ack
It's used as index for a byte array and as parameter to
ft2232_spi_command_fits(), so `size_t`.
Patch Set #34, Line 307: 65536
hmmm, max_data_write is 256
That is right, and assuming an overall sound flashrom, we shouldn't
hit this case. But it seems a good idea to check this "external" data
locally again. Or do you mean we could make the check tighter? I
guess we could do so. Probably in a follow up as this just mimicks
the original code.
Patch Set #34, Line 311: Out of memory!
`Out of memory!` is usually printed when a malloc() or similar function fails. […]
Ack
"Command does not fit" to align with other error messages
that don't have a "FTDI" prefix.
nit: no space between unary operators: […]
Generally right, but it's mimicking the original code and that will
be reverted later. I hope it's ok to leave it for the moment.
Patch Set #34, Line 349: /* send_buf */
I'd say this comment is redundant, and would remove it
Ack
To view, visit change 40477. To unsubscribe, or for help writing mail filters, visit settings.