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 124:
(6 comments)
Usually, I still feel confident about my signed-off-by when I copied a few lines (if it's merely boilerplate for instance). But what you have done here seems bold. Don't know if too bold ;)
Are you thinking of code quality issues or license-related problems? Because for one, it works, and for the other, isn't the recommended way to get started porting a board to copy configuration that works, etc?
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/ac.asl:
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 31: Method (_STA, 0, NotSerialized) // _STA: Status : { : Return (0x0F) : }
Not the method but its value is implicit. Spec says: […]
Ack
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 79: If (Local0) : { : Local1 = (EBDC * 0x0A) : } : Else : { : Local1 = EBDC : } : : Arg1 [One] = Local1 // Design capacity : If (Local0) : { : Local2 = (EBFC * 0x0A) : } : Else : { : Local2 = EBFC : }
Yes, I guess the order of EC accesses might matter, but we can keep that […]
That did seem logical. Tested, working.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 102: Arg1 [0x05] = Local3
I don't follow. `7%` is just a different way of saying `* 7 / 100` or […]
I suppose this can be hard to read. Beyond the 'multiply by 10' (maybe an EC model or firmware quirk?), I was also finding "*Last* full charge capacity" mildly confusing, but batteries do degrade.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 107: Arg1 [0x06] = Local4
/* 5. […]
Done
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 283: (Local1 & 0x08)
`EB0R` as I read it? […]
I'm not sure about endianness for the 8051 and if it matters to ACPI.
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... PS119, Line 29: // #include "acpi/optimus_mb.asl"
Optimus support depends upon CB:28380 and requires implementation per-SoC. […]
Optimus support has been removed. I'll work on this separately.