Attention is currently required from: Tim Wawrzynczak, Derek Huang, Brandon Breitenstein, Curtis Chen. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57138 )
Change subject: src/ec/google/chromeec: Add and modify APIs for USB-C DP ALT mode ......................................................................
Patch Set 12:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57138/comment/7bcc93ca_9cded882 PS12, Line 9: Add API to allow AP to send the command to EC to enter DP ALT mode, : API to wait for DP HPD event and API to get USB-C mux related : information. Rename google_chromeec_usb_pd_control() to : google_chromeec_usb_pd_get_info() for better understanding. Change : the return value of google_chromeec_wait_for_displayport() : and google_chromeec_pd_get_amode(). Thanks for addressing the comments, Derek. This change however seems to be doing a lot of different things. I think it would be best to divide it into smaller logical changes so that its easier to review and also in case things go wrong only the one small change can be reverted.
I think this can be divided into following CLs:
1. Change `google_chromeec_usb_pd_control()` to `google_chromeec_usb_pd_get_info()`.
2. Update `google_chromeec_pd_get_amode()` to return bitmask. Update `google_chromeec_wait_for_displayport()` to handle the updated return value of `google_chromeec_pd_get_amode()`. Also, `google_chromeec_pd_get_amode()` is not used outside of ec.c. So, it can be dropped from ec.h and made static inside ec.c
3. Add new API functions for DP: a. google_chromeec_typec_control_enter_dp_mode b. google_chromeec_wait_for_dp_hpd
4. Add new API function for USB-C mux handling: google_chromeec_get_usbc_mux_info
5. `google_chromeec_usb_pd_get_info()` is used only inside ec.c. So, make it static and drop it from ec.h
File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/57138/comment/a2e2b23d_04f5953d PS12, Line 12: usbc_mux.h This file doesn't exist.
https://review.coreboot.org/c/coreboot/+/57138/comment/0495d99d_08503a31 PS12, Line 41: int Why was this type changed? I think we should keep it as long.
File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/57138/comment/7e734eed_e2e2cc24 PS12, Line 1481: cable `active_cable` to make the param intent clear.
https://review.coreboot.org/c/coreboot/+/57138/comment/63f5ddd4_17e30e12 PS12, Line 1513: if (google_chromeec_check_feature(EC_FEATURE_TYPEC_REQUIRE_AP_MODE_ENTRY)) { If you invert the check, you can eliminate the additional tab required for rest of the function.
``` if (!google_chromeec_check_feature(EC_FEATURE_TYPEC_REQUIRE_AP_MODE_ENTRY)) return 0;
... ```
https://review.coreboot.org/c/coreboot/+/57138/comment/033dc512_2e02eafc PS12, Line 1653: if (ret) : return ret; : else : return 0; `return ret;` would do the right thing.
https://review.coreboot.org/c/coreboot/+/57138/comment/4186a60a_3e4bfa3a PS12, Line 1679: : return ret; printk BIOS_ERR in this case?