Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/23178 )
Change subject: ec/lenovo/h8: Implement ACPI methods to set battery thresholds ......................................................................
Patch Set 24:
(9 comments)
https://review.coreboot.org/#/c/23178/24//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/23178/24//COMMIT_MSG@9 PS24, Line 9: There are two known (yet) different ways to manage battery thresholds. : This patch implements them and adds a way to enable them for : different mainboards. Are those methods documented somewhere? Is there a specification?
https://review.coreboot.org/#/c/23178/24//COMMIT_MSG@22 PS24, Line 22: What do the `_24` and `_b0` in the filename mean?
https://review.coreboot.org/#/c/23178/24//COMMIT_MSG@26 PS24, Line 26: Change-Id: I2a90f9e9b32462b8a5e9bc8d3087ae0fea563ea5 Please put this right above the Signed-off-by lines.
https://review.coreboot.org/#/c/23178/24/src/ec/lenovo/h8/acpi/thinkpad.asl File src/ec/lenovo/h8/acpi/thinkpad.asl:
https://review.coreboot.org/#/c/23178/24/src/ec/lenovo/h8/acpi/thinkpad.asl@... PS24, Line 321: #if IS_ENABLED(CONFIG_H8_HAS_BAT_TRESHOLDS_IMPL) Does this work with the indentation?
https://review.coreboot.org/#/c/23178/24/src/ec/lenovo/h8/acpi/thinkpad_bat_... File src/ec/lenovo/h8/acpi/thinkpad_bat_thresholds.asl:
https://review.coreboot.org/#/c/23178/24/src/ec/lenovo/h8/acpi/thinkpad_bat_... PS24, Line 23: /* SetBatteryCharge Start/Stop Capacity Threshold Please add a blank comment line.
/* * SetBatteryCharge …
https://review.coreboot.org/#/c/23178/24/src/ec/lenovo/h8/acpi/thinkpad_bat_... PS24, Line 40: * Bit 31: Error status Align *Error* with *Reserved*?
https://review.coreboot.org/#/c/23178/24/src/ec/lenovo/h8/acpi/thinkpad_bat_... PS24, Line 137: Bit 8:Batterycharge Add space after colon (:)?
https://review.coreboot.org/#/c/23178/24/src/ec/lenovo/h8/acpi/thinkpad_bat_... PS24, Line 143: * Bit 31: Error status Align?
https://review.coreboot.org/#/c/23178/24/src/ec/lenovo/h8/acpi/thinkpad_bat_... File src/ec/lenovo/h8/acpi/thinkpad_bat_thresholds_b0.asl:
https://review.coreboot.org/#/c/23178/24/src/ec/lenovo/h8/acpi/thinkpad_bat_... PS24, Line 116: Return (TSH1 & ~0x80) Does this also need to be conditioned on the Kconfig option with bit 7?