Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41533 )
Change subject: raiden_debug_spi.c: Adds support for USB SPI protocol V2 ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/c/flashrom/+/41533/2/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41533/2/raiden_debug_spi.c@391 PS2, Line 391:
alignment \t \s mixing.
Ack
https://review.coreboot.org/c/flashrom/+/41533/2/raiden_debug_spi.c@720 PS2, Line 720: int
This variable should be declared out of the loop, c.f. […]
Ack
https://review.coreboot.org/c/flashrom/+/41533/2/raiden_debug_spi.c@726 PS2, Line 726:
Replace 8 spaces with a tab?
I'm using spaces to align these multi-line function calls, especially the ones with mutli-line strings. They should have the same level of tabbed indentation.
https://review.coreboot.org/c/flashrom/+/41533/2/raiden_debug_spi.c@782 PS2, Line 782: status =
Sounds good
Ack
https://review.coreboot.org/c/flashrom/+/41533/2/raiden_debug_spi.c@855 PS2, Line 855: PACKET_ID_V2_RESPONSE_CONTINUE) {
This fits on a single line, or is it very close to the limit?
I think it's 79 (w/ 4 tabs) characters, I'll switch it if it's still reasonable.
https://review.coreboot.org/c/flashrom/+/41533/2/raiden_debug_spi.c@913 PS2, Line 913: } else {
How about "else if" here? […]
I can add these as function pointers in 'struct raiden_debug_spi_data'.
These would then follow this template and no chains are needed.
if (ctx_data->write_command) { ctx_data->write_command(ctx_data, &write_ctx, &read_ctx); } else { /* Raise error */ msg(...) }
The same without the error else state can be done for the restart_response_v2(ctx_data) later on too. If a v3 protocol exists, it just fills in the fields that it uses.
https://review.coreboot.org/c/flashrom/+/41533/2/raiden_debug_spi.c@1063 PS2, Line 1063:
indent has spaces not tab
Ack