Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30989 )
Change subject: intelblocks/systemagent: Add ACPI table generation hook ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/30989/6/src/soc/intel/common/block/systemage... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/#/c/30989/6/src/soc/intel/common/block/systemage... PS6, Line 58: }
Well, this approach just fits in current infrastructure we have around.
You mean your change? That's fine. I just commented on this driver generally, not meant as incentive to adapt your change.
So your idea would be to define a second driver just on SoC level which just covers the PCI IDs of that single SoC? And then duplicate it for the SoC even if we have this driver around already which will match the given IDs?
No, I wouldn't place a pci_driver into common/ code in the first place (unless it is 100% common). With the current weak-function approach, you have to jump back and forth between soc and common code if you want to read it. It would be much easier if we would keep the driver in the soc code and don't jump back.
With the abstraction mechanism (function pointers) of pci_driver, we could already solve such problems better before we made excessive use of weak functions. Now this code is layering abstraction mechanisms, making it harder to maintain.