Attention is currently required from: Andrey Petrov, Intel coreboot Reviewers, Jérémy Compostella, Karthik Ramasubramanian, Ronak Kanabar, Subrata Banik.
Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86225?usp=email )
Change subject: drivers/intel/fsp2_0: Add low battery indicator screen ......................................................................
Patch Set 13:
(1 comment)
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/86225/comment/187a0cd8_cf831976?usp... : PS13, Line 535: config HAVE_ESOL_SUPPORT_FOR_LOW_BATTERY_INDICATOR Actually, I'm not really sure anymore why this option exists and why mainboards should select it. Why would a mainboard ever not want to select this if the dependencies are there? Why don't we just make `PLATFORM_HAS_EARLY_LOW_BATTERY_INDICATOR` `depends on PLATFORM_HAS_LOW_BATTERY_INDICATOR && (FSP_UGOP_EARLY_SIGN_OF_LIFE || MAINBOARD_HAS_EARLY_LIBGFXINIT)` directly?
(But then again, `FSP_UGOP_EARLY_SIGN_OF_LIFE` and `MAINBOARD_HAS_EARLY_LIBGFXINIT` only determine whether the platform has the ability to show an early update message, but it doesn't say whether that message is actually enabled. But if it is not enabled, there's no point in enabling the early low battery screen. So should we maybe depend in `CHROMEOS_ENABLE_ESOL` instead, because that's actually the option that turns the early message on. But why is that even a `CHROMEOS` option? Shouldn't that be more generalized, either in `src/soc/intel/common` or in the toplevel Kconfig?)
-
Julius Werner (Code Review)