Hello Bernhard, Thanks, this is clearly a useful feature.
On Wed, 16 Mar 2016 16:43:18 +0100 Bernhard Nortmann <[email protected]> wrote: > See https://github.com/linux-sunxi/sunxi-tools/issues/37 > > The patch introduces a "--dev" (-d) option to specify the > desired FEL device. This is useful if multiple target devices > are connected to the same host. > > Signed-off-by: Bernhard Nortmann <[email protected]> Should we also give more explicit credit in the commit message to NextThingCo people for their work on the initial implementation of this patch? Yes, I know that the github tracker link has all the necessary details, but still we could be a bit nicer. I have added Wynter Woods to CC. > fel.c | 101 > +++++++++++++++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 78 insertions(+), 23 deletions(-) > > diff --git a/fel.c b/fel.c > index 59f0f72..3e6c6ae 100644 > --- a/fel.c > +++ b/fel.c > @@ -1270,12 +1270,67 @@ static unsigned int file_upload(libusb_device_handle > *handle, size_t count, > return i; // return number of files that were processed > } > > +/* open libusb handle to desired FEL device */ > +static void request_libusb_handle(libusb_device_handle **handle, > + int busnum, int devnum, uint16_t vendor_id, uint16_t product_id) > +{ > + if (busnum < 0 || devnum < 0) > + // With the default values (busnum -1, devnum -1) we don't care > + // for a specific USB device; so let libusb open the first > + // device that matches VID/PID. > + *handle = libusb_open_device_with_vid_pid(NULL, vendor_id, > product_id); Some style nitpicks here: 1) Mixing the C++ '//' comments with the C '/* */' comments does not look very consistent. Probably it's best to stick to a single style across the whole source file. 2) https://www.kernel.org/doc/Documentation/CodingStyle Do not unnecessarily use braces where a single statement will do. if (condition) action(); and if (condition) do_this(); else do_that(); This does not apply if only one branch of a conditional statement is a single statement; in the latter case use braces in both branches: if (condition) { do_this(); do_that(); } else { otherwise(); } > + else { > + // look for specific bus and device number > + pr_info("Selecting USB Bus %03d Device %03d\n", busnum, devnum); > + libusb_device **list; > + size_t ndevs, i; > + bool found = false; > + > + ndevs = libusb_get_device_list(NULL, &list); > + for (i = 0; i < ndevs; i++) A style nitpick again. It would be probably better to add {} around the loop body here because the loop body is not just a single line statement. > + if (libusb_get_bus_number(list[i]) == busnum > + && libusb_get_device_address(list[i]) == devnum) { > + found = true; // bus:devnum matched > + struct libusb_device_descriptor desc; > + libusb_get_device_descriptor(list[i], &desc); > + if (desc.idVendor != vendor_id > + || desc.idProduct != product_id) { > + fprintf(stderr, "ERROR: Bus %03d Device > %03d not a FEL device (expected %04x:%04x, got %04x:%04x)\n", I'm not too keen on strictly enforcing the 80 characters line limit everywhere, but this particular line can be easily made shorter: fprintf(stderr, "ERROR: Bus %03d Device %03d not a FEL device " "(expected %04x:%04x, got %04x:%04x)\n", ...); > + busnum, devnum, vendor_id, > product_id, desc.idVendor, desc.idProduct); > + exit(1); > + } > + // open handle to this specific device > (incrementing its refcount) > + libusb_open(list[i], handle); Are we sure that the *handle variable is set to NULL if libusb_open() call fails? The documentation at http://libusb.sourceforge.net/api-1.0/group__dev.html#ga8163100afdf933fabed0db7fa81c89d1 says "handle - output location for the returned device handle pointer. Only populated when the return code is 0" > + break; > + } > + libusb_free_device_list(list, true); > + > + if (!found) { > + fprintf(stderr, "ERROR: Bus %03d Device %03d not found > in libusb device list\n", > + busnum, devnum); > + exit(1); > + } > + } > + if (!*handle) { > + switch (errno) { Are we sure that the 'errno' has not been overwritten by something else by now? The http://man7.org/linux/man-pages/man3/errno.3.html says: A common mistake is to do if (somecall() == -1) { printf("somecall() failed\n"); if (errno == ...) { ... } } where errno no longer needs to have the value it had upon return from somecall() (i.e., it may have been changed by the printf(3)). If the value of errno should be preserved across a library call, it must be saved: if (somecall() == -1) { int errsv = errno; printf("somecall() failed\n"); if (errsv == ...) { ... } } > + case EACCES: > + fprintf(stderr, "ERROR: You don't have permission to > access Allwinner USB FEL device\n"); > + break; > + default: > + fprintf(stderr, "ERROR: Allwinner USB FEL device not > found!\n"); > + break; > + } > + exit(1); > + } > +} > + > int main(int argc, char **argv) > { > bool uboot_autostart = false; /* flag for "uboot" command = U-Boot > autostart */ > bool pflag_active = false; /* -p switch, causing "write" to output > progress */ > int rc; > libusb_device_handle *handle = NULL; > + int busnum = -1, devnum = -1; > int iface_detached = -1; > rc = libusb_init(NULL); > assert(rc == 0); > @@ -1284,6 +1339,7 @@ int main(int argc, char **argv) > printf("Usage: %s [options] command arguments... [command...]\n" > " -v, --verbose Verbose > logging\n" > " -p, --progress \"write\" > transfers show a progress bar\n" > + " -d, --dev bus:devnum Use specific > USB bus and device number\n" > "\n" > " spl file Load and > execute U-Boot SPL\n" > " If file additionally contains a main > U-Boot binary\n" > @@ -1316,18 +1372,29 @@ int main(int argc, char **argv) > ); > } > > - handle = libusb_open_device_with_vid_pid(NULL, 0x1f3a, 0xefe8); > - if (!handle) { > - switch (errno) { > - case EACCES: > - fprintf(stderr, "ERROR: You don't have permission to > access Allwinner USB FEL device\n"); > - break; > - default: > - fprintf(stderr, "ERROR: Allwinner USB FEL device not > found!\n"); > - break; > - } > - exit(1); > + /* process all "prefix"-type arguments first */ > + while (argc > 1) { > + if (strcmp(argv[1], "--verbose") == 0 || strcmp(argv[1], "-v") > == 0) > + verbose = true; > + else if (strcmp(argv[1], "--progress") == 0 || strcmp(argv[1], > "-p") == 0) > + pflag_active = true; > + else if (strncmp(argv[1], "--dev", 5) == 0 || strncmp(argv[1], > "-d", 2) == 0) { > + char *dev_arg = argv[1]; > + dev_arg += strspn(dev_arg, "-dev="); // skip option > chars, ignore '=' > + if (*dev_arg == 0 && argc > 2) { // at end of argument, > use the next one instead > + dev_arg = argv[2]; > + argc -= 1; > + argv += 1; > + } > + busnum = strtoul(dev_arg, &dev_arg, 10); > + devnum = strtoul(dev_arg + 1, NULL, 10); What if the 'dev_arg' string is malformed and contains only a single decimal number without the ':' separator? It is probably best to rewrite this as if (sscanf(dev_arg, "%u:%u", &busnum, &devnum) != 2) { fprintf(stderr, "ERROR: Expected 'bus:devnum', got '%s'.\n", dev_arg); exit(1); } > + } else > + break; /* no valid (prefix) option detected, exit loop > */ > + argc -= 1; > + argv += 1; > } > + > + request_libusb_handle(&handle, busnum, devnum, 0x1f3a, 0xefe8); > rc = libusb_claim_interface(handle, 0); > #if defined(__linux__) > if (rc != LIBUSB_SUCCESS) { > @@ -1343,18 +1410,6 @@ int main(int argc, char **argv) > exit(1); > } > > - /* process all "prefix"-type arguments first */ > - while (argc > 1) { > - if (strcmp(argv[1], "--verbose") == 0 || strcmp(argv[1], "-v") > == 0) > - verbose = true; > - else if (strcmp(argv[1], "--progress") == 0 || strcmp(argv[1], > "-p") == 0) > - pflag_active = true; > - else > - break; /* no valid (prefix) option detected, exit loop > */ > - argc -= 1; > - argv += 1; > - } > - > while (argc > 1 ) { > int skip = 1; > -- Best regards, Siarhei Siamashka -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
