Attention is currently required from: Nico Huber. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55651 )
Change subject: ich_descriptors: Normalize chipset detection ......................................................................
Patch Set 5:
(1 comment)
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/55651/comment/473eb2b3_5f45bdb3 PS5, Line 936: if (content->ISL <= 16) : return CHIPSET_5_SERIES_IBEX_PEAK;
For instance, when I wrote this patch, I would not have guessed that a server platform will land on this ICCRIBA == 0 branch. Trying to predict such details can lead to walking in circles patch wise.
I'm sure you did what seemed right with the information that you had at the time. Intel threw a curvaball at us with this.
For instance, I assume that it's easier to follow the code when it's more condensed, like after this patch. Before the entire patch train it was a forest of if's and nobody knew where to put anything. We should avoid that this happens again.
Yeah, flashrom tends to accumulate cruft as new similar-but-slightly-different hardware gets added. I'm glad you refactored this, and FWIW the original EBG patch (CB:54965) definitely looked better after rebasing.
Only one thing I have to ask you: As the purpose of this patch was to normalize, please do so with explicit if's for all chipsets then. warn_peculiar_desc() probably shouldn't have to check anything then.
Done: CB:57793
-
David Hendricks (Code Review)