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 9:
(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
Done
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@10 PS2, Line 10: APIS
APIs
Done
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@17 PS2, Line 17: Type-A dongle on Volteer
no need to indent here
Done
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@17 PS2, Line 17: Type-A dongle on Volteer
no need to indent here
Done
https://review.coreboot.org/c/coreboot/+/42078/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42078/5//COMMIT_MSG@9 PS5, Line 9: EC being the TCPM decides the mux configuration after : negotiating with the port partner on the Type-C port. The APIs : added here will give the current essential mux state information : for a given port.
Please re-flow for 75 characters per line.
Done
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h... PS5, Line 30: /* Check for the current mux state in EC : * in: int port physical port number of the type-c port : * out: uint8_t flags flags representing the status of the mux such as : * usb capability, dp capability, cable type, etc : */
Done
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h... PS5, Line 325: int google_chromeec_get_num_pd_ports(uint8_t *num_ports);
Why is the signature changed? Please stay with the “native” types like `unsigned int` or `size_t`.
changed this to unsigned int...there is no need for this to be an integer since there is no possible negative amount of ports
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 will update these to the best of my ability but they are not my area of expertise. […]
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 36: */
Same thing, more documentation please. […]
Done
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?
Done
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 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++)
google_chromeec_pd_get_amode() alreacdy existed, so changing it from sending a "raw" command to call […]
ok sure I'll remove it in the next update