Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35523 )
Change subject: mb/acer: Add Acer Aspire VN7-572G ......................................................................
Patch Set 146:
(10 comments)
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 126: Divide
Ack. […]
It would make sense, but I haven't seen remainders mentioned very much.
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 135: Arg1
any idea of what each bit means?
`EB0S`? Only by context, if possible at all.
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 261: Local0 = EB0A
This was using EB0S before, was it wrong?
No, but I was reducing use of `EB0S` (EB0S & 1 = EB0A). Then I changed `Local1` throughout the function to `Local0`.
What I don't understand is "EB0A & 0x40" on the next line, but it doesn't seem to have caused any problems.
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 251: // _Qxx: EC Query, xx=0x00-0xFF
I'd drop these comments for known (or just all) queries
Done
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/cmos.layout:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 7: #0 112 r 0 lower_reserved
This one wasn't commented out: […]
So, this is something that I don't understand. Must cmos_layout be defined at each offset, or only the bits used? On the flipside, is there a concern that if reserved bits are defined, coreboot or the user may attempt to write to them?
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 8: #112 264 r 0 unused
These `unused` commented-out entries are just extra maintenance burden
As above.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 89: register "PmTimerDisabled" = "0"
It defaults to zero already
Done
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/gpio.h:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 259: PAD_CFG_GPIO_BIDIRECT(GPP_E22, 0, NONE, DEEP, LEVEL, ACPI),
Nico commented on it: […]
The vendor firmware has a configuration option called "bidirectional PROCHOT#," which defaults to enabled. However, according to the schematics, GPP_E22 is not connected and no GPP is connected to PROCHOT#.
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/35523/144/src/mainboard/acer/aspire... PS144, Line 70: 0x80860101
Acer uses a different subsystem ID. […]
Apparently not for HDMI audio, which might make sense if they didn't make any modifications.
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/romstage.c:
https://review.coreboot.org/c/coreboot/+/35523/145/src/mainboard/acer/aspire... PS145, Line 34: TRUE
true (in lowercase)
Very well. Done.