Attention is currently required from: Sean Rhodes, Paul Menzel. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55263 )
Change subject: ec: Add Star Labs ITE 5570E support ......................................................................
Patch Set 4:
(10 comments)
File src/ec/starlabs/it5570/acpi/ac.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/df71739d_20442a4b PS4, Line 27: Method (_Q0A, 0, NotSerialized) : { : Notify(BAT0, 0x81) : Notify(ADP1, 0x80) : } : : Method (_Q0B, 0, NotSerialized) : { : Notify(BAT0, 0x81) : Notify(BAT0, 0x80) : } Is the meaning of these two queries known?
File src/ec/starlabs/it5570/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/84001e54_0d54004e PS4, Line 11: If(And(ECRD(RefOf(ECPS)), 0x02)) How about:
If (ECRD(RefOf(ECPS)) & 0x02)
File src/ec/starlabs/it5570/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/49881316_b5a1fd35 PS4, Line 28: Name(LIDS, 0) there should already be a LIDS in GNVS
https://review.coreboot.org/c/coreboot/+/55263/comment/9e142f65_ea96a7d6 PS4, Line 45: // Store (0x03, _SB.PCI0.GFX0.CLID) Huh? _STA shouldn't have side-effects
https://review.coreboot.org/c/coreboot/+/55263/comment/82993b09_7720e8ee PS4, Line 113: Store(DerefOf (Arg0), Local1) How about:
Local1 = DerefOf (Arg0)
File src/ec/starlabs/it5570/acpi/hid.asl:
PS4: Is this specific to the IT5570E EC?
File src/ec/starlabs/it5570/acpi/lid.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/ef89fdf7_3853e8a9 PS4, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ nit: add a blank line after the license header
File src/ec/starlabs/it5570/acpi/thermal.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/c5e4b5ff_e4bbd312 PS4, Line 5: Notify (_TZ.TZ01, 0x80 Missing closing parenthesis, won't build
File src/ec/starlabs/it5570/ec.h:
https://review.coreboot.org/c/coreboot/+/55263/comment/8902ee26_5781314c PS4, Line 14: #define IT5570E_FIXED_ADDR 0x4e Seems to be board-specific?
File src/ec/starlabs/it5570/ec.c:
https://review.coreboot.org/c/coreboot/+/55263/comment/0ca80487_779b1ac9 PS4, Line 72: { NULL, 0, 0, 0, } If you have a datasheet, it would be great to describe the resource consumption of the logical devices here.
-
Angel Pons (Code Review)