Attention is currently required from: Felix Singer, Nico Huber, Angel Pons. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57810 )
Change subject: ft2232_spi: reintroduce generic GPIOL control ......................................................................
Patch Set 17:
(4 comments)
File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/57810/comment/b54c6a96_750bd501 PS16, Line 483: msg_perr
As the message says "warning", use msg_pwarn()?
Done
https://review.coreboot.org/c/flashrom/+/57810/comment/317fcb77_bd3b759f 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 […]
Done
https://review.coreboot.org/c/flashrom/+/57810/comment/957647d4_5cb08188 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);
Yep, nice idea. I took the second one.
`char *const arg` doesn't work, since arg is declared globally already. I'm reusing that one instead.
Also just noticed: In the current form, any premature `return` inside the loop would leak some of these arg_gpiol[] pointers.
Sure, any return would need `free(arg)` before. That also applies to the solutions above, doesn't it?
https://review.coreboot.org/c/flashrom/+/57810/comment/8616d729_ceb1049e PS16, Line 538: bool format_error = 0;
Just to mention it, don't know how reasonable it would look: For error […]
Heh, I thought about using it but feared people (you :P) wouldn't like it :D Done
-
Michael Niewöhner (Code Review)