Attention is currently required from: Paul Menzel, Angel Pons. 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 19:
(25 comments)
File src/ec/starlabs/merlin/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/a82dd54e_4714491d PS19, Line 41: 0x0a
10
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/6073ae6e_d2fbfb65 PS19, Line 42: 0x64
100
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/68d826d7_00f1b4b8 PS19, Line 43: 0x64
100
Done
File src/ec/starlabs/merlin/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/364f10ae_d6e5e192 PS19, Line 67: Embedded
nit: `Embedded Controller`?
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/5f3154c2_f859e84b PS19, Line 104: // ECWT (Embedded Write Method)
What exactly does this method do? Is it used anywhere?
I'll stop being a horder.
https://review.coreboot.org/c/coreboot/+/58343/comment/fd3c6ff1_a8ccc6ad PS19, Line 135: Embedded
nit: `Embedded Controller`?
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/08c8182e_fb731e80 PS19, Line 170: Method (ECMD, 0, Serialized) : { : Return (0x00) : } : : Method (ECM1, 0, Serialized) : { : Return (0x00) : } : : Method (ECNT, 0, Serialized) : { : Return (0x00) : }
Are these methods needed?
Done
File src/ec/starlabs/merlin/acpi/hid.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/64ced04c_81b60e88 PS19, Line 109: 0xFA
250
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/01b46492_837ffa0b PS19, Line 111: 0x04
4
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/cc6f8c86_dc57b959 PS19, Line 244: Its
It's
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/1929550d_fff434bc PS19, Line 248: Its
It's
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/ece19da1_087229ca PS19, Line 255: Its
It's
Done
File src/ec/starlabs/merlin/acpi/keyboard.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/9e23e954_0f590802 PS19, Line 23: _Q80
Copy-paste error
Done
File src/ec/starlabs/merlin/acpi/thermal.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/697a4856_f63672d0 PS19, Line 5: Notify (_TZ.TZ01, 0x80
Syntax error. Looks like the file is unused, I'd remove it.
Done
File src/ec/starlabs/merlin/acpi/typec.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/5c737e5d_cbae64f3 PS19, Line 5: 0x32
50
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/c34705bf_61787882 PS19, Line 35: 0x32
50
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/19d93dab_9229fc3e PS19, Line 60: 0x32
50
Done
File src/ec/starlabs/merlin/ec.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/52bb404b_468e2e50 PS19, Line 41: /* Keyboard Backlight Timeout */ : #define SEC_30 0x00 : #define MIN_1 0x01 : #define MIN_3 0x02 : #define MIN_5 0x03 : #define NEVER 0x04 : : /* Fn Ctrl Swap */ : #define FN_CTRL 0x00 : #define CTRL_FN 0x01 : : /* Max Charge Setting */ : #define CHARGE_100 0x00 : #define CHARGE_80 0xbb : #define CHARGE_60 0xaa : : /* Fan Mode Setting */ : #define FAN_NORMAL 0x00 : #define FAN_AGGRESSIVE 0xbb : #define FAN_QUIET 0xaa : : /* Fn Lock State */ : #define UNLOCKED 0x00 : #define LOCKED 0x01 : : /* Trackpad State */ : #define TRACKPAD_ENABLED 0x00 : #define TRACKPAD_DISABLED 0x22 : : /* Keyboard Brightness Levels */ : #define KBL_ON 0xdd : #define KBL_OFF 0xcc : #define KBL_LOW 0xbb : #define KBL_HIGH 0xaa : : /* Keyboard Backlight State */ : #define KBL_DISABLED 0x00 : #define KBL_ENABLED 0xaa
I'd use actual enums for these instead of macros.
Done
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/4fca8885_2176c394 PS19, Line 68: 0
I'd use a named value here
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/3bbaf25b_3a48f59b PS19, Line 81: default:
nit: for completeness, I'd explicitly handle `SEC_30`: […]
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/604b9652_b4d389fa PS19, Line 59: /* : * Keyboard Backlight Timeout : * : * Setting: kbl_timeout : * : * Values: 30 Seconds, 1 Minute, 3 Minutes, 5 Minutes, Never : * Default: 30 Seconds : * : */ : switch (get_uint_option("kbl_timeout", 0)) { : case MIN_1: : ec_write(ECRAM_KBL_TIMEOUT, MIN_1); : break; : case MIN_3: : ec_write(ECRAM_KBL_TIMEOUT, MIN_3); : break; : case MIN_5: : ec_write(ECRAM_KBL_TIMEOUT, MIN_5); : break; : case NEVER: : ec_write(ECRAM_KBL_TIMEOUT, NEVER); : break; : default: : ec_write(ECRAM_KBL_TIMEOUT, SEC_30); : break; : }
Idea: Encapsulate these into helper functions?
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/0d573910_e47b6279 PS19, Line 113: switch (get_uint_option("max_charge", 0)) { : case CHARGE_80: : ec_write(ECRAM_MAX_CHARGE, CHARGE_80); : break; : case CHARGE_60: : ec_write(ECRAM_MAX_CHARGE, CHARGE_60); : break; : default: : ec_write(ECRAM_MAX_CHARGE, CHARGE_100); : break; : }
cmos.layout of CB:56088 won't work with this, the default case will always be taken. […]
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/74c45d63_ee8feccf 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
Why not use a regular C check?
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/eccf5575_d3daf6d7 PS19, Line 193: #if CONFIG(EC_STARLABS_KBL_LEVELS) : switch (get_uint_option("kbl_brightness", 0)) { : case KBL_OFF: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_OFF); : break; : case KBL_HIGH: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_HIGH); : break; : default: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_LOW); : break; : } : #else : switch (get_uint_option("kbl_brightness", 0)) { : case KBL_OFF: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_OFF); : break; : default: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_ON); : break; : } : #endif
Same as before
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/4d45a1cf_503a3236 PS19, Line 251: ITE ITE
One "ITE" is enough?
lol