16 comments:
Hex literals are sometimes written in upper case, sometimes in lower
case, please unify (personally, I would prefer all lower case).
Patch Set #3, Line 8: version 2 of the License
is it intentional to restrict the license to v2?
Patch Set #3, 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
which?
Patch Set #3, Line 27: UART DSW4
nit, maybe place a comma between `UART, DSW4`
Patch Set #3, Line 28: DSW4-1
DSW4-1 is the most significant bit?
Patch Set #3, Line 29: * turned on)
nit, missing full-stop
#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?
Patch Set #3, 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+.
Patch Set #3, Line 137: char *
`const char *` please
sizeof(myserial)
Patch Set #3, 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...
Patch Set #3, Line 181: (char *)
Nit, no space after the closing parenthesis, please.
Patch Set #3, Line 207: commencing
heh, should have read this first...
Patch Set #3, Line 227: return 1;
libusb_exit(...)
Patch Set #3, Line 232: return 1;
same here
To view, visit change 26948. To unsubscribe, or for help writing mail filters, visit settings.