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:
(2 comments)
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/86225/comment/15f8590a_16182560?usp... : PS13, Line 525: config PLATFORM_HAS_LOW_BATTERY_INDICATOR
Does it still make sense that these options are in the FSP directory? Maybe they should also be in ` […]
Kconfigs should all be cleaned up and moved out of FSP directory in later patch, as discussed in CB:86367.
https://review.coreboot.org/c/coreboot/+/86225/comment/9d407bc7_c5e4fe24?usp... : PS13, Line 535: config HAVE_ESOL_SUPPORT_FOR_LOW_BATTERY_INDICATOR
Allow rendering low-battery shutdown msg is a platform choice, so we can;t force a platform to show the eSOL msg or UI msg depending upon mainboard supports uGOP/libgfxinit
Well, that is a choice by the person compiling coreboot. Mainboards should only `select` things that are actually required by the hardware, not policy choices by whoever ships that board. Those choices should be made in menuconfig (in the ChromeOS case, that means in our ebuilds). In this case, I think that is already done by the `PLATFORM_HAS_EARLY_LOW_BATTERY_INDICATOR` option itself (because that is menuconfig-configurable).
low battery rendering would use a portion of eSOL technology. take a case for PTL, where eSOL feature is ready but not low-battery handing inside uGOP therefore, we can't enforce to enable this feature if platform supports either uGOP or libgfxinit
Okay, that's a fair point. But that is determined by the SoC, not the mainboard. So if you want this to guard against someone selecting the option on platforms where it doesn't do anything, then you'd have to put the `select HAVE_ESOL_SUPPORT_FOR_LOW_BATTERY_INDICATOR` into `src/soc/intel/alderlake/Kconfig`.
So I guess this patch is good to merge, but CB:86316 should be changed to target the SoC instead.
To maintain platform independence from ChromeOS, this feature allows overrides for configurable parameters like APIs for logo display, text messages, poweroff control, and low-battery indicators.
I mean, I don't see anything there that's ChromeOS-specific other than the splash screen image itself, and that is already handled by the image file Kconfig. All the other things here are generic features that any OS could choose to use. So I don't think `CHROMEOS_ENABLE_ESOL` should be a ChromeOS option, it should probably move to a more generic place (but doesn't need to be done in this patch).
-
Julius Werner (Code Review)