Attention is currently required from: Angel Pons, Dinesh Gehlot, Paul Menzel.
Jayvik Desai has posted comments on this change by Jayvik Desai. ( https://review.coreboot.org/c/coreboot/+/83705?usp=email )
Change subject: vc/google/chromeos: Enable eSOL config with libgfx and uGOP ......................................................................
Patch Set 16:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83705/comment/c75e4a83_90243163?usp... : PS9, Line 7: src/device
vc/google/chromeos
Updated
Commit Message:
https://review.coreboot.org/c/coreboot/+/83705/comment/309baab0_f1938338?usp... : PS13, Line 9: This patch enables the eSOL config option when libgfx or uGOP is enabled : for early graphics initialization.
*early sign of life* could be spelled out once.
Spelled out. Thanks
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/83705/comment/a2345d51_5a4b8aa2?usp... : PS5, Line 83: config CHROMEOS_ENABLES_ESOL
nit: Kconfig symbol names often use "imperative mood" (I forget what it's called), e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/83705/comment/5591cfb5_f2e45d53?usp... : PS5, Line 85: depends on MAINBOARD_HAS_EARLY_LIBGFXINIT || SOC_INTEL_METEORLAKE_SIGN_OF_LIFE
I don't like repeating the condition here and in the `select` statement in `config CHROMEOS`, but `C […]
Changed, cleaned it up!
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/83705/comment/39035dd1_d7cfe0d5?usp... : PS6, Line 105: menu "CHROMEOS_ESOL"
we don't need this
Acknowledged
https://review.coreboot.org/c/coreboot/+/83705/comment/97f26a21_0d3d38ea?usp... : PS6, Line 109: SOC_INTEL_METEORLAKE_SIGN_OF_LIFE
FSP_UGOP_ESOL
Added
https://review.coreboot.org/c/coreboot/+/83705/comment/4aa83b0e_e4203ce8?usp... : PS6, Line 114: endmenu
same
Ack
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/83705/comment/063c89f6_9d17bc93?usp... : PS9, Line 106: bool : default y if FSP_UGOP_EARLY_SIGN_OF_LIFE || MAINBOARD_HAS_EARLY_LIBGFXINIT : default n
nit: I think this is a less verbose equivalent: […]
Updated
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/83705/comment/99f89478_f4052cfd?usp... : PS11, Line 111:
one empty line
Acknowledged
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/83705/comment/8ea5a7d7_ceb4560e?usp... : PS12, Line 106: def_bool y if FSP_UGOP_EARLY_SIGN_OF_LIFE || MAINBOARD_HAS_EARLY_LIBGFXINIT : def_bool n
I suggested setting the default directly using a boolean expression. Did this not work properly? […]
it works properly, i misunderstood your previous comment, updated!