Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33102 )
Change subject: ec/apple: Add ACPI code for Apple MacBooks ......................................................................
Patch Set 17: Code-Review+1
(11 comments)
https://review.coreboot.org/c/coreboot/+/33102/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33102/13//COMMIT_MSG@10 PS13, Line 10: it's
Please use possesive "its" […]
Done
https://review.coreboot.org/c/coreboot/+/33102/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33102/16//COMMIT_MSG@12 PS16, Line 12: Old code is rewritten with new ASL syntax
Use the same tense as the other statements: […]
Done
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/ac.asl File src/ec/apple/acpi/ac.asl:
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/ac.asl@1... PS17, Line 16: Just to be safe:
#ifndef HPAC_OFFSET #error (i don't recall the syntax) #endif
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/ac.asl@2... PS17, Line 22: , 1, If this is LIDS, I'd say using different macro names isn't worth the hassle.
Also, I don't know why the original code is split over so many files, it makes keeping some things in sync harder
https://review.coreboot.org/c/coreboot/+/33102/16/src/ec/apple/acpi/ac_01.as... File src/ec/apple/acpi/ac_01.asl:
https://review.coreboot.org/c/coreboot/+/33102/16/src/ec/apple/acpi/ac_01.as... PS16, Line 21: 0x01
Already done, but will upload together with all affected boards.
Done
https://review.coreboot.org/c/coreboot/+/33102/16/src/ec/apple/acpi/ac_60.as... File src/ec/apple/acpi/ac_60.asl:
https://review.coreboot.org/c/coreboot/+/33102/16/src/ec/apple/acpi/ac_60.as... PS16, Line 22: , 1,
Yes
Ack
https://review.coreboot.org/c/coreboot/+/33102/5/src/ec/apple/acpi/battery.a... File src/ec/apple/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/33102/5/src/ec/apple/acpi/battery.a... PS5, Line 147: Local0++ & 0xFFFF
I have doubts about this, is this correct? It was `And(Increment(Local0), 0xFFFF, Local0)`
It's basically doing the two's complement of Local0. This means that the addition must be performed before the AND
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/battery.... File src/ec/apple/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/battery.... PS17, Line 80: 0xFFFFFFFF, // 1: Design cap These two aren't indented with tabs
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/battery.... PS17, Line 132: What's with these spaces?
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/ec.asl File src/ec/apple/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/ec.asl@4... PS17, Line 43: 0x62 Why was this changed to 0x62 ?
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/lid.asl File src/ec/apple/acpi/lid.asl:
https://review.coreboot.org/c/coreboot/+/33102/17/src/ec/apple/acpi/lid.asl@... PS17, Line 16: Just to be safe:
#ifndef LIDS_OFFSET #error (i don't recall the syntax) #endif #ifndef WKLD_OFFSET #error (i don't recall the syntax) #endif