Nico Huber has posted comments on this change. ( https://review.coreboot.org/26948 )
Change subject: programmer: Add Developerbox/CP2104 bit bang driver ......................................................................
Patch Set 3:
(16 comments)
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c File developerbox_spi.c:
PS3: Hex literals are sometimes written in upper case, sometimes in lower case, please unify (personally, I would prefer all lower case).
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@8 PS3, Line 8: version 2 of the License is it intentional to restrict the license to v2?
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@16 PS3, Line 16: /* Bit bang driver for the 96Boards Developerbox (a.k.a. Synquacer E-series) nit, please start comment with /* on an otherwise empty line
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@18 PS3, Line 18: will which?
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@27 PS3, Line 27: UART DSW4 nit, maybe place a comma between `UART, DSW4`
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@28 PS3, Line 28: DSW4-1 DSW4-1 is the most significant bit?
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@29 PS3, Line 29: * turned on) nit, missing full-stop
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@34 PS3, Line 34: #include <sys/types.h> : #include <stdlib.h> : #include <stdio.h> : #include <string.h> : #include <limits.h> : #include <errno.h> : #include <libusb.h> : #include "flash.h" : #include "chipdrivers.h" : #include "programmer.h" : #include "spi.h" The list seems excessive (for the little code below), are these all needed?
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@137 PS3, Line 137: static struct libusb_device_handle *get_device_by_vid_pid_serial(uint16_t vid, uint16_t pid, char *serialno) We should make this more generic and generally available instead of copying the code around.
I would prefer that to happen before this gets in, but it doesn't have to.
Btw. if this function is the reason for the GPL v2 only: I wrote it, and would sign-off any patch that makes it GPL v2+.
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@137 PS3, Line 137: char * `const char *` please
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@173 PS3, Line 173: 64 sizeof(myserial)
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@181 PS3, Line 181: strlen(serialno) strncmp() with `n == strlen()` of one of its arguments looks very odd. If this is to compensate for a missing `\0` termination in `myserial`, please use memcmp() instead. Hmm, but it seems not, the msg_pdbg() would already fail. If you are just comparing two C strings, just use strcmp().
What it effectively does here is allowing `serialno` to be a prefix of `myserial`. If that is intentional, please add a comment (I've seen this pattern very often and it was almost always not intentional).
Now I've also looked into libusb, they always add the terminating `\0`. Wonder, why they don't mention that in the documentation...
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@181 PS3, Line 181: (char *) Nit, no space after the closing parenthesis, please.
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@207 PS3, Line 207: commencing heh, should have read this first...
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@227 PS3, Line 227: return 1; libusb_exit(...)
https://review.coreboot.org/#/c/26948/3/developerbox_spi.c@232 PS3, Line 232: return 1; same here
-
Nico Huber (Code Review)