Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31448 )
Change subject: src/device/pci: Add support for discrete VGA initialization and OpROM loading ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/#/c/31448/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31448/12//COMMIT_MSG@15 PS12, Line 15: Later will try to make a common VFCT table for both adapters. I'd focus on that. This patch just seems like a hack...
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@795 PS10, Line 795: MULTIPLE_VGA_ADAPTERS
Almost: e.g. […]
It seems like a better idea to get that working instead of this hack... Maybe look at the vendor ACPI code to get a clue of how they do it.
https://review.coreboot.org/#/c/31448/10/src/device/pci_device.c@830 PS10, Line 830: Integrated
Ternary operator done. […]
That is poor reasoning. It's not because on your board one PCI device is VGA compatible and the other not that you can generalize that to discrete/integrated. PCI classes mean exactly what they mean and coreboot code should not invent a new meaning for it.
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c File src/device/pci_rom.c:
https://review.coreboot.org/#/c/31448/10/src/device/pci_rom.c@230 PS10, Line 230: if (!CONFIG(MULTIPLE_VGA_ADAPTERS)) {
Currently G505S is the only device using this MULTIPLE_VGA_ADAPTERS config, and if I remember correc […]
No board Kconfigs in here please. What I meant is that it is printing a message depending on a Kconfig while this code get's passed a struct device *. That makes no sense.