Attention is currently required from: Sean Rhodes. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58343 )
Change subject: ec/starlabs: Add standardised ITE EC support ......................................................................
Patch Set 5:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/58343/comment/0ca62f35_03df8e3f PS5, Line 14: Wow, thank you very much. Could you please elaborate a little?
1. What datasheets were used? 2. Please mention the different EC firmware options. 3. Where are the interfaces described? 4. On what laptop was it tested, and how can it be checked under GNU/Linux? 5. Does Microsoft Windows and other operating systems work with the Merlin firmware?
File src/ec/starlabs/merlin/Kconfig:
https://review.coreboot.org/c/coreboot/+/58343/comment/3dae57ae_dbeae730 PS5, Line 7: ITE embedded controller … with XXX firmware …
https://review.coreboot.org/c/coreboot/+/58343/comment/4b7ea12e_dee33d98 PS5, Line 28: Use open source embedded controller firmware 1. Please add an URL to the repository or Web site. 2. Could you also list the differences between the vendor EC firmware and the Merlin one?
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/1b10d321_fb7ba4a4 PS5, Line 39: printk(BIOS_DEBUG, "ITE: Device not found.\n"); Should that be an error?
Also, what are the consequences, if it is not found?
https://review.coreboot.org/c/coreboot/+/58343/comment/5dd103cd_871a830d PS5, Line 57: * Remove the blank line?
File src/ec/starlabs/merlin/variants/cml/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/8325288d_49337672 PS5, Line 4: * EC communication interface for ITE ITE Embedded Controller. Please remove the dot/period at the end, as it’s not a sentence.
https://review.coreboot.org/c/coreboot/+/58343/comment/e98dbd48_01549394 PS5, Line 4: ITE ITE Embedded Controller Is exactly one ITE enough?
https://review.coreboot.org/c/coreboot/+/58343/comment/68db04ab_91599bd0 PS5, Line 10: /* IT5570 chip ID byte values. */ Ditto.
https://review.coreboot.org/c/coreboot/+/58343/comment/159aca4a_289b7890 PS5, Line 14: /* EC RAM offsets. */ Ditto. (End probably other files.)
-
Paul Menzel (Code Review)