Nico Huber 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 14:
(1 comment)
Ideally, this would be three patches: one that adds the EC code, one that hooks it up to the console and one that hooks it up to the mainboard.
About the console output, where does it go? If it ends up on a UART for instance, it would be better to use the existing functions instead of adding yet another console interface.
https://review.coreboot.org/c/coreboot/+/43612/14/src/ec/system76/ec/system7... File src/ec/system76/ec/system76_ec.c:
https://review.coreboot.org/c/coreboot/+/43612/14/src/ec/system76/ec/system7... PS14, Line 40: // Read length Just a random thought: if you'd use defined names for the registers, it would be more readable and you could drop all these code-rephrasing comments.