Attention is currently required from: Andy Pont, Paul Menzel, Angel Pons, Felix Held. Sean Rhodes 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:
(9 comments)
File src/ec/starlabs/merlin/Kconfig:
https://review.coreboot.org/c/coreboot/+/58343/comment/7d9ad14a_e4ad93fb PS37, Line 22: bool "Adjustable Keyboard Backlight"
Is this meant to be user-configurable? Or is it mainboard-dependent?
Mainboard dependent. Updated help text, is that alright?
https://review.coreboot.org/c/coreboot/+/58343/comment/19dc22ec_d4fc4c1a PS37, Line 36: Use open-source ec aka Merlin
How about `Use open-source Merlin EC firmware` ?
Done
File src/ec/starlabs/merlin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/58343/comment/9e00b0c8_ddf56400 PS37, Line 9: $(VARIANT_DIR)
Is this using `CONFIG_VARIANT_DIR` from mainboard settings? I'd much prefer to add more Kconfig opti […]
Like so?
https://review.coreboot.org/c/coreboot/+/58343/comment/9fb0831d_d8c09628 PS37, Line 27: Mk IV
Just this model?
Done
File src/ec/starlabs/merlin/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/50710efc_b6e90942 PS37, Line 27: 0x64
100
Done
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/c84e6ea0_f7249094 PS19, Line 134: #if CONFIG(EC_STARLABS_FAN) : switch (get_uint_option("fan_mode", 0)) { : case FAN_AGGRESSIVE: : ec_write(ECRAM_FAN_MODE, FAN_AGGRESSIVE); : break; : case FAN_QUIET: : ec_write(ECRAM_FAN_MODE, FAN_QUIET); : break; : default: : ec_write(ECRAM_FAN_MODE, FAN_NORMAL); : break; : } : #endif
Done
Just realised why I didn't before - APL/GLK/GLK-R don't have ECRAM_FAN_MODE defined so won't build with C. I could just define it as `0x00`?
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/3c452a64_8ff74eaa PS37, Line 79: 5
nit: specifying the size isn't necessary when using an array initializer
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/ebae76fe_2980f284 PS37, Line 201: Off, Low, High
Missing `On` ?
Done
File src/ec/starlabs/merlin/variants/apl/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/4169051f_64a968d2 PS37, Line 4: ITE ITE
One is enough
Done