thinkpad_acpi: Add support for battery thresholds
Subsystems (3)
PATCH v5 3/3 thinkpad_acpi: Add support for battery thresholds
This patch uses the new battery hooking API to add support for the ACPI battery wear control present in Lenovo ThinkPads that have >=SandyBridge processors.
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
--- drivers/platform/x86/Kconfig | 1 + drivers/platform/x86/thinkpad_acpi.c | 463 ++++++++++++++++++++++++++++++++++- 2 files changed, 463 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 +462 -1
499 lines not on this page · raw message
Signature · 1 line
2.7.4
Andy Shevchenko
drivers/platform/x86/thinkpad_acpi.c line 88
Quoted 12 lines · Ognjen Galic
#include <
linux/jiffies.h>
#include <linux/workqueue.h>
#include <linux/acpi.h>drivers/platform/x86/thinkpad_acpi.c +2
-0+#include <linux/power_supply.h> #include <linux/pci_ids.h> #include <linux/thinkpad_acpi.h> #include <sound/core.h> @@ -84,6 +85,8 @@#include <sound/initval.h> #include <linux/uaccess.h> #include <acpi/video.h> +#include <acpi/battery.h>
I would try to keep it more ordered (yes, I noticed that no order in the original file), like
#include <linux/acpi.h>
#include <linux/pci_ids.h>
+#include <linux/power_supply.h>
#include <linux/thinkpad_acpi.h>
+#include <acpi/battery.h>
#include <acpi/video.h>
instead of above.
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9249
Quoted 1 line
drivers/platform/x86/thinkpad_acpi.c +1
-0+/**
I think the function name is missed here.
Quoted 8 lines
drivers/platform/x86/thinkpad_acpi.c +8
-0+* This evaluates a ACPI method call specific to the battery +* ACPI extension. The specifics are that an error is marked +* in the 32rd bit of the response, so we just check that here. +* +* Returns 0 on success +*/ +static int tpacpi_battery_eval (char *method,int *ret,int param) +{
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9267
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +10
-0+if (!acpi_evalf (hkey_handle, ret, method,"dd" , param)) { +pr_err ("%s: evaluate failed" , method); +return AE_ERROR ; + } + +if (*ret &METHOD_ERR ) { +pr_err ("%s evaluated but flagged as error" , method); +return AE_ERROR ; + } +
Just a side note: does it make any sense to switch to acpi_handle_err() at some point?
Quoted 4 lines
drivers/platform/x86/thinkpad_acpi.c +3
-0+return AE_OK ; +} +static int tpacpi_battery_get (int what,int battery,int *ret)
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9298
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +8
-0+{ +/* +* On the stop value, if we return 0 that +* does not make any sense. 0 means Default, which +* means that charging stops at 100%, so we return +* that. +*/ + *ret = *ret ==0 ?100 : *ret;
Hmm... Perhaps just
if (*ret == 0)
*ret = 100;
?
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9316
Quoted 13 lines
drivers/platform/x86/thinkpad_acpi.c +12
-0+return 0 ; +} + +static int tpacpi_battery_set (int what,int battery,int value) +{ + +int param =0x0 , ret =0xFFFFFFFF ; + +/* 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 );
Redundant parens.
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9356
Quoted 13 lines
drivers/platform/x86/thinkpad_acpi.c +12
-0+} + +static int tpacpi_battery_probe (int battery) +{ +int ret =0 ; + +/* Reset the struct */ + battery_info.start_support[battery] =0x0 ; + battery_info.stop_support[battery] =0x0 ; + battery_info.start_charge[battery] =0x0 ; + battery_info.stop_charge[battery] =0x0 ; +
Hmm... If you reorganize your data struct to
struct _info {
};
struct _pdata {
...
struct _info info[3];
};
You can use memset() and memcpy() or just plain assignment whenever it's needed.
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9372
Quoted 2 lines
drivers/platform/x86/thinkpad_acpi.c +2
-0+/* Individual addressing is in bit 9 */ +if (ret & (1 <<9 ))
BIT(9) ?
Review
Quoted 2 lines
(quoted diff) +2
-0+ /* Support is marked in bit 8 */ + if (ret & (1 << 8))
BIT(8) ?
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9379
Quoted 4 lines
drivers/platform/x86/thinkpad_acpi.c +3
-0+ battery_info.start_support[battery] =1 ; + else + return -ENODEV;
OK, here it is in align with the previous cond.
Quoted 11 lines
drivers/platform/x86/thinkpad_acpi.c +10
-0+/* Support is marked in bit 8 */ +if (ret & (1 <<8 )) + battery_info.stop_support[battery] =1 ; +pr_info ("battery %d registered (start %d, stop %d)" , + battery, + battery_info.start_charge[battery], + battery_info.stop_charge[battery]); + +return 0 ; +}
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9424
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +9
-0+ +/* General helper functions */ + +static int tpacpi_battery_get_id (const char *battery_name) +{ + +if (strcmp (battery_name,"BAT0" ) ==0 ) +return BAT_PRIMARY ; +else if (strcmp (battery_name,"BAT1" ) ==0 )
1. Redundant 'else'.
2. Consider to use match_string().
Quoted 14 lines
drivers/platform/x86/thinkpad_acpi.c +13
-0+} +static int tpacpi_battery_get_attr (struct device_attribute *attr) +{ +if (strcmp (START_ATTR , attr->attr.name) ==0 ) +return THRESHOLD_START ; +else if (strcmp (STOP_ATTR , attr->attr.name) ==0 ) +return THRESHOLD_STOP ; + +pr_crit ("Invalid attribute: %s" , attr->attr.name); +return -EINVAL ; +} + +/* sysfs interface */
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9456
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +9
-0+ +static ssize_t tpacpi_battery_store (struct device *dev, +struct device_attribute *devattr, +const char *buf,size_t count) +{ +int attr, battery; +long value; +struct power_supply *supply = + container_of(dev, struct power_supply, dev);
drivers/power/supply/power_supply_core.c:671:
Perhaps move it to the header of power supply subsystem?
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9459
Quoted 3 lines
drivers/platform/x86/thinkpad_acpi.c +3
-0+ +if (!supply) { +pr_crit ("Can't upcast to power_supply!" );
I recommend to reconsider levels on which you print some messages here. Do we really need KERN_CRIT?
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9480
Quoted 11 lines
drivers/platform/x86/thinkpad_acpi.c +10
-0+return -ENODEV ; + } + +if (battery_info.individual_addressing) +/* BAT_PRIMARY or BAT_SECONDARY */ + battery =tpacpi_battery_get_id (supply->desc->name); +else + battery =BAT_PRIMARY ; + +if (kstrtol (buf,10 , &value))
...ul()?
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9525
Quoted 6 lines
drivers/platform/x86/thinkpad_acpi.c +5
-0+return -EINVAL ; + + attr =tpacpi_battery_get_attr (devattr); + + value = value ==100 ?0 : value;
if (value == 100)
value = 0;
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9546
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +9
-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 = + container_of(dev, struct power_supply, dev);
See above.
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +9
-0+} + +static const struct device_attribute battery_start = { + .show = tpacpi_battery_show, + .store = tpacpi_battery_store, + .attr = { + .name =START_ATTR , + .mode =0644 , + },
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9595
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +10
-0+}; + +static const struct device_attribute battery_stop = { + .show = tpacpi_battery_show, + .store = tpacpi_battery_store, + .attr = { + .name =STOP_ATTR , + .mode =0644 , + }, +};
Don't we have some helper macros for above?
Something based on __ATTR_RW() ?
Quoted 4 lines
drivers/platform/x86/thinkpad_acpi.c +4
-0+static int tpacpi_battery_add (struct power_supply *battery) +{ +int batteryid =tpacpi_battery_get_id (battery->desc->name); +
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9612
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +10
-0+pr_info ("battery %s added" , battery->desc->name); + +if (tpacpi_battery_probe (batteryid)) +return -ENODEV ; + +if (device_create_file (&battery->dev, &battery_start)) +return -ENODEV ; + +if (device_create_file (&battery->dev, &battery_stop)) +return -ENODEV ;
Shouldn't be done via group attribures?
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9638
Quoted 12 lines
drivers/platform/x86/thinkpad_acpi.c +9
-0+ +return 0 ; +} +static int __init tpacpi_battery_init (struct ibm_init_struct *ibm) +{ + tp_features.battery =0 ; +battery_hook_register (&battery_hook); + +pr_info ("battery driver initialized" );
This kind of messages is somehow useless. 'initcall_debug' will show you this.
Quoted 4 lines
drivers/platform/x86/thinkpad_acpi.c +4
-0+ + battery_info.individual_addressing =0 ; + tp_features.battery =1 ; +
drivers/platform/x86/thinkpad_acpi.c static struct ibm_struct mute_led_driver_data = { · line 9651
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +9
-0+return 0 ; +} + +static void tpacpi_battery_exit (void ) +{ +if (!tp_features.battery) { +pr_info ("battery driver was not loaded, not de-registering" ); +return ; + }
When might it happen?
Quoted 3 lines
drivers/platform/x86/thinkpad_acpi.c +3
-0+ +battery_hook_unregister (&battery_hook); +}
Signature · 2 lines
With Best Regards, Andy Shevchenko