Attention is currently required from: David Reguera Garcia, Thomas Heijligen.
Patch set 8:Code-Review +1
5 comments:
Commit Message:
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:
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:
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/Documentation/process/coding-style.rst)
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>
}Patch Set #8, 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
To view, visit change 79299. To unsubscribe, or for help writing mail filters, visit settings.