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 122:
(14 comments)
I tried to give it a quick look, but the first files (ACPI) are incredibly hard to read. Didn't come very far.
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 11: Name (ACST, One) Where is this consumed? Why is it needed to cache this?
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 14: // _HID: Hardware ID Automated comments make it look like you borrowed the code from somebody else.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 19: If (EACS) : { : ACST = One : } : Else : { : ACST = Zero : } Isn't it easier to just write
ACST = EACS
? I would expect it to compile.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 31: Method (_STA, 0, NotSerialized) // _STA: Status : { : Return (0x0F) : } This is the default, are you sure it's needed?
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 11: EB0A, 1, : , 2, : EB0R, 1, : EB0L, 1, : EB0F, 1, : EB0N, 1 These seem to describe the bits in `EB0S` below. Why not use them instead of the manual masking? If it's to reduce the number of EC RAM reads, defined macro names for the masks would be nice.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 41: Offset (0xE0), Same offset as above. So the EC provides different data on the same addresses in a predefined sequence, I assume? Can we have a comment what the expected sequence is?
As far as I can see:
EBDC EBFC EBDV /* new value after reading EBFC */ EBSN EBDN /* entire new value */ EBCH /* entire new value */ EBMN /* entire new value */
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 69: Arg1 [One] = 0xFFFFFFFF : Arg1 [0x02] = 0xFFFFFFFF : Arg1 [0x04] = 0xFFFFFFFF : Arg1 [0x05] = Zero : Arg1 [0x06] = Zero Again, looks like borrowed code. Wouldn't
Arg1[1] = ... Arg1[2] = ... ... Arg1[6] = ...
be easier to read?
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 78: (Local0 ^ One) isn't this just `!Local0`?
Are the parentheses needed?
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 78: Zero 0?
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 : } Can this be written with one `if` block?
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 100: 0x64 % calculations are easier to follow if you use decimal numbers.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 100: Divide (Local2, 0x64, Local7, Local6) // Warning capacity Why Divide() instead of `Local6 = Local2 / 100`? Would it hurt to write
Arg1[5] = (Local2 / 100) * 7 Arg1[6] = ((Local2 / 100) * 11) / 2
?
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 102: Arg1 [0x05] = Local3 /* 7% */
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 107: Arg1 [0x06] = Local4 /* 5.5% */