thinkpad_acpi: Add support for battery thresholds
PATCH v8 2/3 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.
This patch depends on the following patches:
"battery: Add the battery hooking API"
v2:
* Re-write the patch to make the changes inbattery.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
--- drivers/platform/x86/Kconfig | 1 + drivers/platform/x86/thinkpad_acpi.c | 442 ++++++++++++++++++++++++++++++++++- include/linux/power_supply.h | 2 + 3 files changed, 444 insertions(+), 1 deletion(-)
drivers/platform/x86/Kconfig +1 -0
@@ -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 +441 -1
485 lines not on this page · raw message
include/linux/power_supply.h +2 -0
@@ -371,6 +371,8 @@ devm_power_supply_register_no_ws(struct device *parent,extern void power_supply_unregister (struct power_supply *psy);extern int power_supply_powers (struct power_supply *psy,struct device *dev); +#define to_power_supply (device) container_of(device, struct power_supply, dev) +extern void *power_supply_get_drvdata (struct power_supply *psy);/* For APM emulation, think legacy userspace. */ extern struct class *power_supply_class;
Signature · 1 line
2.7.4
Andy Shevchenko
Quoted 11 lines · Ognjen Galic
thinkpad_acpiregisters two new attributes for each battery:1) Charge start threshold
/sys/class/power_supply/BATN/charge_start_thresholdValid 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
Quoted 7 lines
Valid values are [1, 100]. A value of 100 turns off the stop threshold wear control. This must be configured first.
This patch depends on the following patches:
"battery: Add the battery hooking API"
Sorry, didn't notice earlier some things commented below.
Quoted 4 lines
#include <
acpi/video.h>+
/* ThinkPad CMOS commands */
Redundant change.
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9229
Quoted 3 lines
drivers/platform/x86/thinkpad_acpi.c +3
-0+enum { +/* Error condition bit */ +METHOD_ERR = (1 <<31 ),
bitops.h but no BIT() use?
BIT(31) ?
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9318
Quoted 6 lines
drivers/platform/x86/thinkpad_acpi.c +4
-0+}; +static int tpacpi_battery_set (int what,int battery,int value) +{ +
Redundant.
Quoted 1 line
+ int param =
0x0, ret =0xFFFFFFFF;
Useless assignment for param, not sure abour ret.
Quoted 23 lines
drivers/platform/x86/thinkpad_acpi.c +23
-0+ +/* The first 8 bits are the value of the threshold */ + param = value; +/* The battery ID is in bits 8-9, 2 bits */ + param |= battery <<8 ; + +switch (what) { + +case THRESHOLD_START : + +if (tpacpi_battery_acpi_eval (SET_START , &ret, param)) { +pr_err ("failed to set charge threshold on battery %d" , + battery); +return -ENODEV ; + } + +return 0 ; + +case THRESHOLD_STOP : + +if (tpacpi_battery_acpi_eval (SET_STOP , &ret, param)) { +pr_err ("failed to set charge stop threshold: %d" , battery); +return -ENODEV ;
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9351
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +9
-0+ } + +return 0 ; + +default : +pr_crit ("wrong parameter: %d" , what); +return -EINVAL ; + } +
Redundant.
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9358
Quoted 9 lines
drivers/platform/x86/thinkpad_acpi.c +7
-0+} + +static int tpacpi_battery_probe (int battery) +{ +int ret =0 ; + +/* Reset the struct */
Useless.
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9459
Quoted 11 lines
drivers/platform/x86/thinkpad_acpi.c +9
-0+memset (&battery_info,0 ,sizeof (struct tpacpi_battery_driver_data )); +} +static ssize_t tpacpi_battery_store (struct device *dev, +struct device_attribute *devattr, +const char *buf,size_t count) +{ +int attr, battery; +unsigned long value; +struct power_supply *supply =to_power_supply (dev);
Can you use reverse xmas tree, esp. put assignments first.
Quoted 1 line
+ if (!supply) {
How this could be possible?!
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9484
Quoted 6 lines
drivers/platform/x86/thinkpad_acpi.c +5
-0+pr_err ("Can't upcast to power_supply!" ); +return -ENODEV ; + } +if (kstrtoul (buf,10 , &value)) +return -EINVAL ;
Don't shadow an error code from kstrtoul().
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9551
Quoted 8 lines
drivers/platform/x86/thinkpad_acpi.c +7
-0+static ssize_t tpacpi_battery_show (struct device *dev, +struct device_attribute *devattr, +char *buf) +{ +int ret =0x0 , attr, battery; +struct power_supply *supply =to_power_supply (dev); +if (!supply) {
How this could be possible?!
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9585
Quoted 11 lines
drivers/platform/x86/thinkpad_acpi.c +10
-0+pr_crit ("Can't upcast to power_supply!" ); +return -ENODEV ; + } +return sprintf (buf,"%d\n" , ret); +} + +DEVICE_ATTR (charge_start_threshold,0644 , + tpacpi_battery_show, tpacpi_battery_store); +DEVICE_ATTR (charge_stop_threshold,0644 , + tpacpi_battery_show, tpacpi_battery_store);
DEVICE_ATTR_RW().
include/linux/power_supply.h devm_power_supply_register_no_ws(struct device *parent, · line 378
Quoted 11 lines
include/linux/power_supply.h +2
-0@@ -371,6 +371,8 @@ devm_power_supply_register_no_ws(struct device *parent,extern void power_supply_unregister (struct power_supply *psy);extern int power_supply_powers (struct power_supply *psy,struct device *dev); +#define to_power_supply (device) container_of(device, struct power_supply, dev) +extern void *power_supply_get_drvdata (struct power_supply *psy);/* For APM emulation, think legacy userspace. */ extern struct class *power_supply_class;
This one sounds to me as a separate change.
At the same time you may convert the current user of it to make sense of the change. drivers/power/supply/power_supply_core.c:671:
I think I pointed to this out once.
Signature · 2 lines
With Best Regards, Andy Shevchenko
A message that is not in the archive (sent off-list or not yet received): CAHp75Vc2MYDLanvZmqaPkmFVfNbyBQoPwbRDDFmKDtSPAmfj9w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
On 21 Dec 2017 2:53 pm, "Andy Shevchenko" <[email protected]> wrote:
Quoted 11 lines · Ognjen Galic
thinkpad_acpiregisters two new attributes for each battery:1) Charge start threshold
/sys/class/power_supply/BATN/charge_start_thresholdValid 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
Quoted 7 lines
Valid values are [1, 100]. A value of 100 turns off the stop threshold wear control. This must be configured first.
This patch depends on the following patches:
"battery: Add the battery hooking API"
Sorry, didn't notice earlier some things commented below.
Quoted 4 lines
#include <
acpi/video.h>+
/* ThinkPad CMOS commands */
Redundant change.
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9229
Quoted 3 lines
drivers/platform/x86/thinkpad_acpi.c +3
-0+enum { +/* Error condition bit */ +METHOD_ERR = (1 <<31 ),
bitops.h but no BIT() use?
BIT(31) ?
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9318
Quoted 6 lines
drivers/platform/x86/thinkpad_acpi.c +4
-0+}; +static int tpacpi_battery_set (int what,int battery,int value) +{ +
Redundant.
Quoted 1 line
+ int param =
0x0, ret =0xFFFFFFFF;
Useless assignment for param, not sure abour ret.
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = {
Quoted 12 lines
drivers/platform/x86/thinkpad_acpi.c +12
-0+ +/* The first 8 bits are the value of the threshold */ + param = value; +/* The battery ID is in bits 8-9, 2 bits */ + param |= battery <<8 ; + +switch (what) { + +case THRESHOLD_START : + +if (tpacpi_battery_acpi_eval (SET_START , &ret, param)) { + pr_err("failed to set charge threshold on battery
%d",
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = {
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +10
-0+ battery); +return -ENODEV ; + } + +return 0 ; + +case THRESHOLD_STOP : + +if (tpacpi_battery_acpi_eval (SET_STOP , &ret, param)) { + pr_err("failed to set charge stop threshold: %d",
battery);
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9351
Quoted 11 lines
drivers/platform/x86/thinkpad_acpi.c +10
-0+return -ENODEV ; + } + +return 0 ; + +default : +pr_crit ("wrong parameter: %d" , what); +return -EINVAL ; + } +
Redundant.
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9358
Quoted 9 lines
drivers/platform/x86/thinkpad_acpi.c +7
-0+} + +static int tpacpi_battery_probe (int battery) +{ +int ret =0 ; + +/* Reset the struct */
Useless.
Quoted 1 line
+ memset(&battery_info, 0, sizeof(struct
tpacpi_battery_driver_data));
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9459
Quoted 9 lines
drivers/platform/x86/thinkpad_acpi.c +8
-0+} +static ssize_t tpacpi_battery_store (struct device *dev, +struct device_attribute *devattr, +const char *buf,size_t count) +{ +int attr, battery; +unsigned long value; +struct power_supply *supply =to_power_supply (dev);
Can you use reverse xmas tree, esp. put assignments first.
Quoted 1 line
+ if (!supply) {
How this could be possible?!
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9484
Quoted 6 lines
drivers/platform/x86/thinkpad_acpi.c +5
-0+pr_err ("Can't upcast to power_supply!" ); +return -ENODEV ; + } +if (kstrtoul (buf,10 , &value)) +return -EINVAL ;
Don't shadow an error code from kstrtoul().
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9551
Quoted 8 lines
drivers/platform/x86/thinkpad_acpi.c +7
-0+static ssize_t tpacpi_battery_show (struct device *dev, +struct device_attribute *devattr, +char *buf) +{ +int ret =0x0 , attr, battery; +struct power_supply *supply =to_power_supply (dev); +if (!supply) {
How this could be possible?!
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9585
Quoted 11 lines
drivers/platform/x86/thinkpad_acpi.c +10
-0+pr_crit ("Can't upcast to power_supply!" ); +return -ENODEV ; + } +return sprintf (buf,"%d\n" , ret); +} + +DEVICE_ATTR (charge_start_threshold,0644 , + tpacpi_battery_show, tpacpi_battery_store); +DEVICE_ATTR (charge_stop_threshold,0644 , + tpacpi_battery_show, tpacpi_battery_store);
DEVICE_ATTR_RW().
I did not use DEVICE_ATTR_RW() because I can't use a common store and show function for both attributes to minimize the code I need. If I used DEVICE_ATTR_RW() I would need to define 4 more functions that do the same. That seems redundant.
include/linux/power_supply.h devm_power_supply_register_no_ws(struct device
Quoted 3 lines
include/linux/power_supply.h +0
-0@@ -371,6 +371,8 @@ devm_power_supply_register_no_ws(struct device
*parent,
include/linux/power_supply.h devm_power_supply_register_no_ws(struct device · line 372
Quoted 2 lines
include/linux/power_supply.h +0
-0extern void power_supply_unregister (struct power_supply *psy); extern int power_supply_powers(struct power_supply *psy, struct device
*dev);
include/linux/power_supply.h devm_power_supply_register_no_ws(struct device · line 373
Quoted 2 lines
include/linux/power_supply.h +1
-0+#define to_power_supply (device) container_of(device, struct
power_supply, dev)
include/linux/power_supply.h devm_power_supply_register_no_ws(struct device *parent, · line 378
Quoted 4 lines
include/linux/power_supply.h +1
-0+extern void *power_supply_get_drvdata (struct power_supply *psy);/* For APM emulation, think legacy userspace. */ extern struct class *power_supply_class;
This one sounds to me as a separate change.
At the same time you may convert the current user of it to make sense of the change. drivers/power/supply/power_supply_core.c:671:
I think I pointed to this out once.
I wanted to minimize the changes in pm to avoid going through all the review process hassle for a few simple changes, because I'm modifying a third subsystem. But I will do it later tonight.
This is my first patchset and I did not expect for the review process to be this agonizingly slow, so I wanted to speed it up by not touching pm.
Thanks for the comments!
Ognjen
--
With Best Regards,
Andy Shevchenko
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
ibm-acpi-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Andy Shevchenko
Quoted 2 lines · Ognjen Galić
On 21 Dec 2017 2:53 pm, "Andy Shevchenko" <andy.shevchenko(a)gmail.com> wrote:
On Thu, Dec 21, 2017 at 12:00 PM, Ognjen Galic <smclt30p(a)gmail.com> wrote:
You need stop using HTML as well.
Quoted 11 lines
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9585
Quoted 4 lines
drivers/platform/x86/thinkpad_acpi.c +4
-0+DEVICE_ATTR (charge_start_threshold,0644 , + tpacpi_battery_show, tpacpi_battery_store); +DEVICE_ATTR (charge_stop_threshold,0644 , + tpacpi_battery_show, tpacpi_battery_store);
DEVICE_ATTR_RW().I did not use
DEVICE_ATTR_RW()because I can't use a common store and show function for both attributes to minimize the code I need. If I usedDEVICE_ATTR_RW()I would need to define 4 more functions that do the same. That seems redundant.
_RW() variant still preferable since it makes much more easy to understand that this attribute doesn't need any *special* permissions.
I think it's not big payment to make it so.
Quoted 9 lines
Quoted 4 lines
+#define
to_power_supply(device)container_of(device, struct power_supply, dev)This one sounds to me as a separate change.
At the same time you may convert the current user of it to make sense of the change.
drivers/power/supply/power_supply_core.c:671:
Quoted 5 lines
I think I pointed to this out once.
I wanted to minimize the changes in pm to avoid going through all the review process hassle for a few simple changes, because I'm modifying a third subsystem. But I will do it later tonight.
Thanks!
Quoted 2 lines
This is my first patchset and I did not expect for the review process to be this agonizingly slow, so I wanted to speed it up by not touching pm.
I see, though you actually made an opposite by that decision :-)
Signature · 2 lines
With Best Regards, Andy Shevchenko