Julius Werner 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:
(6 comments)
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.
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.
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.
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.
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.
I would suggest doing something like this:
enum ec_current_image google_chromeec_get_current_image_type(void) { MAYBE_STATIC_BSS enum type = EC_IMAGE_UNKNOWN;
if (type != EC_IMAGE_UNKNOWN) return type;
...do the whole command to initialize 'type'... }
Then you don't have to be careful with the call sequence order, you can just implement google_ec_running_ro() as (google_chromeec_get_current_image_type() == EC_IMAGE_RO) and it will only run the command once.
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...).