Attention is currently required from: Nico Huber, Subrata Banik, Angel Pons, Anastasia Klimchuk.
3 comments:
Commit Message:
Patch Set #1, Line 7: ichspi: Add support for Ice Lake
As discussed during review of the first of these patches, adding all […]
Not sure what you mean by "undone" as those add legitimate new chipset support, seems like hyperbole. 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.
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.
File chipset_enable.c:
if (strcmp(name, "Ice Lake") == 0)
return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_400_SERIES_ICE_POINT);
else
return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_400_SERIES_COMET_POINT);
This feels wrong to have a strcmp(). I do not have a copy of IceLake/IcePoint SPI Programming Guide to figure out if there is any difference, if you have a copy Subrata can we look together and figure out a strategy?
If this patch is right to say there is no difference then can we rename `CHIPSET_400_SERIES_COMET_POINT` to `CHIPSET_400_SERIES_COMET_ICE_POINT` Maybe?
Patch Set #1, Line 2149: 0x34a4
But can it be hidden? Maybe not everybody have updated their code […]
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.
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.
To view, visit change 63584. To unsubscribe, or for help writing mail filters, visit settings.