Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 3:
(10 comments)
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1578: DEVTREE_EARLY Just a thought: Can we add all this new code to ec_chip.c? Reasons:
1. That file can be added only to ramstage. No need for DEVTREE_EARLY guard. 2. We can slowly start moving all the static EC ASL code into ec_chip.c. All ACPI code can be kept in one place.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1599: enum ec_pd_try_power_role_caps try_power_role) This can probably fit on the previous line?
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1607: " nit: space before "
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1631: LEFT Why are the strings for port locations upper case and the rest lower case? Not sure if it matters, but I think it would be good to be consistent?
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1663: = NULL No need to initialize this to NULL. dsd is set before it gets used.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1667: = 0 No need to initialize to 0. It gets set before being used.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1691: (uint64_t) Is the cast required?
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1695: (uint64_t) same here.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1709: * CONx just to make it clear how it is different than the device on line 1712.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1712: * GOOGLE_CHROMEEC_USBC_DEVICE_NAME