diff mbox

[2/3,v7] thinkpad_acpi: Add support for battery thresholds

Message ID 20171220151558.GA12609@thinkpad (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Ognjen Galic Dec. 20, 2017, 3:15 p.m. UTC
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 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

Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
---
 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(-)

Comments

kernel test robot Dec. 24, 2017, 4:34 a.m. UTC | #1
Hi Ognjen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on platform-drivers-x86/for-next]
[also build test ERROR on v4.15-rc4 next-20171222]
[cannot apply to pm/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ognjen-Galic/battery-Add-the-battery-hooking-API/20171223-144855
base:   git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next
config: x86_64-acpi-redef (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from include/linux/list.h:9:0,
                    from include/linux/module.h:9,
                    from drivers/power/supply/ds2780_battery.c:16:
>> include/linux/kernel.h:928:42: error: expected identifier or '(' before '{' token
    #define container_of(ptr, type, member) ({    \
                                             ^
   include/linux/power_supply.h:374:33: note: in expansion of macro 'container_of'
    #define to_power_supply(device) container_of(device, struct power_supply, dev)
                                    ^~~~~~~~~~~~
   drivers/power/supply/ds2780_battery.c:59:36: note: in expansion of macro 'to_power_supply'
    static inline struct power_supply *to_power_supply(struct device *dev)
                                       ^~~~~~~~~~~~~~~
--
   In file included from include/linux/list.h:9:0,
                    from include/linux/module.h:9,
                    from drivers/power/supply/ds2781_battery.c:14:
>> include/linux/kernel.h:928:42: error: expected identifier or '(' before '{' token
    #define container_of(ptr, type, member) ({    \
                                             ^
   include/linux/power_supply.h:374:33: note: in expansion of macro 'container_of'
    #define to_power_supply(device) container_of(device, struct power_supply, dev)
                                    ^~~~~~~~~~~~
   drivers/power/supply/ds2781_battery.c:57:36: note: in expansion of macro 'to_power_supply'
    static inline struct power_supply *to_power_supply(struct device *dev)
                                       ^~~~~~~~~~~~~~~
--
   In file included from include/linux/list.h:9:0,
                    from include/linux/module.h:9,
                    from drivers/power//supply/ds2780_battery.c:16:
>> include/linux/kernel.h:928:42: error: expected identifier or '(' before '{' token
    #define container_of(ptr, type, member) ({    \
                                             ^
   include/linux/power_supply.h:374:33: note: in expansion of macro 'container_of'
    #define to_power_supply(device) container_of(device, struct power_supply, dev)
                                    ^~~~~~~~~~~~
   drivers/power//supply/ds2780_battery.c:59:36: note: in expansion of macro 'to_power_supply'
    static inline struct power_supply *to_power_supply(struct device *dev)
                                       ^~~~~~~~~~~~~~~
--
   In file included from include/linux/list.h:9:0,
                    from include/linux/module.h:9,
                    from drivers/power//supply/ds2781_battery.c:14:
>> include/linux/kernel.h:928:42: error: expected identifier or '(' before '{' token
    #define container_of(ptr, type, member) ({    \
                                             ^
   include/linux/power_supply.h:374:33: note: in expansion of macro 'container_of'
    #define to_power_supply(device) container_of(device, struct power_supply, dev)
                                    ^~~~~~~~~~~~
   drivers/power//supply/ds2781_battery.c:57:36: note: in expansion of macro 'to_power_supply'
    static inline struct power_supply *to_power_supply(struct device *dev)
                                       ^~~~~~~~~~~~~~~

vim +928 include/linux/kernel.h

^1da177e4 Linus Torvalds 2005-04-16  911  
91f68b735 Wu Fengguang   2009-01-07  912  
e8c97af0c Randy Dunlap   2017-10-13  913  /**
e8c97af0c Randy Dunlap   2017-10-13  914   * swap - swap values of @a and @b
e8c97af0c Randy Dunlap   2017-10-13  915   * @a: first value
e8c97af0c Randy Dunlap   2017-10-13  916   * @b: second value
91f68b735 Wu Fengguang   2009-01-07  917   */
ac7b90049 Peter Zijlstra 2009-02-04  918  #define swap(a, b) \
ac7b90049 Peter Zijlstra 2009-02-04  919  	do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
91f68b735 Wu Fengguang   2009-01-07  920  
^1da177e4 Linus Torvalds 2005-04-16  921  /**
^1da177e4 Linus Torvalds 2005-04-16  922   * container_of - cast a member of a structure out to the containing structure
^1da177e4 Linus Torvalds 2005-04-16  923   * @ptr:	the pointer to the member.
^1da177e4 Linus Torvalds 2005-04-16  924   * @type:	the type of the container struct this is embedded in.
^1da177e4 Linus Torvalds 2005-04-16  925   * @member:	the name of the member within the struct.
^1da177e4 Linus Torvalds 2005-04-16  926   *
^1da177e4 Linus Torvalds 2005-04-16  927   */
^1da177e4 Linus Torvalds 2005-04-16 @928  #define container_of(ptr, type, member) ({				\
c7acec713 Ian Abbott     2017-07-12  929  	void *__mptr = (void *)(ptr);					\
c7acec713 Ian Abbott     2017-07-12  930  	BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&	\
c7acec713 Ian Abbott     2017-07-12  931  			 !__same_type(*(ptr), void),			\
c7acec713 Ian Abbott     2017-07-12  932  			 "pointer type mismatch in container_of()");	\
c7acec713 Ian Abbott     2017-07-12  933  	((type *)(__mptr - offsetof(type, member))); })
^1da177e4 Linus Torvalds 2005-04-16  934  

:::::: The code at line 928 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 2c745e8..d705c62 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -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
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 117be48..06bcfbd 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -23,7 +23,7 @@ 
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#define TPACPI_VERSION "0.25"
+#define TPACPI_VERSION "0.26"
 #define TPACPI_SYSFS_VERSION 0x030000
 
 /*
@@ -66,6 +66,7 @@ 
 #include <linux/seq_file.h>
 #include <linux/sysfs.h>
 #include <linux/backlight.h>
+#include <linux/bitops.h>
 #include <linux/fb.h>
 #include <linux/platform_device.h>
 #include <linux/hwmon.h>
@@ -78,13 +79,16 @@ 
 #include <linux/workqueue.h>
 #include <linux/acpi.h>
 #include <linux/pci_ids.h>
+#include <linux/power_supply.h>
 #include <linux/thinkpad_acpi.h>
 #include <sound/core.h>
 #include <sound/control.h>
 #include <sound/initval.h>
 #include <linux/uaccess.h>
+#include <acpi/battery.h>
 #include <acpi/video.h>
 
+
 /* ThinkPad CMOS commands */
 #define TP_CMOS_VOLUME_DOWN	0
 #define TP_CMOS_VOLUME_UP	1
@@ -331,6 +335,7 @@  static struct {
 	u32 sensors_pdev_attrs_registered:1;
 	u32 hotkey_poll_active:1;
 	u32 has_adaptive_kbd:1;
+	u32 battery:1;
 } tp_features;
 
 static struct {
@@ -9201,6 +9206,437 @@  static struct ibm_struct mute_led_driver_data = {
 	.resume = mute_led_resume,
 };
 
+/***************** Battery Wear Control Driver ******************/
+
+/* Metadata */
+
+#define GET_START	"BCTG"
+#define SET_START	"BCCS"
+#define GET_STOP	"BCSG"
+#define SET_STOP	"BCSS"
+
+#define START_ATTR "charge_start_threshold"
+#define STOP_ATTR  "charge_stop_threshold"
+
+enum {
+	BAT_ANY = 0,
+	BAT_PRIMARY = 1,
+	BAT_SECONDARY = 2
+};
+
+enum {
+	/* Error condition bit */
+	METHOD_ERR = (1 << 31),
+};
+
+enum {
+	/* This is used in the get/set helpers */
+	THRESHOLD_START,
+	THRESHOLD_STOP,
+};
+
+struct tpacpi_battery_data {
+	int charge_start;
+	int start_support;
+	int charge_stop;
+	int stop_support;
+};
+
+struct tpacpi_battery_driver_data {
+	struct tpacpi_battery_data batteries[3];
+	int individual_addressing;
+};
+
+static struct tpacpi_battery_driver_data battery_info;
+
+/* ACPI helpers/functions/probes */
+
+/**
+ * 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_acpi_eval(char *method, int *ret, int param)
+{
+	if (!acpi_evalf(hkey_handle, ret, method, "dd", param)) {
+		acpi_handle_err(hkey_handle, "%s: evaluate failed", method);
+		return AE_ERROR;
+	}
+
+	if (*ret & METHOD_ERR) {
+		acpi_handle_err(hkey_handle,
+				"%s evaluated but flagged as error", method);
+		return AE_ERROR;
+	}
+
+	return AE_OK;
+}
+
+static int tpacpi_battery_get(int what, int battery, int *ret)
+{
+	switch (what) {
+
+	case THRESHOLD_START:
+
+		if (tpacpi_battery_acpi_eval(GET_START, ret, battery))
+			return -ENODEV;
+
+		/* The value is in the low 8 bits of the response */
+		*ret = *ret & 0xFF;
+		return 0;
+
+	case THRESHOLD_STOP:
+
+		if (tpacpi_battery_acpi_eval(GET_STOP, ret, battery))
+			return -ENODEV;
+
+		/* Value is in lower 8 bits */
+		*ret = *ret & 0xFF;
+
+		/*
+		 * 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.
+		 */
+		if (*ret == 0)
+			*ret = 100;
+
+		return 0;
+
+	default:
+		pr_crit("wrong parameter: %d", what);
+		return -EINVAL;
+	}
+
+}
+
+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;
+
+	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;
+		}
+
+		return 0;
+
+	default:
+		pr_crit("wrong parameter: %d", what);
+		return -EINVAL;
+	}
+
+}
+
+static int tpacpi_battery_probe(int battery)
+{
+	int ret = 0;
+
+	/* Reset the struct */
+	memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));
+
+	/*
+	 * 1) Get the current start threshold
+	 * 2) Check for support
+	 * 3) Get the current stop threshold
+	 * 4) Check for support
+	 */
+
+	if (acpi_has_method(hkey_handle, GET_START)) {
+
+		if (tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {
+			pr_err("Error probing battery %d\n", battery);
+			return -ENODEV;
+		}
+
+		/* Individual addressing is in bit 9 */
+		if (ret & BIT(9))
+			battery_info.individual_addressing = true;
+
+		/* Support is marked in bit 8 */
+		if (ret & BIT(8))
+			battery_info.batteries[battery].start_support = 1;
+		else
+			return -ENODEV;
+
+		if (tpacpi_battery_get(THRESHOLD_START, battery,
+			&battery_info.batteries[battery].charge_start)) {
+			pr_err("Error probing battery %d\n", battery);
+			return -ENODEV;
+		}
+
+	}
+
+	if (acpi_has_method(hkey_handle, GET_STOP)) {
+
+		if (tpacpi_battery_acpi_eval(GET_STOP, &ret, battery)) {
+			pr_err("Error probing battery stop; %d\n", battery);
+			return -ENODEV;
+		}
+
+		/* Support is marked in bit 8 */
+		if (ret & BIT(8))
+			battery_info.batteries[battery].stop_support = 1;
+		else
+			return -ENODEV;
+
+		if (tpacpi_battery_get(THRESHOLD_STOP, battery,
+			&battery_info.batteries[battery].charge_stop)) {
+			pr_err("Error probing battery stop: %d\n", battery);
+			return -ENODEV;
+		}
+	}
+
+	pr_info("battery %d registered (start %d, stop %d)",
+			battery,
+			battery_info.batteries[battery].charge_start,
+			battery_info.batteries[battery].charge_stop);
+
+	return 0;
+}
+
+/* General helper functions */
+
+static int tpacpi_battery_get_id(const char *battery_name)
+{
+
+	if (strcmp(battery_name, "BAT0") == 0)
+		return BAT_PRIMARY;
+	if (strcmp(battery_name, "BAT1") == 0)
+		return BAT_SECONDARY;
+
+	/*
+	 * If for some reason the battery is not BAT0 nor is it
+	 * BAT1, we will assume it's the default, first battery,
+	 * AKA primary.
+	 */
+	pr_warn("unknown battery %s, assuming primary", battery_name);
+	return BAT_PRIMARY;
+}
+
+static int tpacpi_battery_get_attr(struct device_attribute *attr)
+{
+	if (strcmp(START_ATTR, attr->attr.name) == 0)
+		return THRESHOLD_START;
+	if (strcmp(STOP_ATTR, attr->attr.name) == 0)
+		return THRESHOLD_STOP;
+
+	pr_crit("Invalid attribute: %s", attr->attr.name);
+	return -EINVAL;
+}
+
+/* sysfs interface */
+
+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);
+
+	if (!supply) {
+		pr_err("Can't upcast to power_supply!");
+		return -ENODEV;
+	}
+
+	/*
+	 * Some systems have support for more than
+	 * one battery. If that is the case,
+	 * tpacpi_battery_probe marked that addressing
+	 * them individually is supported, so we do that
+	 * based on the device struct.
+	 *
+	 * On systems that are not supported, we assume
+	 * the primary as most of the ACPI calls fail
+	 * with "Any Battery" as the parameter.
+	 */
+	if (battery_info.individual_addressing)
+		/* BAT_PRIMARY or BAT_SECONDARY */
+		battery = tpacpi_battery_get_id(supply->desc->name);
+	else
+		battery = BAT_PRIMARY;
+
+	if (kstrtoul(buf, 10, &value))
+		return -EINVAL;
+
+	attr = tpacpi_battery_get_attr(devattr);
+
+	switch (attr) {
+
+	case THRESHOLD_START:
+
+		if (!battery_info.batteries[battery].start_support)
+			return -ENODEV;
+
+		/* valid values are [0, 99] */
+		if (value < 0 || value > 99)
+			return -EINVAL;
+
+		if (value > battery_info.batteries[battery].charge_stop)
+			return -EINVAL;
+
+		if (tpacpi_battery_set(THRESHOLD_START, battery, value))
+			return -ENODEV;
+
+		battery_info.batteries[battery].charge_stop = value;
+		return count;
+
+	case THRESHOLD_STOP:
+
+		if (!battery_info.batteries[battery].stop_support)
+			return -ENODEV;
+
+		/* valid values are [1, 100] */
+		if (value < 1 || value > 100)
+			return -EINVAL;
+
+
+		if (value < battery_info.batteries[battery].charge_start)
+			return -EINVAL;
+
+		battery_info.batteries[battery].charge_stop = value;
+
+		/*
+		 * When 100 is passed to stop, we need to flip
+		 * it to 0 as that the EC understands that as
+		 * "Default", which will charge to 100%
+		 */
+		if (value == 100)
+			value = 0;
+
+		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
+			return -EINVAL;
+
+		return count;
+
+	default:
+		pr_crit("Wrong parameter: %d", attr);
+		return -EINVAL;
+	}
+
+	return count;
+}
+
+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) {
+		pr_crit("Can't upcast to power_supply!");
+		return -ENODEV;
+	}
+
+	/* THRESHOLD_START or THRESHOLD_STOP */
+	attr = tpacpi_battery_get_attr(devattr);
+
+	/*
+	 * Some systems have support for more than
+	 * one battery. If that is the case,
+	 * tpacpi_battery_probe marked that addressing
+	 * them individually is supported, so we do that
+	 * based on the device struct.
+	 *
+	 * On systems that are not supported, we assume
+	 * the primary as most of the ACPI calls fail
+	 * with "Any Battery" as the parameter.
+	 */
+	if (battery_info.individual_addressing)
+		/* BAT_PRIMARY or BAT_SECONDARY */
+		battery = tpacpi_battery_get_id(supply->desc->name);
+	else
+		battery = BAT_PRIMARY;
+
+	if (tpacpi_battery_get(attr, battery, &ret))
+		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);
+
+static struct attribute *tpacpi_battery_attrs[] = {
+	&dev_attr_charge_start_threshold.attr,
+	&dev_attr_charge_stop_threshold.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(tpacpi_battery);
+
+/* ACPI battery hooking */
+
+static int tpacpi_battery_add(struct power_supply *battery)
+{
+	int batteryid = tpacpi_battery_get_id(battery->desc->name);
+
+	if (tpacpi_battery_probe(batteryid))
+		return -ENODEV;
+
+	if (device_add_groups(&battery->dev, tpacpi_battery_groups))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int tpacpi_battery_remove(struct power_supply *battery)
+{
+	device_remove_groups(&battery->dev, tpacpi_battery_groups);
+	return 0;
+}
+
+static struct acpi_battery_hook battery_hook = {
+	.add_battery = tpacpi_battery_add,
+	.remove_battery = tpacpi_battery_remove,
+	.name = "ThinkPad Battery Extension",
+};
+
+/* Subdriver init/exit */
+
+static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
+{
+	battery_hook_register(&battery_hook);
+	return 0;
+}
+
+static void tpacpi_battery_exit(void)
+{
+	battery_hook_unregister(&battery_hook);
+}
+
+static struct ibm_struct battery_driver_data = {
+	.name = "battery",
+	.exit = tpacpi_battery_exit,
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9647,6 +10083,10 @@  static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = mute_led_init,
 		.data = &mute_led_driver_data,
 	},
+	{
+		.init = tpacpi_battery_init,
+		.data = &battery_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 79e90b3..f0139b4 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -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;