Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1446: struct ec_response_usb_pd_control_v2 resp = {}; : struct chromeec_command cmd = { : .cmd_code = EC_CMD_USB_PD_CONTROL, : .cmd_version = 2, : .cmd_data_in = &pd_control, : .cmd_size_in = sizeof(pd_control), : .cmd_data_out = &resp, : .cmd_size_out = sizeof(resp), : .cmd_dev_index = 0, : }; :
Are we sure about this interface yet? The EC code looks like it's still has some TODOs, etc.
this is currently being used in depthcharge as well so it should be stable
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1460: PD_CC_DFP_ATTACHED
what about PD_CC_DFP_DEBUG_ACC ? That would still indicate it's UFP rather than DFP. […]
I will have to verify why it was done this way with Divya
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1495: * Check for the current mux state in EC : * Flags representing mux state can be : * found in ec_commands.h
80 chars wide
Ack
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1501: if (port < 0) : return -1;
this should go after the declarations
Ack
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1516: if (google_chromeec_command(&cmd) < 0)
blank line after declarations
Ack
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1531: uint8_t num_ports; : int ret; : struct chromeec_command cmd; : int i; : : ret = google_chromeec_get_num_pd_ports(&num_ports); : if (ret < 0) : return -1; : : for (i = 0; i < num_ports; i++)
Thanks for the refactor! But would you mind dumping this into a separate commit?
Any specific reason why? the function was added here so only logical to me that it is propagated across the file