Tim Wawrzynczak 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 8:
(14 comments)
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.h... PS3, Line 328: /**
/*
Done
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: […]
I like that, will do.
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?
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1607: "
nit: space before "
Done
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, b […]
These are straight from power_manager, and apparently, according to the bug are supposed to all be uppercase ¯_(ツ)_/¯ (see comment#9 on the bug)
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1658: /*struct ec_response_usb_pd_power_info port_info;*/
left over?
Done
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.
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1668: scope
since scope only gets used once it probably doesn't need to be a variable
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1691: (uint64_t)
Is the cast required?
Strictly speaking, no. Will remove.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1695: (uint64_t)
same here.
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1712: *
GOOGLE_CHROMEEC_USBC_DEVICE_NAME
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec_l... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec_l... PS3, Line 467: CHIP_NAME("Google Chrome EC")
Whoops, good catch Paul, thanks!
Done