Attention is currently required from: Nico Huber, Eugene Myers, Benjamin Doron, Paul Menzel, Angel Pons, Michael Niewöhner.
Patch set 190:Code-Review +1
5 comments:
Patchset:
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:
Patch Set #190, 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:
Patch Set #190, 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.
Patch Set #190, 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:
Patch Set #190, Line 113: Still WIP.
Maybe document what still needs to be done?
To view, visit change 35523. To unsubscribe, or for help writing mail filters, visit settings.