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 2:
(2 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 720: EC_LPC_HOST_PACKET_SIZE You can't check an LPC-specific constant in common code.
The depthcharge version we run EC_CMD_GET_PROTOCOL_INFO to figure out the maximum buffer size, and you must do the same. In some cases it may be smaller than you expect (e.g. some SPI NPCX have a bug with returning data after the 256th byte, so we have to restrict the response packet size to that or end up getting errors).
In general, I think we have an overall problem with request/response buffers which it would be better to solve first. Right now we just have a static array to copy into in most drivers (and you're trying to add one for LPC as well), which is inefficient in both speed and memory use. It would be much better if we could just send data from and receive it into the buffer used by the upper application layer directly. We already have the command and the data separated in most parts of the API, it's just create_proto3_request() where the data is copied behind the command in the end. There should be really no need for that... for all our transfer protocols, it doesn't matter whether you send one big buffer or two small ones. If we pull the handling of the data buffers into the individual protocol handlers, we shouldn't need the intermediary transfer buffer anymore, we can directly send from and receive into what we were given.
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().