Attention is currently required from: Felix Singer, Alan Green, Angel Pons, Michael Niewöhner. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57810 )
Change subject: ft2232_spi: reintroduce generic GPIOL control ......................................................................
Patch Set 16: Code-Review+1
(5 comments)
Patchset:
PS16: Alan, would the new syntax suit your needs?
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/57810/comment/310892f1_87318f3a PS16, Line 483: msg_perr As the message says "warning", use msg_pwarn()?
https://review.coreboot.org/c/flashrom/+/57810/comment/9d2ecba1_f5c3f5d0 PS16, Line 484: "in the future. Use `gpiolX=C` instead.\n"); We try to keep output within 80 columns. Could add a \n after the first sentence for instance.
https://review.coreboot.org/c/flashrom/+/57810/comment/4586c346_7eaeb773 PS16, Line 513: arg_gpiol[3] = extract_programmer_param("gpiol3"); This looks a little odd. How about moving the call into the loop? We could still use an array for the string literals, e.g.
const char *const gpiol_params[] = { "gpiol0", "gpiol1", ... char *const arg = extract_programmer_param(gpiol_params[pin]);
Alternatively, a single buffer could be used like this:
char gpiol_param[7]; snprintf(gpiol_param, sizeof(gpiol_param), "gpiol%d", pin); char *const arg = extract_programmer_param(gpiol_param);
Also just noticed: In the current form, any premature `return` inside the loop would leak some of these arg_gpiol[] pointers.
https://review.coreboot.org/c/flashrom/+/57810/comment/8d311a6c_c4f6ed80 PS16, Line 538: bool format_error = 0; Just to mention it, don't know how reasonable it would look: For error paths, using `goto` is common. The end of the loop body could look like this:
free(arg); continue; format_error: msg_perr("Error: ..."); free(arg); return -2; }
It would save us an indentation level.
-
Nico Huber (Code Review)