Attention is currently required from: Nico Huber, Edward O'Callaghan, Light, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63130 )
Change subject: pony_spi.c: Extract out get_params to simplify init ......................................................................
Patch Set 3:
(7 comments)
File pony_spi.c:
https://review.coreboot.org/c/flashrom/+/63130/comment/8833d157_f0edb292 PS3, Line 153: { nit: add space before opening brace
https://review.coreboot.org/c/flashrom/+/63130/comment/10aeaede_615bf794 PS3, Line 167: bool have_device;
From my point of view: Review doesn't scale well with the size of a patch. Even if it's a very simple patch hunk, every additional hunk can distract from the error-prone part of a patch. Doing fixes and simple changes like this in advance works well, FWIW.
+1
For me, it's very easy to miss something when reviewing commits that do multiple things at once. I'd rather review multiple commits where each does one and only one thing.
Whenever I want/need to make significant code changes, I draft in advance a "plan of action". I use several criteria to segment the changes into commits, one of them is "at most one thing/reason/idea per commit".
In this case, "simplify `pony_spi` init logic" and "use `bool` type for boolean variables" are two different things, and they are conceptually independent: disregarding merge conflicts, it's possible to do them in either order, or only do one of them. When accounting for merge conflicts, I start with the easiest (most likely to be approved) commits, so that I don't have to manually rebase them if another commit needs to be fixed.
That being said, there's no need to over-split things. For example, I'd retype both `have_device` and `have_prog` in the same commit, because the rationale is the same.
TL;DR: Please retype the variables in a separate commit (preferably before the "extract `get_params`" commit to streamline the submission process).
https://review.coreboot.org/c/flashrom/+/63130/comment/523207c5_d0f3c3b6 PS3, Line 171: return 1; If the `dev` parameter is correct (i.e. `have_device` is true) but `get_params()` returns -1 because of an error with the `type` parameter, the serial port (what `serialport_shutdown()` releases) will leak.
https://review.coreboot.org/c/flashrom/+/63130/comment/3b064e8c_49fe92dc PS3, Line 181: return 1; Serial port can also leak here.
https://review.coreboot.org/c/flashrom/+/63130/comment/229d3eda_23804449 PS3, Line 190: serialport_shutdown
I think you are mis-interpreting the diff presented by gerrit Anastasia.
This line is OK here, but it's missing in some other return paths.
https://review.coreboot.org/c/flashrom/+/63130/comment/54be3b14_cde22584 PS3, Line 232: true This is unrelated. Please do it in a separate commit.
https://review.coreboot.org/c/flashrom/+/63130/comment/9797c619_2ea34563 PS3, Line 237: true Ditto