Attention is currently required from: Simon Buhrow, Alan Green, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić.
13 comments:
Commit Message:
Patch Set #25, 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()
Patch Set #25, 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).
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:
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:
Patch Set #25, 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.
Patch Set #25, Line 283: int i = 0,ret = 0, failed = 0;
missing space after first comma.
Patch Set #25, Line 294: /* Overflow check for buf
Should start with a
/*
on a line of its own.
This is 12 now becaues the deassertion of CS# is also packed, AFAICS.
Patch Set #25, 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.
Patch Set #25, 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 {
Patch Set #25, Line 326: optionally
Either
/* An optional read command */
or
/* Optionally, a read command */
Patch Set #25, 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`.
* 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.)
To view, visit change 40477. To unsubscribe, or for help writing mail filters, visit settings.