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 36: EC_VER_MASK
I don't see "ver" as a very common abbreviation; how about just EC_VERSION_MASK?
Done
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 720: EC_LPC_HOST_PACKET_SIZE
You can't check an LPC-specific constant in common code.
Ack. I missed that, my bad.
The depthcharge version...
Understood, will add support for that.
In general...
I understand what you're saying here. However, the LPC driver does actually use the "command" buffer the user provides directly, and does not invoke crosec_proto; google_chromeec_command() passes the struct chromeec_command* directly to google_chromeec_command_v3 for example, which calls write_bytes(), which directly writes to the LPC port, no copying involved. The SPI and I2C drivers do use crosec_proto which does the copying you reference. This seems like an issue with the crosec_proto driver in particular, not this code. Let me spend a little time looking this over.
https://review.coreboot.org/c/coreboot/+/36207/2/src/ec/google/chromeec/ec.c... PS2, Line 771: int google_chromeec_test(void)
There's already a google_chromeec_hello().
That's fair, I'll remove the static declaration and add the check that it returns the correct modification to 'in_data'.