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 123:
(5 comments)
@Nico, were you considering disassembled ASL as "copied code?"
Yes. ASL and AML are merely different representations of the same language. What `iasl` does is more translating than compiling. So, IMO, it's still the very same code after `iasl` "disassembling". 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 ;)
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 14: // _HID: Hardware ID
They were added by iASL's disassembler. […]
Ack
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 31: Method (_STA, 0, NotSerialized) // _STA: Status : { : Return (0x0F) : }
I don't understand. […]
Not the method but its value is implicit. Spec says:
"If a device object describes a device that is not on an enumerable bus and the device object does not have an _STA object, then OSPM assumes that the device is present, enabled, shown in the UI, and functioning."
"present, enabled, shown in the UI, and functioning" is 0xf ;)
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 : }
EBDC and EBFC are in different memory fields and this method is serialized. I'll give it a try.
Yes, I guess the order of EC accesses might matter, but we can keep that order and make it a little more readable:
If (Local0) { Local1 = ... Local2 = ... } Else { Local1 = EBDC Local2 = EBFC }
(If it were C I would just introduce another variable `factor = local0 ? 10 : 1;`. But that's not fun with the `LocalX` ASL variables.)
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 102: Arg1 [0x05] = Local3
Capacity of warning, I assume, but how do you know that? We don't know in which state bits may be, t […]
I don't follow. `7%` is just a different way of saying `* 7 / 100` or `/ 100 * 7` which is what the code does. We don't know the bits, but we know that we report `Local2` as full charge, `Local3 = 7% of Local2` here and `Local4 = 5.5% of Local2` below.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 283: (Local1 & 0x08)
`EB0L`?
`EB0R` as I read it?
EB0A, 1, /* bit 0 */ , 2, /* bits 1, 2 */ EB0R, 1, /* bit 3 */ EB0L, 1, /* bit 4 */