Change in flashrom[master]: programmer: Add Developerbox/CP2104 bit bang driver

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 -- To view, visit https://review.coreboot.org/26948 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2547a96c1a2259ad0d52cd4b6ef42261b37cccf3 Gerrit-Change-Number: 26948 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Thompson <daniel.thompson@linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson@linaro.org> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Idwer Vollering <vidwer@gmail.com> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Wed, 11 Jul 2018 16:04:43 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
participants (1)
-
Nico Huber (Code Review)