Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31506 )
Change subject: inteltool: Add Sunrise Point-LP Skylake PCH IDs ......................................................................
Patch Set 10:
(3 comments)
Do you plan to add the ids to each .c individually? Maybe you can add them in this patch to all cases where nothing changes (probably everything but GPIO)?
https://review.coreboot.org/#/c/31506/9/util/inteltool/inteltool.h File util/inteltool/inteltool.h:
https://review.coreboot.org/#/c/31506/9/util/inteltool/inteltool.h@147 PS9, Line 147: #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_PRE 0x9d41 moving below _LP_SATA would keep numerical order and PCH ids together
https://review.coreboot.org/#/c/31506/9/util/inteltool/inteltool.c File util/inteltool/inteltool.c:
https://review.coreboot.org/#/c/31506/9/util/inteltool/inteltool.c@228 PS9, Line 228: Mobile "Mobile" could also mean PCH-H, just use "Sunrise Point-LP" like below?
https://review.coreboot.org/#/c/31506/9/util/inteltool/inteltool.c@229 PS9, Line 229: { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_BASE_SKL, : "Sunrise Point-LP U Base" }, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_Y_PREM_SKL, : "Sunrise Point-LP Y Premium" }, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_PREM_SKL, : "Sunrise Point-LP U Premium" }, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_BASE_KBL, : "Sunrise Point-LP U Base" }, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_Y_PREM_KBL, : "Sunrise Point-LP Y Premium" }, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_PREM_KBL, : "Sunrise Point-LP U Premium" }, please mention Skylake/Kaby Lake in the strings, too