Karthik Ramasubramanian 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.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_chromeec_flash_write_burst_size() can return.
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; Can you please initialize all the command/request & response with either either zero or some safe value? That will avoid any surprises, especially EC might use some fields to tailor the response. Same goes for all the commands here.
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. The return code will help caller identify there was an error.