Skip to contentSkip to conversation

thinkpad_acpi: Add support for battery thresholds

PATCH Superseded Ognjen Galic 2 participants 3 messages Jan 3, 2018

Subsystems (3)

v12 of 13 Superseded Latest is v13 →

lore
Share & tools
lore
#0 lore raw

PATCH v12 3/4 thinkpad_acpi: Add support for battery thresholds

thinkpad_acpi registers two new attributes for each battery:

1) Charge start threshold /sys/class/power_supply/BATN/charge_start_threshold

Valid values are [0, 99]. A value of 0 turns off the start threshold wear control.

2) Charge stop threshold /sys/class/power_supply/BATN/charge_stop_threshold

Valid values are [1, 100]. A value of 100 turns off the stop threshold wear control. This must be configured first.

---

Notes:
v2:
* Re-write the patch to make the changes in
battery.c generic as suggested by Rafael

v3:
* Fixed a bug where you could not set the stop threshold to 100% when the start threshold is 0%
* Fixed the "Not Charging" quirk to match Lenovo's BIOS Firmware specification

v4:
* Fixed a bug where you could not set the start threshold to >stop if stop was 100%

v5:
* Migrated from symbol_get to native linking, to fix module dependencies
* Fixed a bug where unloading the module would cause a BUG inside battery
* Fixed a bug where you could unload battery before unloading thinkpad_acpi

v6:
* Fixed all the style and naming issues pointed out by Andy Shevchenko

v7:
* No changes in this patch in v7

v8:
* No changes in this patch in v8

v9:
* Use DEVICE_ATTR_RW instead of DEVICE_ATTR
* Use bitopts.h instead of raw operators
* Remove redundant whitespaces
* Remove redundant comments
* Move the power_supply changes to separate patch
* Fix other various styling issues pointed out by Andy Shevchenko

v10:
* Fix a pasting error from v6 that prevents the setting of the start threshold with EINVAL

v11:
* Fix formatting of changelog

v12:
* Remove most whitespaces between ifs
* Change int to acpi_status in tpacpi_battery_acpi_eval

 drivers/platform/x86/Kconfig         |   1 +
 drivers/platform/x86/thinkpad_acpi.c | 386 ++++++++++++++++++++++++++++++++++-
 2 files changed, 386 insertions(+), 1 deletion(-)
drivers/platform/x86/Kconfig +1 -0
index 2c745e8..d705c62 100644
@@ -399,6 +399,7 @@ config SURFACE3_WMI
 config THINKPAD_ACPI
 	tristate "ThinkPad ACPI Laptop Extras"
 	depends on ACPI
+	depends on ACPI_BATTERY
 	depends on INPUT
 	depends on RFKILL || RFKILL = n
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
drivers/platform/x86/thinkpad_acpi.c +385 -1

427 lines not on this page · raw message

Signature · 1 line
2.7.4
#1 lore raw
Quoted 8 lines · Ognjen Galic

thinkpad_acpi registers two new attributes for each battery:

1) Charge start threshold /sys/class/power_supply/BATN/charge_start_threshold

Valid values are [0, 99]. A value of 0 turns off the start threshold wear control.

Quoted 8 lines

2) Charge stop threshold /sys/class/power_supply/BATN/charge_stop_threshold

Valid values are [1, 100]. A value of 100 turns off the stop threshold wear control. This must be configured first.

Signed-off-by Ognjen Galic <smclt30p(a)gmail.com>

Thanks for an update!

Quoted 5 lines

---

Notes:

* Change int to acpi_status in tpacpi_battery_acpi_eval

Then you need to check return code with ACPI_FAILURE() or ACPI_SUCCESS() macros. That is one of the additional burden while I can't see usefulness of ACPI return codes in that function and why I suggested to use plain int.

One more question: why don't you use dev_err()/dev_*() macros instead of pr_*() ones?
(Note, it's a question, needs a bit of discussion, I would like to hear a rationale of this, I think it might be one)

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#2 lore raw
Quoted 22 lines · Andy Shevchenko
Quoted 8 lines · Ognjen Galic

thinkpad_acpi registers two new attributes for each battery:

1) Charge start threshold /sys/class/power_supply/BATN/charge_start_threshold

Valid values are [0, 99]. A value of 0 turns off the start threshold wear control.

Quoted 8 lines

2) Charge stop threshold /sys/class/power_supply/BATN/charge_stop_threshold

Valid values are [1, 100]. A value of 100 turns off the stop threshold wear control. This must be configured first.

Thanks for an update!

Quoted 2 lines

---

Quoted 8 lines
Quoted 3 lines

Notes:

* Change int to acpi_status in tpacpi_battery_acpi_eval

Then you need to check return code with ACPI_FAILURE() or ACPI_SUCCESS() macros. That is one of the additional burden while I can't see usefulness of ACPI return codes in that function and why I suggested to use plain int.

Thos macros basically do the same stuff I do manually. I think that most of the kernel does errors on a non-0 basis, and ACPI is no exception.

That being said, I think that using a simple non-0 check if more clean than using those macros.

Isn't this:

if (!acpi_eval....

nicer than this?

if ACPI_FAILURE(acpi_eval...

It just adds more stuff to get wrong. And I don't really think that the ACPI implementation will change those macros anytime soon, or ever.

Keeping it simple.

Quoted 5 lines

One more question: why don't you use dev_err()/dev_*() macros instead of pr_*() ones?
(Note, it's a question, needs a bit of discussion, I would like to hear a rationale of this, I think it might be one)

Techically, I'm not adding a device this I did not use those macros. I'm only adding more attributes to a device that the battery module manages.

Quoted 4 lines
Signature · 3 lines
-- 
With Best Regards,
Andy Shevchenko