Looks good but I fear some things have to change. At least the `transferred`
pointer to libusb_interrupt_transfer() and the missing libusb_exit().

If it's not too much trouble, it would be nice to retest, but only after the
review is done. Doesn't have to cover a whole flash, a layout that covers two
or more erase blocks with `--noverify-all` could speed things up. `--fmap` is
also a nice test for nearly abitratry reads (if the FMAP is hard to find, i.e.
not at a naturally aligned location).

Two things were noticed during review that should be fixed across all libusb
programmers in the future:

• We shouldn't use the default context (passing NULL to libusb_init() etc.). It could be one libusb context per driver instance (flashrom_programmer_context), or maybe better a single one in a yet to be invented `flashrom_context` that is shared among driver instances.
• We could always use libusb_strerror() in the code and implement our own (that simply returns the return code number as string, for instance) for older libusb versions, based on a version check.

View Change

10 comments:

• File pickit2_spi.c:

  • Patch Set #7, Line 54: #endif

    Not used here in this file? Then drop it.

  • Patch Set #7, Line 104: (unsigned char *)

    This cast shouldn't be needed any more. And many more below...

  • Patch Set #7, Line 104: NULL

    NULL is only allowed since libusb 1.0.21. Sorry.

  • Patch Set #7, Line 366: libusb_strerror

    It was only introduced 6 years ago, I guess that is why it's not
    used by any other driver. As you dropped most of these calls, let's
    not use it for now but keep it in mind.

  • Patch Set #7, Line 373: libusb_close(pickit2_handle);

    libusb_exit(NULL);

  • Patch Set #7, Line 428: }

    Not a bug, could be a library user opening the device a second
    time. Just drop, please.

  • Patch Set #7, Line 430: NULL

    NB. This will have to change in the future, but for all libusb
    drivers.

  • Patch Set #7, Line 445: msg_perr("Could not open device PICkit2!\n");

    libusb_exit(NULL);

  • Patch Set #7, Line 451: libusb_close(pickit2_handle);

    libusb_exit(NULL);

  • Patch Set #7, Line 456: libusb_close(pickit2_handle);

    libusb_exit(NULL);

To view, visit change 32213. To unsubscribe, or for help writing mail filters, visit settings.