Attention is currently required from: Simon Buhrow, Alan Green, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić.
8 comments:
File ft2232_spi.c:
Patch Set #25, Line 189: ft2232_spi_send_command
Ack
Maybe we should do this rather early to have more test coverage.
Patch Set #25, Line 317: if (!cmds->readcnt && ((cmds + 1)->writecnt || (cmds + 1)->readcnt)){
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.
Well, you placed it really far up. I guess it could be directly above
ft2232_spi_send_multicommand(), no?
More important: I think we need to reset 'i' after send_buf in case the next command does not fit into buffer?
Oh, right. I completely missed that (it is missing). It should be
always reset after send_buf(), as that's where we consume all the
gathered commands.
File ft2232_spi.c:
Patch Set #29, Line 171: return 12 + cmd->writecnt + cmd->readcnt <= buffer_size;
I just realized we could make the check more accurate, taking into account
if we are going to write and read or just one of them. e.g.
const size_t cmd_len = 3; /* same length for any ft2232 command */
return
/* commands for CS# assertion and de-assertion: */
cmd_len + cmd_len
/* commands for either a write, a read or both: */
+ (cmd->writecnt && cmd->readcnt ? cmd_len + cmd_len : cmd_len)
/* payload: */
+ cmd->writecnt + cmd->readcnt
<= buffer_size;
Patch Set #29, Line 301: (cmds->writecnt || cmds->readcnt)
Nit, doesn't need parentheses anymore.
/* Overflow check for buf
* 12 : up to 12 CMD-Bytes are added below
*/
I think we can drop the comment here, the function name should be self-explanatory.
((cmds + 1)->writecnt || (cmds + 1)->readcnt) &&
ft2232_spi_command_fits((cmds + 1), FTDI_HW_BUFFER_SIZE - i)) {
Please indent either with 4 spaces or two tabs (relative to the `if`).
It shouldn't be indented like the if-body.
msg_perr("send_buf failed: %i\n", ret);
break;
Needs braces.
msg_perr("get_buf failed: %i\n", ret);
break;
Same here.
To view, visit change 40477. To unsubscribe, or for help writing mail filters, visit settings.