Tim Wawrzynczak 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:
(12 comments)
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@10 PS2, Line 10: APIS APIs
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@17 PS2, Line 17: Type-A dongle on Volteer no need to indent here
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 30: /* Check for the current mux state in EC */ I realize the comments in here right now are terrible, but it doesn't have to be that way! Can we improve these new ones? e.g., it looks like *flags is an `out` parameter, can we document that? and also that `port` is the port number from the EC's point of view?
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 33: * USB2 and USB3 port numbers between EC and AP : * is not one to one mapping, this function will return the : * correct mapped AP port number in port_map. 80 chars wide is OK here.
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 36: */ Same thing, more documentation please. *port_map is another `out` parameter, and `port` is from EC's point of view?
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 39: int google_chromeec_usb_pd_control(int port, bool *ufp, bool *dbg_acc, What are the possible values of dp_mode?
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.
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. Otherwise how do you tell the difference between PD_CC_DFP_DEBUG_ACC and PD_CC_UFP_DEBUG_ACC?
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
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
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
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?