Attention is currently required from: Hung-Te Lin, Jarried Lin, Nancy Lin, Paul Menzel, Yidi Lin.
Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85952?usp=email )
Change subject: mb/google/rauru: Configure firmware display for eDP panel ......................................................................
Patch Set 5: Code-Review+1
(3 comments)
File src/mainboard/google/rauru/mainboard.c:
https://review.coreboot.org/c/coreboot/+/85952/comment/736a60bc_fc96c9b7?usp... : PS4, Line 104: mt6373_enable_vcn33_3(true);
Would `false` disable? I’d prefer an enable and disable function without an argument.
Yes `false` means "disable". IMO splitting this into 2 functions would make the API even more complicated. Note that all MTK regulator APIs are designed this way. Also, renaming the function to something like `mt6373_vcn33_3_change_state(bool enable)` wouldn't improve readability.
https://review.coreboot.org/c/coreboot/+/85952/comment/270e772e_299e31db?usp... : PS4, Line 104: mt6373_enable_vcn33_3(true); : mt6373_set_vcn33_3_voltage(3300000);
This API looks strange. […]
The "enable" and "set" functions write to different registers. The API seems reasonable to me. For example, if in different boot stages we'd like to set different voltages, then we would not want to re-write to the "enable" register.
File src/mainboard/google/rauru/mainboard.c:
https://review.coreboot.org/c/coreboot/+/85952/comment/95728fae_c0105658?usp... : PS5, Line 111: mtk_display_init(); check return value
``` printk(BIOS_ERR, "%s: Failed to init display\n", __func__); ```