Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35120 )
Change subject: soc/intel/cnl: Add CML IGD IDs ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35120/6/src/soc/intel/cannonlake/bo... File src/soc/intel/cannonlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/35120/6/src/soc/intel/cannonlake/bo... PS6, Line 31: static struct { Pull the struct declarations into a common intel/ header file.
https://review.coreboot.org/c/coreboot/+/35120/6/src/soc/intel/cannonlake/bo... PS6, Line 34: } cpu_table[] = { Not the scope of this patch, but why is report_platform and all these strings defined under bootblock/ diretory?
https://review.coreboot.org/c/coreboot/+/35120/6/src/soc/intel/cannonlake/bo... PS6, Line 152: static uint8_t get_dev_revision(pci_devfn_t dev) Why define this? Just call pci_read_config8() directly.
https://review.coreboot.org/c/coreboot/+/35120/6/src/soc/intel/cannonlake/bo... PS6, Line 157: static uint16_t get_dev_id(pci_devfn_t dev) Same thing.
https://review.coreboot.org/c/coreboot/+/35120/6/src/soc/intel/cannonlake/bo... PS6, Line 274: void report_platform_info(void) As far as I can see there is nothing cannonlake specific in these functions, only the tables would change from one SoC to another. Avoid copy-pasting, declare the table structs in common intel header.