Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34160 )
Change subject: dediprog.c: Add id parameter to dediprog programmer ......................................................................
Patch Set 2: Code-Review+1
(4 comments)
Nice patch!
Found some nits, and got one idea how the loop could be refactored.
https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c File dediprog.c:
https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@826 PS2, Line 826: /* Read the id from the dediprog. This should return the numeric part of the Please start comments with /* on a separate line.
https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@1089 PS2, Line 1089: strlen(prefix) != 2 This is true by definition.
https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@1094 PS2, Line 1094: dediprog_id_prefix(id) or just `prefix`
https://review.coreboot.org/c/flashrom/+/34160/2/dediprog.c@1229 PS2, 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).