Attention is currently required from: Karthik Ramasubramanian, Subrata Banik.
Jérémy Compostella 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 1:
(5 comments)
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/86225/comment/fcc2e2d2_760943ba?usp... : PS1, Line 107: default n unnecessary, n is the default for boolean.
https://review.coreboot.org/c/coreboot/+/86225/comment/64d4ff94_f84efedf?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 ?
https://review.coreboot.org/c/coreboot/+/86225/comment/1cff6a4a_756edbac?usp... : PS1, Line 123: If enabled, this option displays a low battery indicator early Could we maybe precise something like "before memory is available" ?
https://review.coreboot.org/c/coreboot/+/86225/comment/f565221f_5a4cf476?usp... : PS1, Line 128: Don't select if not sure. Is this a useful statement ?
File src/vendorcode/google/chromeos/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/86225/comment/50ce9827_408828c7?usp... : PS1, Line 43: cbfs-files-$(CONFIG_CHROMEOS_LOW_BATTERY_INDICATOR_SCREEN) += low_battery_logo.bmp This is the second time this code is duplicated. I would suggest to introduce a function.