Attention is currently required from: Furquan Shaikh, Paul Menzel, Tim Wawrzynczak, Brandon Breitenstein, Curtis Chen. Derek Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57138 )
Change subject: src/ec/google/chromeec: Add APIs for USB-C DP ALT mode ......................................................................
Patch Set 14:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57138/comment/903303eb_03d779ad 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 reviewing. I will update patch set to address the comments.
Done
Commit Message:
https://review.coreboot.org/c/coreboot/+/57138/comment/9e59fea5_78b48700 PS13, Line 10: and API to wait for DP HPD event
Please add a dot/period at the end.
Done
https://review.coreboot.org/c/coreboot/+/57138/comment/e083ba9f_bebc9fe7 PS13, Line 11:
Please elaborate, why 100 ms delay steps are used. […]
Done
File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/57138/comment/39c62212_f33cde59 PS12, Line 12: usbc_mux.h
This file doesn't exist.
Done
https://review.coreboot.org/c/coreboot/+/57138/comment/7bfcf982_65dfa74c PS12, Line 41: int
Why was this type changed? I think we should keep it as long.
Done
File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/57138/comment/240d03d1_6a7abab7 PS12, Line 1481: cable
`active_cable` to make the param intent clear.
Done
https://review.coreboot.org/c/coreboot/+/57138/comment/de7815c3_73a58164 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. […]
Done
https://review.coreboot.org/c/coreboot/+/57138/comment/a880b02d_4b14aa25 PS12, Line 1653: if (ret) : return ret; : else : return 0;
`return ret;` would do the right thing.
Done
https://review.coreboot.org/c/coreboot/+/57138/comment/ba00a2d8_f9e9441e PS12, Line 1679: : return ret;
printk BIOS_ERR in this case?
Done
File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/57138/comment/04789aa4_7879a345 PS13, Line 1674: mdelay(100);
100 ms sounds excessive for coreboot’s normal execution time.
This function should be used only when there is no internal display (like Chromebox) so we must poll HPD event of external display. Also we need display in bios only when the system is in dev mode or recovery mode, we don't care about boot time so much, so we poll every 100ms up to `timeout_ms`