Nice patch!

Found some nits, and got one idea how the loop could be refactored.

Patch set 2:Code-Review +1

View Change

4 comments:

• File dediprog.c:

  • Patch Set #2, Line 826: /* Read the id from the dediprog. This should return the numeric part of the

    Please start comments with /* on a separate line.

  • Patch Set #2, Line 1089: strlen(prefix) != 2

    This is true by definition.

  • Patch Set #2, Line 1094: dediprog_id_prefix(id)

    or just `prefix`

  • Patch Set #2, Line 1229: }

    One thought to make this whole looping easier to read (not 100% sure if
    it would look better):

    Move the original three calls (_dev_get, _set_configuration, _claim_interface)
    into a separate function (e.g. dediprog_open()). Then it should be easier to
    separate the `id` case from the `usedevice` case:

        if (id != -1) {
                for (i = 0; ; i++) {
                        ret = dediprog_open(i);
                        if (ret == -1) {
                                /* no dev */
                                ...
                                return 1;
                        } else if (ret == -2) {
                                /* busy dev */
                                continue;
                        }

                        found_id = dediprog_read_id();
                        ...
                        break;
                }
        } else {
                if (dediprog_open(usedevice)) {
                        ...
                        return 1;
                }
                found_id = dediprog_read_id();
        }

    Or maybe even put the loop body into yet another function (makes it easier
    to have a common error path).

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