Nico Huber 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/3dc76f6e_24c6be6a PS5, Line 936: if (content->ISL <= 16) : return CHIPSET_5_SERIES_IBEX_PEAK; Ok, your number example is FLMAP2 == 0 and ISL == 80. But your rea- soning seems to assume FLMAP2 != 0, so I'm going to assume FLMAP2 == 1 for this example, maybe that helps. We can also derive ICCRIBA == 0 because this comment thread is on that path. So before this change:
ICCRIBA = 0, FLMAP2 = 1, ISL = 80 if on line 929 => true if on line 930 => false if on line 932 => false if on line 934 => false if on line 936 => false if on line 938 => false warning on line 944 => printed return on line 945 CHIPSET_5_SERIES_IBEX_PEAK
and after this change:
ICCRIBA = 0, FLMAP2 = 1, ISL = 80 if on line 929 => true if on line 930 => false if on line 932 => false if on line 934 => false if on line 936 => false warning on line 942 => printed return on line 943 CHIPSET_5_SERIES_IBEX_PEAK
I see the same side effects and conclude that for these numbers this change does not add assumptions.
With this patch (CB:55651) applied it's unclear if the intent of the default case is to positively detect Ibex Peak, or to simply return Ibex Peak as a reasonable default when nothing else is explicitly detected.
I'm going to assume that what you call "the default case" is the return statement on line 943 (after this change). In line 942, we check `content->ISL != 16` always before the default case. So the information you are missing is still there, it's just not in the current state of the code inside an if statement.
NB. The purpose of the code is to make flashrom work, not to provide a manual for future development. And it's open-source, it can be changed to anything reasonable. It's also possible to change the code to something unreasonable. What happened by accident. This thread here also provides a plausible explanation: A bad rebase on top of this change. But no matter what you pretend, I did not review CB:54965, so I don't know if that is a possibility.