I like the USB code and its potential to unify programmer
parameters. It belongs into a separate patch, though, to
ease review.

View Change

5 comments:

• File raiden_debug_spi.c:

  • Patch Set #3, Line 35: * Software Foundation.

    I guess one could infer GPLv3 compatibility from the 3-clause above. But
    IANAL, so I better ask. Can we get this under "GPLv2 and later"? I try
    to encourage contributors to this because of libflashrom. Having a GPLv2-
    only library is not of much use these days (won't get better in the future,
    I guess).

    We are still far from a GPLv2+ flashrom core, though :-/

  • Patch Set #3, Line 154: * maximum USB packet size for the device.

    NB. Writing and reviewing such comments takes much more time than fixing the
    interface, IMHO.

  • Patch Set #3, Line 157: command header maximum size

    That's kind of true, but highly confusing when one knows what the macros
    are actually referring to:

    JEDEC_BYTE_PROGRAM_OUTSIZE is the size of a full byte-program command
    not just a header. It's including the byte to be programmed. The calculation
    is correct nevertheless, because it matches the `header` of a 4BA byte
    programm.

    Anyway, it renders this paragraph useless, IMO.

• File usb_device.h:

  • Patch Set #3, Line 64: return

    That kind of thing breaks people's internal parsers

    I agree, this macro is a blocker. Hidden control flow statements are not
    a good idea.

    Also, the whole expression is not valid C. Turning a compound statement
    into an expression ({ ... }) is a GNU extension, IIRC. And with our limited
    compile testing, I wouldn't want to take any risks.

    Also, with an implementation in a C function, one doesn't have to repeat
    too much boilerplate, e.g.

        ret = libusb_control_transfer(...);
        if (usb_check_error(ret, "OMG, it failed"))
                return ret;

    I think that would be much easier to read than those CHECK(LIBUSB(libusb_...
    blocks.

• File usb_device.c:

  • Patch Set #3, Line 35: * Software Foundation.

    See `raiden_debug_spi.c`.

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