Nico Huber 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 4: Code-Review+1
(5 comments)
Looks quite good. Only blocker is the missing . in the manpage. The other comments are mostly for my understanding.
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);
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
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?
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, I don't know what it is.
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 it in stlinkv3_check_version() below.
https://review.coreboot.org/c/flashrom/+/34661/4/stlinkv3_spi.c@170 PS4, Line 170: reqd what is `reqd`?