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 7:
(10 comments)
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.h... PS7, Line 153: google_chromeec_hello
Function header/documentation?
Done
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 720: EC_LPC_HOST_PACKET_SIZE
For the stack, you ... […]
Done
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.c... PS7, Line 692: EC_LPC_HOST_PACKET_SIZE
Still need to use GET_PROTOCOL_INFO for this.
Done
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.c... PS7, Line 694: sizeof(struct ec_host_request);
Looks like an inconsistent indentation.
Done
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.c... PS7, Line 701: return EC_FLASH_WRITE_VER0_SIZE;
We don't really need to support this anymore, all ECs that will use this code have the new version.
Done
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.c... PS7, Line 810: if (rv == -EC_RES_INVALID_PARAM || rv == -EC_RES_INVALID_COMMAND) {
This too... all current and future ECs support this.
Done
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.c... PS7, Line 1408: static enum ec_current_image ec_image_type;
Please initialize explicitly to = EC_IMAGE_UNKNOWN if you want to rely on that, for clarity.
Done
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.c... PS7, Line 1409: google_chromeec_get_current_image
Still not really happy how this works... now you'd just call it twice. […]
Done
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.c... PS7, Line 1442: google_chromeec_get_current_image();
This wouldn't need to be here then (and honestly, we don't really need to do hello() here either... […]
Done
https://review.coreboot.org/c/coreboot/+/36207/7/src/ec/google/chromeec/ec.c... PS7, Line 1474: for (i = 0; i < resp.num_ports; i++) { : struct ec_params_usb_pd_get_mode_request params; : struct ec_params_usb_pd_get_mode_response resp2; : int svid_idx = 0; : : do { : /* Reset cmd in each iteration in case : google_chromeec_command changes it. */ : params.port = i; : params.svid_idx = svid_idx; : cmd.cmd_code = EC_CMD_USB_PD_GET_AMODE; : cmd.cmd_version = 0; : cmd.cmd_data_in = ¶ms; : cmd.cmd_size_in = sizeof(params); : cmd.cmd_data_out = &resp2; : cmd.cmd_size_out = sizeof(resp2); : cmd.cmd_dev_index = 0; : : if (google_chromeec_command(&cmd) < 0) : return -1; : if (resp2.svid == svid) : return 1; : svid_idx++; : } while (resp2.svid);
Was this supposed to be put into the previous CL?
Done