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 7: Code-Review+1
(7 comments)
Muuuch better!
https://review.coreboot.org/c/coreboot/+/43612/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43612/2//COMMIT_MSG@11 PS2, Line 11: Tested on system76/lemp9. An associated patch will be sent that converts : system76/lemp9 over to EC_SYSTEM76_EC.
The mainboard change is here: https://review.coreboot. […]
Ah yes, the diffstat has definitely improved 😄
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 I'd use all-y
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) { nit: including these files at such a high scope is probably the reason why you need so many ^^^^ in ACPI
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 I'd keep preprocessor unindented
https://review.coreboot.org/c/coreboot/+/43612/7/src/ec/system76/ec/acpi/s76... PS7, Line 67: #if EC_COLOR_KEYBOARD I'd make this a Kconfig option
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'd use the timer API in <timer.h>
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 Are these changes part of the EC code?