Attention is currently required from: Julius Werner, Jérémy Compostella, Karthik Ramasubramanian.
Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86225?usp=email )
Change subject: vc/google/chromeos: Add low battery indicator screen ......................................................................
Patch Set 3:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/86225/comment/8dd9c3be_0283c9e7?usp... : PS2, Line 11:
Nit: Looks like a double space. Same for the next line too.
Acknowledged
https://review.coreboot.org/c/coreboot/+/86225/comment/85a5b9b0_9a42562f?usp... : PS2, Line 23: TEST=Able to capture the eventlog for low battery boot event.
Can you also please mention that you saw the low battery logo on the screen?
Acknowledged
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/86225/comment/55820920_b4b1823e?usp... : PS1, Line 107: default n
unnecessary, n is the default for boolean.
Acknowledged
https://review.coreboot.org/c/coreboot/+/86225/comment/b7b7fdc4_22fb0c29?usp... : PS1, Line 112: in the firmware. This screen can be used to warn the user
Since this is coreboot code, do we really need to precise that this is a firmware operation ?
Acknowledged
https://review.coreboot.org/c/coreboot/+/86225/comment/7761d321_e9c1dbc5?usp... : PS1, Line 123: If enabled, this option displays a low battery indicator early
Could we maybe precise something like "before memory is available" ?
Acknowledged
https://review.coreboot.org/c/coreboot/+/86225/comment/e77a2112_319c28fa?usp... : PS1, Line 128: Don't select if not sure.
Then maybe something like "Enable only if the platform support it" would be more helpful. Should we introduce a `HAVE_` flag to capture this dependency formally?
we cam introduce yet another Kconfig with `HAVE_` but I don't feel there is a need. We can enable these Kconfigs from config.$BOARD in cros build.
But I'm open for the recommendation/suggestion
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/86225/comment/3321c441_d322125d?usp... : PS2, Line 120: bool "Display Early Low Battery Indicator in firmware"
nit: would be better to more explicitly say "in romstage" or "before memory initialization", rather […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/86225/comment/831caa56_7c363d56?usp... : PS2, Line 121: depends on CHROMEOS_LOW_BATTERY_INDICATOR_SCREEN
depends on CHROMEOS_ENABLE_ESOL?
Acknowledged
File src/vendorcode/google/chromeos/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/86225/comment/36f1dcac_9d480dc5?usp... : PS1, Line 43: cbfs-files-$(CONFIG_CHROMEOS_LOW_BATTERY_INDICATOR_SCREEN) += low_battery_logo.bmp
Acknowledged
-
Subrata Banik (Code Review)