Nico Huber has posted comments on this change. ( https://review.coreboot.org/27168 )
Change subject: inteltool: Add PCI IDs for the C220 PCH series ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/#/c/27168/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/27168/1//COMMIT_MSG@9 PS1, Line 9: ICH8 ICH was the predecessor of the PCH. Because some features of the GMCH (northbridge) also moved into the PCH Intel counts the PCH series based on the GMCH numbers. i.e. after 4 series GMCH came 5 series PCH. This is about the 8 series PCHs.
Also, please break lines before 72 characters.
https://review.coreboot.org/#/c/27168/1//COMMIT_MSG@12 PS1, Line 12: Documentation on the PCH can be found here: Please separate paragraphs with an empty line.
https://review.coreboot.org/#/c/27168/1//COMMIT_MSG@13 PS1, Line 13: https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/8-se... No links please. Document and revision numbers are more useful in the long run.
https://review.coreboot.org/#/c/27168/1/util/inteltool/gpio.c File util/inteltool/gpio.c:
https://review.coreboot.org/#/c/27168/1/util/inteltool/gpio.c@866 PS1, Line 866: case PCI_DEVICE_ID_INTEL_H81: According to the datasheet the non-LP version doesn't have the registers above 0x68. Can you confirm if you see (or don't see) valid values above that?
In case, you can still use the same register description, just reduce the `size` below.
https://review.coreboot.org/#/c/27168/1/util/inteltool/inteltool.c File util/inteltool/inteltool.c:
https://review.coreboot.org/#/c/27168/1/util/inteltool/inteltool.c@235 PS1, Line 235: { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_C8_MOBILE, : "Intel(R) C8 Mobile"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_C8_DESKTOP, : "Intel(R) C8 Desktop"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Z87, : "Intel(R) Z87"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Z85, : "Intel(R) Z85"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_HM86, : "Intel(R) HM86"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_H87, : "Intel(R) H87"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_HM87, : "Intel(R) HM87"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q85, : "Intel(R) Q85"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q87, : "Intel(R) Q87"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_QM87, : "Intel(R) QM87"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_B85, : "Intel(R) B85"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_C222, : "Intel(R) C222"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_C224, : "Intel(R) C224"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_C226, : "Intel(R) C226"}, : { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_H81, : "Intel(R) H81"}, Please don't break lines that fit into 80 chars.