Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34661 )
Change subject: Add support for STLINK V3 debugger/programmer via its SPI bridge ......................................................................
Patch Set 5:
(5 comments)
AFAICS, the code in stlinkv3_spi.c could be much condensed by a simple function that handles the command/answer pattern, e.g.
int stlinkv3_command(const uint8_t *command, size_t command_length, uint8_t *answer, size_t answer_length);
Many thanks for the review! I have fixed your review comments, but have not tested with actual hardware yet. I will try to test it tomorrow and let you know the results.
https://review.coreboot.org/c/flashrom/+/34661/4/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/34661/4/flashrom.8.tmpl@1191 PS4, Line 1191: SS
.SS
Done
https://review.coreboot.org/c/flashrom/+/34661/4/stlinkv3_spi.c File stlinkv3_spi.c:
https://review.coreboot.org/c/flashrom/+/34661/4/stlinkv3_spi.c@112 PS4, Line 112: LAST
What does `last` refer to? the last known version? the last digit of the version?
ST versions the STLink's firmware parts separately (JTAG/Bridge/SWIM/Serial). This constant is used for checking the bridge firmware version to make sure that it is compatible with the current host implementation. I have renamed this constant to FIRST_COMPATIBLE_BRIDGE_FW_VERSION which is more descriptive I think.
https://review.coreboot.org/c/flashrom/+/34661/4/stlinkv3_spi.c@114 PS4, Line 114: #define USB_TIMEOUT 5000
Can you give the unit in the name? e.g. USB_TIMEOUT_MS, if it's ms, […]
Sure, it is in milliseconds.
https://review.coreboot.org/c/flashrom/+/34661/4/stlinkv3_spi.c@138 PS4, Line 138: memset(answer, 0, sizeof(answer));
This is not necessary, is it? I only noticed, because you didn't clear […]
Done
https://review.coreboot.org/c/flashrom/+/34661/4/stlinkv3_spi.c@170 PS4, Line 170: reqd
what is `reqd`?
requested sorry.