Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43612 )
Change subject: ec/system76_ec: Add support for System76 EC ......................................................................
Patch Set 8:
(9 comments)
https://review.coreboot.org/c/coreboot/+/43612/8/src/ec/system76/ec/Kconfig File src/ec/system76/ec/Kconfig:
https://review.coreboot.org/c/coreboot/+/43612/8/src/ec/system76/ec/Kconfig@... PS8, Line 7: bool This would need to "depends on EC_SYSTEM76_EC"
https://review.coreboot.org/c/coreboot/+/43612/8/src/ec/system76/ec/Kconfig@... PS8, Line 8: default n indent should use tabs
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/Makefile... File src/ec/system76/ec/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/Makefile... PS7, Line 3: bootblock-y += system76_ec.c
Ah, didn't know about that, thanks
Done. I think `all` also includes SMM, but not a big deal. In any case, the unreferenced stuff should get culled by the compiler's optimizing magic.
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/acpi/ec.... File src/ec/system76/ec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/acpi/ec.... PS7, Line 3: Scope (_SB) {
True. […]
Yes, I think it would be easier to handle it in a separate change.
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/acpi/s76... File src/ec/system76/ec/acpi/s76.asl:
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/acpi/s76... PS7, Line 17: #if EC_COLOR_KEYBOARD
Ack
Done
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/acpi/s76... PS7, Line 67: #if EC_COLOR_KEYBOARD
Ack
Done
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/system76... File src/ec/system76/ec/system76_ec.c:
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/system76... PS7, Line 33: for (timeout = 10000; timeout > 0; timeout--) {
I'll take a look
Oh, nice one. There's `wait_ms` if you want to use that
https://review.coreboot.org/c/coreboot/+/43612/8/src/ec/system76/ec/system76... File src/ec/system76/ec/system76_ec.c:
https://review.coreboot.org/c/coreboot/+/43612/8/src/ec/system76/ec/system76... PS8, Line 32: wait_us(10000, system76_ec_read(0) == 0); Tabs, tabs, tabs 😋
https://review.coreboot.org/c/coreboot/+/43612/7/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/43612/7/src/mainboard/system76/lemp... PS7, Line 149: # LPC configuration from lspci -s 1f.0 -xxx
Yes. Without them the regions used by the EC will not be correctly set. […]
Ack