Attention is currently required from: Nico Huber, Eugene Myers, Benjamin Doron, Paul Menzel, Angel Pons, Michael Niewöhner. Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35523 )
Change subject: mb/acer: Add Acer Aspire VN7-572G ......................................................................
Patch Set 190: Code-Review+1
(5 comments)
Patchset:
PS190: I gave this a cursory review and didn't see anything that I thought should particularly keep it from going in, especially since it's all contained in the single mainboard directory.
It looks as if this is part of an ongoing board bringup, so I'd expect that any issues would be fixed along the way.
I'd like to see the patch rebased on top of the current coreboot codebase instead of a patch that's several months old before it's submitted. It merges cleanly, but until it's rebased, we can't tell if there's something that would keep it from building and breaking the coreboot build.
Please address all outstanding comments, even if it's just to say that you'll fix things in follow-on patches (maybe add TODOs in the code). After that's done, I don't see any reason we can't merge this.
File src/mainboard/acer/Kconfig:
https://review.coreboot.org/c/coreboot/+/35523/comment/d3c8f396_80495988 PS190, Line 15: string This should probably go into a separate commit since it's not actually related to this patch. Typically I wouldn't care, but the rest of the patch is big enough, and this removal makes no functional difference.
File src/mainboard/acer/aspire_vn7_572g/mainboard.c:
https://review.coreboot.org/c/coreboot/+/35523/comment/a336bf54_a932f69e PS190, Line 34: 0x10251037 Nit: use #defines: src/include/device/pci_ids.h PCI_VENDOR_ID_AI
You'd have to add 1037 as HDA. I'm fine with this being done in a follow-on patch.
https://review.coreboot.org/c/coreboot/+/35523/comment/c36812f8_032414f1 PS190, Line 110: ec_init(void) Lots of magic numbers in this function that could be turned into #defines.
File src/mainboard/acer/aspire_vn7_572g/smihandler.c:
https://review.coreboot.org/c/coreboot/+/35523/comment/1fda8839_80423ae9 PS190, Line 113: Still WIP. Maybe document what still needs to be done?