Attention is currently required from: Sean Rhodes, Andy Pont, Paul Menzel, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58343 )
Change subject: ec/starlabs: Add standardised ITE EC support ......................................................................
Patch Set 37: Code-Review+1
(15 comments)
File src/ec/starlabs/merlin/Kconfig:
https://review.coreboot.org/c/coreboot/+/58343/comment/00abd36a_50b3e6bf PS37, Line 22: bool "Adjustable Keyboard Backlight" Is this meant to be user-configurable? Or is it mainboard-dependent?
https://review.coreboot.org/c/coreboot/+/58343/comment/d263a2d2_2568d646 PS37, Line 29: bool "Fan device" Same as above.
https://review.coreboot.org/c/coreboot/+/58343/comment/78f96a89_834ae672 PS37, Line 36: Use open-source ec aka Merlin How about `Use open-source Merlin EC firmware` ?
File src/ec/starlabs/merlin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/58343/comment/a4ec9bf3_c103bbd3 PS37, Line 9: $(VARIANT_DIR) Is this using `CONFIG_VARIANT_DIR` from mainboard settings? I'd much prefer to add more Kconfig options so that mainboards can select the right EC type. You can also add a `EC_VARIANT_DIR` option to do this, which would do the same thing but also avoid having to treat Merlin separately (just prioritize its conditional default by putting it before the other defaults).
https://review.coreboot.org/c/coreboot/+/58343/comment/58ad9fcd_928c8870 PS37, Line 27: Mk IV Just this model?
File src/ec/starlabs/merlin/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/310a8527_9476b5a4 PS37, Line 27: 0x64 100
https://review.coreboot.org/c/coreboot/+/58343/comment/f0b876aa_fd296d9e PS37, Line 36: 0x01 I'd use decimal for the indices
File src/ec/starlabs/merlin/acpi/cmos.asl:
PS37: Would be really interesting to generate these definitions from cmos.layout but for now this is good enough.
File src/ec/starlabs/merlin/acpi/hid.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/8af4cf26_113a60c6 PS37, Line 14: Usually, ACPI will check if the OS is 0x07DD (2013 - Windows 8.1ish) And this is why Linux claims to be Windows-compatible in ACPI _OSI queries. No one should be using older Windows versions on these machines anyway, so I think the chosen approach is best.
File src/ec/starlabs/merlin/ec.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/a01d1c51_8aab01b4 PS37, Line 12: binary blob `EC firmware`?
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/c554a22b_ccddb9f1 PS37, Line 79: 5 nit: specifying the size isn't necessary when using an array initializer
https://review.coreboot.org/c/coreboot/+/58343/comment/bc890c70_abeaae1b PS37, Line 201: Off, Low, High Missing `On` ?
https://review.coreboot.org/c/coreboot/+/58343/comment/34e8f56d_e228c4f5 PS37, Line 212: if (CONFIG(EC_STARLABS_KBL_LEVELS)) : ec_write(ECRAM_KBL_BRIGHTNESS, : get_ec_value_from_option("kbl_brightness", : 2, : kbl_brightness, : ARRAY_SIZE(kbl_brightness))); : else : ec_write(ECRAM_KBL_BRIGHTNESS, : get_ec_value_from_option("kbl_brightness", : 0, : kbl_brightness, : ARRAY_SIZE(kbl_brightness))); I'd use two different tables, but I'm fine with the current approach.
File src/ec/starlabs/merlin/variants/apl/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/3218ce84_36cb6fa2 PS37, Line 4: ITE ITE One is enough
https://review.coreboot.org/c/coreboot/+/58343/comment/af056c5b_f2b0674b PS37, Line 7: _EC_STARLABS_ITE_CFG_H I'd avoid having the same guard name on multiple files so that there's a build-time error if conflicting headers are included (e.g. both apl and merlin headers are included). How about `EC_STARLABS_APL_EC_DEFS_H` ? Same logic for the other headers.