Attention is currently required from: Paul Menzel, Angel Pons. Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55263 )
Change subject: ec: Add Star Labs ITE 5570E support ......................................................................
Patch Set 9:
(10 comments)
File src/ec/starlabs/it5570/acpi/ac.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/c7e4ab73_28972bb6 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?
Yup, you after a comment to say what or just interested?
File src/ec/starlabs/it5570/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/0a8ea302_24f9de1e PS4, Line 11: If(And(ECRD(RefOf(ECPS)), 0x02))
How about: […]
Done
File src/ec/starlabs/it5570/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/46e7b58a_294cf2f1 PS4, Line 28: Name(LIDS, 0)
there should already be a LIDS in GNVS
Done
https://review.coreboot.org/c/coreboot/+/55263/comment/0296868f_58664355 PS4, Line 45: // Store (0x03, _SB.PCI0.GFX0.CLID)
Huh? _STA shouldn't have side-effects
Are you saying that about the `Return (0x0F)`?
https://review.coreboot.org/c/coreboot/+/55263/comment/81dd86a9_1b666348 PS4, Line 113: Store(DerefOf (Arg0), Local1)
How about: […]
Done
File src/ec/starlabs/it5570/acpi/hid.asl:
PS4:
Is this specific to the IT5570E EC?
It's different to three others - that's all I know.
File src/ec/starlabs/it5570/acpi/lid.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/3bbcb7d2_dc5ef858 PS4, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */
nit: add a blank line after the license header
Done
File src/ec/starlabs/it5570/acpi/thermal.asl:
https://review.coreboot.org/c/coreboot/+/55263/comment/3d56dca2_dcc10aa3 PS4, Line 5: Notify (_TZ.TZ01, 0x80
Missing closing parenthesis, won't build
Done
File src/ec/starlabs/it5570/ec.h:
https://review.coreboot.org/c/coreboot/+/55263/comment/0c73d727_2c03e9a7 PS4, Line 14: #define IT5570E_FIXED_ADDR 0x4e
Seems to be board-specific?
I'm pretty sure it's EC specific, with 89xx and 57xx using 0x4e and 87xx using 0x2e.
File src/ec/starlabs/it5570/ec.c:
https://review.coreboot.org/c/coreboot/+/55263/comment/5bbb5975_c7c5f6b0 PS4, Line 72: { NULL, 0, 0, 0, }
If you have a datasheet, it would be great to describe the resource consumption of the logical devic […]
Ack
-
Sean Rhodes (Code Review)