Attention is currently required from: Arthur Heymans, Jonathan Zhang, Johnny Lin, David Hendricks, Christian Walter, Stefan Reinauer, Edward O'Callaghan, Deomid "rojer" Ryabkov, Tim Chu. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 )
Change subject: Revert "Add support for Intel Emmitsburg PCH" ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
The patch was reviewed and tested by multiple parties - you were one of them! Your comments didn't contain any useful guidance for how you'd like to see this implemented, nor did you raise any specific, actionable concerns.
I asked for something. I didn't get an answer. I'm too busy to take on all details of a patch before the basic questions are answered. Usually that turns into wasted time. If some- body else wants to take the responsibility that's ok for me, *if* they act responsibly when something goes wrong. Apologizing and asking for help is also ok. But making wild justifications whilst still not reading the code (see below) is not, IMO.
We went with the approach that aligned with how the code was already implemented
Except that somebody forgot to write consistent code.
if (x <= 80) return;
...x != 16...
Makes no sense, no matter the context, documentation or anything. That you still haven't realized that tells me that the patch shouldn't have been merged. Reverting it would fix that.
and worked when testing on real hardware.
Great works-for-me attitude! And of course if the new hardware works, we can skip the review and don't have to care about older hardware. We can also argue, based on the broken code, that the consistent code wasn't good anyway, ofc.
Let's be serious again. If we can't agree that everything gets properly reviewed *before* it's merged (afterwards nobody will do it, you just proved that), I'm really done with this branch.
-
Nico Huber (Code Review)