battery: Add the battery hooking API
Subsystems (3)
PATCH v11 1/5 battery: Add the battery hooking API
This is a patch that implements a generic hooking API for the generic ACPI battery driver.
With this new generic API, drivers can expose platform specific behaviour via sysfs attributes in /sys/class/power_supply/BATn/ in a generic way.
A perfect example of the need for this API are Lenovo ThinkPads.
Lenovo ThinkPads have a ACPI extension that allows the setting of start and stop charge thresholds in the EC and battery firmware via ACPI. The thinkpad_acpi module can use this API to expose sysfs attributes that it controls inside the ACPI battery driver sysfs tree, under /sys/class/power_supply/BATN/.
The file drivers/acpi/battery.h has been moved to include/acpi/battery.h and the includes inside ac.c, sbs.c, and battery.c have been adjusted to reflect that.
When drivers hooks into the API, the API calls add_battery() for each battery in the system that passes it a acpi_battery struct. Then, the drivers can use device_create_file() to create new sysfs attributes with that struct and identify the batteries for per-battery attributes.
v7:
* Implemented mutual exclusion for hooking methods and battery callbacks
* Fixed a BUG where errors in other modules would occur when the modules that depend on battery get unloaded
v8:
* Use list_for_each_safe instead of list_for_each for the module exit function where deletion of nodes occurs
v9:
* No changes in this patch in v9
v10:
* Fix compiler warnings in Intel's 0-day CI
v11:
* Fix changelog formatting
* Make lists and mutex static
--- drivers/acpi/ac.c | 2 +- drivers/acpi/battery.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++++- drivers/acpi/battery.h | 11 --- drivers/acpi/sbs.c | 2 +- include/acpi/battery.h | 21 ++++++ 5 files changed, 199 insertions(+), 16 deletions(-) delete mode 100644 drivers/acpi/battery.h create mode 100644 include/acpi/battery.h
drivers/acpi/ac.c +1 -1
@@ -33,7 +33,7 @@#include <linux/platform_device.h> #include <linux/power_supply.h> #include <linux/acpi.h> -#include "battery.h" +#include <acpi/battery.h> #define PREFIX "ACPI: "
drivers/acpi/battery.c +176 -3
228 lines not on this page · raw message
drivers/acpi/battery.h deleted +0 -11
@@ -1,11 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -#ifndef __ACPI_BATTERY_H -#define __ACPI_BATTERY_H - -#define ACPI_BATTERY_CLASS "battery" - -#define ACPI_BATTERY_NOTIFY_STATUS 0x80 -#define ACPI_BATTERY_NOTIFY_INFO 0x81 -#define ACPI_BATTERY_NOTIFY_THRESHOLD 0x82 - -#endif
drivers/acpi/sbs.c +1 -1
@@ -32,9 +32,9 @@#include <linux/delay.h> #include <linux/power_supply.h> #include <linux/platform_data/x86/apple.h> +#include <acpi/battery.h> #include "sbshc.h" -#include "battery.h" #define PREFIX "ACPI: "
include/acpi/battery.h new file +21 -0
@@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __ACPI_BATTERY_H +#define __ACPI_BATTERY_H + +#define ACPI_BATTERY_CLASS "battery" + +#define ACPI_BATTERY_NOTIFY_STATUS 0x80 +#define ACPI_BATTERY_NOTIFY_INFO 0x81 +#define ACPI_BATTERY_NOTIFY_THRESHOLD 0x82 + +struct acpi_battery_hook { +const char *name; +int (*add_battery)(struct power_supply *battery); +int (*remove_battery)(struct power_supply *battery); +struct list_head list; +}; + +void battery_hook_register (struct acpi_battery_hook *hook); +void battery_hook_unregister (struct acpi_battery_hook *hook); + +#endif
Signature · 1 line
2.7.4
Andy Shevchenko
Quoted 19 lines · Ognjen Galic
This is a patch that implements a generic hooking API for the generic ACPI battery driver.
With this new generic API, drivers can expose platform specific behaviour via sysfs attributes in
/sys/class/power_supply/BATn/in a generic way.A perfect example of the need for this API are Lenovo ThinkPads.
Lenovo ThinkPads have a ACPI extension that allows the setting of start and stop charge thresholds in the EC and battery firmware via ACPI. The
thinkpad_acpimodule can use this API to expose sysfs attributes that it controls inside the ACPI battery driver sysfs tree, under/sys/class/power_supply/BATN/.The file
drivers/acpi/battery.hhas been moved toinclude/acpi/battery.hand the includes insideac.c,sbs.c, andbattery.chave been adjusted to reflect that.
Quoted 5 lines
When drivers hooks into the API, the API calls
add_battery()for each battery in the system that passes it aacpi_batterystruct. Then, the drivers can usedevice_create_file()to create new sysfs attributes with that struct and identify the batteries for per-battery attributes.
Thanks for an update. Few comments below.
(Keep in mind, please, that we are not expecting new version immediately, and Rafael might also apply this one without changes, since they are minors)
drivers/acpi/battery.c line 34
Quoted 11 lines
#include <
linux/kernel.h>
#include <linux/module.h>drivers/acpi/battery.c +2
-0+#include <linux/mutex.h> #include <linux/init.h> #include <linux/types.h> #include <linux/jiffies.h> @@ -30,6 +31,7 @@#include <linux/dmi.h> #include <linux/delay.h> #include <linux/slab.h> +#include <linux/list.h>
Just a nit: perhaps add this before module.h ?
drivers/acpi/battery.c static const struct device_attribute alarm_attr = { · line 655
Quoted 11 lines
drivers/acpi/battery.c +11
-0+void __battery_hook_unregister (struct acpi_battery_hook *hook,int force) +{ +struct list_head *position; +struct acpi_battery *battery; + +/* +* In order to remove a hook, we first need to +* de-register all the batteries that are registered. +*/ + +if (!force)
I'm not sure force is a good name here, perhaps "locked"? Btw, would it be boolean?
drivers/acpi/battery.c static const struct device_attribute alarm_attr = { · line 661
Quoted 6 lines
drivers/acpi/battery.c +5
-0+mutex_lock (&hook_mutex); +list_for_each (position, &acpi_battery_list) { + battery = list_entry(position, struct acpi_battery, list); + hook->remove_battery (battery->bat); + }
list_for_each_entry()
Quoted 6 lines
drivers/acpi/battery.c +6
-0+ +/* Then, just remove the hook */ + +list_del (&hook->list); + +if (!force)
drivers/acpi/battery.c static const struct device_attribute alarm_attr = { · line 695
Quoted 10 lines
drivers/acpi/battery.c +8
-0+mutex_unlock (&hook_mutex); + +pr_info ("battery: extension unregistered: %s\n" , hook->name); +} +void battery_hook_register (struct acpi_battery_hook *hook) +{ +list_for_each (position, &acpi_battery_list) { + battery = list_entry(position, struct acpi_battery, list);
list_for_each_entry()
drivers/acpi/battery.c static const struct device_attribute alarm_attr = { · line 708
Quoted 3 lines
drivers/acpi/battery.c +3
-0+__battery_hook_unregister (hook,1 ); +return ; +
Perhaps you meant empty line followed by return statement.
drivers/acpi/battery.c static const struct device_attribute alarm_attr = { · line 712
Quoted 4 lines
drivers/acpi/battery.c +4
-0+ } + } + +pr_info ("battery: new extension: %s\n" , hook->name);
Perhaps move "battery:" to pr_fmt() ?
I have counted already 3 prints with it and see more below.
drivers/acpi/battery.c static const struct device_attribute alarm_attr = { · line 742
Quoted 2 lines
drivers/acpi/battery.c +2
-0+list_for_each (position, &battery_hook_list) { + hook_node = list_entry(position, struct acpi_battery_hook, list);
list_for_each_entry()
Review
Quoted 2 lines
(quoted diff) +2
-0+ list_for_each(position, &battery_hook_list) { + hook = list_entry(position, struct acpi_battery_hook, list);
Ditto.
drivers/acpi/battery.c static const struct device_attribute alarm_attr = { · line 791
Quoted 2 lines
drivers/acpi/battery.c +2
-0+list_for_each_safe (position, temp, &battery_hook_list) { + hook = list_entry(position, struct acpi_battery_hook, list);
Ditto.
drivers/acpi/battery.h
Quoted 2 lines
drivers/acpi/battery.h deleted +0
-0
Apparently you didn't apply -M -C (perhaps with some non-default threshold) to avoid such thing in the patch.
include/acpi/battery.h
Quoted 2 lines
include/acpi/battery.h new file +0
-0
Ditto.
Signature · 2 lines
With Best Regards, Andy Shevchenko