Attention is currently required from: Subrata Banik, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63584 )
Change subject: ichspi: Add support for Ice Lake ......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/63584/comment/afd2d940_23a77e9a PS1, Line 7: ichspi: Add support for Ice Lake
Not sure what you mean by "undone" as those add legitimate new chipset support, seems like hyperbole.
I mean the enum additions should be undone. Not the chipset support that could have been added with much less changes.
I believe you mean to say there should be some refactors happening to help consolidate so many enum branches. As you pointed out Nico, you tried a number of times in the past and didn't manage to trivially do it.
Refactoring it might be difficult. Not adding unnecessary clutter is an orthogonal problem.
In any case, I actually did upload a start towards making progress in that direction in CB:63321 but it has been sitting unreviewed for a few weeks at the moment. I believe the theme of that patch can be applied to a few other places towards a direction of having most of the chipset static details in a lookup table and a switch.
More careful review of SPI Programmer Guides between generations is required to find better common patterns.
Well, let me know if you found something that definitely makes things better. In the meantime, please avoid unnecessary enum additions.
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/63584/comment/e3617614_c50169ff PS1, Line 2149: 0x34a4
Subrata, flashrom can be used in "odd" situations when folks are porting coreboot to boards in the community. I think this is what Nico is alluding to here as to make flashrom as robust as possible to allow for flashing.
It's actually much simpler. We can avoid any regression by not removing the valid LPC ID. Nothing about robustness. Just in case that 0x34a4 is actually hidden on a system where flashrom already works, we wouldn't break it.
Should DID's in the table used for detection of hidden situations perhaps have " (hidden)" suffix and a more detailed code comment explaining so we can avoid confusion? Feel like this is it's own patch.
As mentioned elsewhere, calling the function that deals with hidden devices when there are no hidden devices can simply be avoided. Then the code would be self-documenting.