Jeremy Soller 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:
(6 comments)
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
Ah, didn't know about that, thanks
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 […]
True. Perhaps something I can address in another change, since the old lemp9 ACPI code did this
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
Ack
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
Ack
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. […]
I'll take a look
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?
Yes. Without them the regions used by the EC will not be correctly set.
0xE00 for example is used for the console code above