Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 )
Change subject: ichspi: Split very long init function into two ......................................................................
Patch Set 2: Code-Review+1
(4 comments)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58735/comment/25bc9f2d_f4921a3a PS1, Line 1836: arg);
Another unnecessary line break.
This is addressed in the follow-up commit, can we leave this as-is in this commit?
https://review.coreboot.org/c/flashrom/+/58735/comment/36f5fd9e_2234312c PS1, Line 1917: );
Drop line break?
yeah, the line break before `);` looks really odd.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58735/comment/3c66b125_d75e4ae8 PS2, Line 1822: arg = extract_programmer_param("ich_spi_mode"); I use a very stupid trick to minimise diffstat noise when reindenting blocks of code during refactoring: I change the indentation in a separate, reproducible commit, so that the diffstat of the non-reproducible part is much smaller.
I've done this several times when refactoring coreboot code. For example, I wanted to refactor some chipset code, but all mainboards would need to be adapted. I started with CB:46702 CB:46703 CB:46704 CB:46705 to do nearly all changes to mainboard code reproducibly. I also had to make CB:46769 to ensure things would work properly, and CB:46700 did the actual refactoring.
Another example of how I break down refactoring changes to hopefully ease review: CB:49398 changes behavior but is very small, CB:49399 is not reproducible but doesn't change behavior, and CB:49400 is quite large but reproducible.
There's no need to change this commit, it's just that it reminded me of similar situations I've encountered, and I wanted to describe an approach I came up with. However, if you think this idea might make this change easier to review, feel free to use it. 😊
https://review.coreboot.org/c/flashrom/+/58735/comment/cb90c5df_4fc78b53 PS2, Line 1769: I'd normally complain about unrelated whitespace changes (this seems accidental), but addressing this would mean having to manually rebase the follow-up change as well. If you don't mind, I'd appreciate if you could undo this.
-
Angel Pons (Code Review)