Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35523 )
Change subject: mb/acer: Add Acer Aspire VN7-572G ......................................................................
Patch Set 63:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/acpi/mbdefines.asl:
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 29: 0xFE800000
FIXME: This defines a hardcoded memory region.
Given the high location, this will be some hardware MMIO. Which may be chosen by the firmware.
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 34: 0x7CE7D018
That does sound very bad. How should this be handled instead?
Depends on what the code tries to do, and to be honest, reading it seems like a reverse engineering effort. ACPI features are best added step by step, e.g. you miss battery status, add that, you want an Fn-key to work, add that etc. Copying vendor code (which I assume this is) is a sure way to waste time (unless one doesn't care and accepts to have 80% dead, unreadable code).