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:
(3 comments)
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 504: cmd;
This follows existing practice in this file but I agree that it's a bit dicey. […]
Good idea, I like the static init notation, I'll switch to that.
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 510: *pmask = 0;
Prefer to set the pmask only if there is a valid response. Otherwise we can leave it as is. […]
Ack
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 720: EC_LPC_HOST_PACKET_SIZE
I just don't like adding extra...
I can appreciate that. I'm fine with using alloca() for the buffer. Makes me curious what the stack size is in romstage for newer archs, I'll have to look at that. I also agree some API changes would make everything cleaner. You can absolutely feel free to file feature requests and assign them to me, I would be happy to work on code cleanup as I have time.
So I think we should...
Ack.