Attention is currently required from: Caveh Jalali, Daisuke Nojiri, Karthik Ramasubramanian, Nick Vaccaro, Paul Menzel, Shelley Chen.
Pavan Holla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81967?usp=email )
Change subject: ec/google/chromeec: Do not fill TypeC ACPI device when UCSI is enabled ......................................................................
Patch Set 9:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81967/comment/e8dd3e02_f11c2373 : PS2, Line 7: Hide typec ACPI device if UCSI is supported
Please add prefix: […]
Done in latest patch.
https://review.coreboot.org/c/coreboot/+/81967/comment/b0cb8693_a39d4514 : PS2, Line 19: https://crrev.com/c/5416841 is the change for adding the feature flag
Please add a dot/period at the end of sentences.
Removed the sentence altogether in latest patch.
File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/81967/comment/81402e12_8fccda24 : PS7, Line 427: CBI_COMMON_CONTROL_UCSI_ENABLED
this bit position needs to be derived from ec_commands.h […]
Done
File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/81967/comment/36775c12_cae15640 : PS2, Line 435: int
Why not bool?
Functions called within google_chromeec_get_ucsi_enabled return int as well. Maybe leaving this as int makes sense too?
File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/81967/comment/88799076_e20a2f4f : PS2, Line 735: printk(BIOS_INFO, "Cannot check whether EC_FEATURE_UCSI_PPM is available.\n");
Log the return value?
Done
File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/81967/comment/0ec65d90_509ddff4 : PS7, Line 729: uint32_t cc
Do you want to define cc as union ec_common_control? Then, you can call cbi_get_uint32: […]
Done
File src/ec/google/chromeec/ec_acpi.c:
https://review.coreboot.org/c/coreboot/+/81967/comment/05cbd731_99f10432 : PS2, Line 161: /* UCSI implementations do not require an ACPI device : * with mux info since the linux kernel doesn't set : * the muxes. */
Please use the recommended commenting styles.
Done in latest patch.