Attention is currently required from: David Reguera Garcia, Thomas Heijligen.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email )
Change subject: buspirate_spi: add support for hiz output with pullups=off ......................................................................
Patch Set 8: Code-Review+1
(5 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/79299/comment/eb9f8f34_4cf3a7b1 : PS8, Line 12: Since you merged two patches together, you can add a paragraph from the other patch here, you can just copy-paste (with 72 chars width)
Return init error for invalid values of pullups, hiz, psus. Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. `pullups=1` and expected pullups to be on, while the value `1` was handled as invalid and off.
Is the current text in commit message 72 chars width?
Patchset:
PS8:
Anastasia, can you take a look now?
Great, thank you! I have few comments, which are just code style and commit message, and that would be it!
File buspirate_spi.c:
https://review.coreboot.org/c/flashrom/+/79299/comment/cb7f0137_cee9d4d9 : PS8, Line 374: if (strcasecmp("on", tmp) == 0) : pullup = true; : else if (strcasecmp("off", tmp) == 0) : ; // ignore : else { : msg_perr("Invalid pullups state. Use on/off.\n"); : free(tmp); : return 1; : } For this if-else-elseif and the ones below:
As per coding style, if at least one branch is multi-line and needs braces, put braces for all branches. Previously, all the branches were 1-line, and no braces. Since now you added a multi-line else branch, you need to add braces for existing if and else if.
You don't need to go through the whole file and check everything! Just update the places that you modified in the patch.
(it is p.3 from here https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...)
https://review.coreboot.org/c/flashrom/+/79299/comment/e92bc517_cc6a0b42 : PS8, Line 390: else if (strcasecmp("off", tmp) == 0) : { : if (pullup) : { I think this can be more compact:
else if ((strcasecmp("off", tmp) == 0) && pullup) { error message, free, return } else { ... <same as now> }
https://review.coreboot.org/c/flashrom/+/79299/comment/e1053fea_32db6e1a : PS8, Line 394: pullups on & hiz off at same time is not possible Just slightly add on this message:
Invalid combination: pullups=on & hiz=off at same time is not possible.\n