Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36207 )
Change subject: ec/google/chromeec: Add EC driver support for software sync ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.h... PS2, Line 183: non-zero
Is it non-zero or less than 0? I believe it is < 0, since that will align well with what google_chro […]
I'll just add docs for each one individually, how about that :)
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 36: EC_VER_MASK
This is already defined in https://review.coreboot.org/cgit/coreboot. […]
Ah yes, of course. Wonder why I didn't get a warning for a duplicate #define then...
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 502: int
Probably okay to make this static? I believe it will be used only by the functions in this file just […]
Done
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 544: offset
It will be helpful to have comments indicating what the params mean for this and the other functions […]
Next version has header comments in the .h file for all the functions I added :)
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 720: EC_LPC_HOST_PACKET_SIZE
For the stack, you ...
Yeah, I remember that change to CML :-) 129KiB! IIRC Intel measured ~400 bytes when FSP-M got called.