diff mbox series

[3/5] ACPI: battery: Allow battery hooks to be registered multiple times.

Message ID 20220912125342.7395-4-W_Armin@gmx.de
State New
Headers show
Series platform/x86: dell: Add new dell-wmi-ddv driver | expand

Commit Message

Armin Wolf Sept. 12, 2022, 12:53 p.m. UTC 
Registering multiple instances of a battery hook is beneficial
for drivers which can be instantiated multiple times. Until now,
this would mean that such a driver would have to implement some
logic to manage battery hooks.

Extend the battery hook handling instead.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/battery.c               | 21 ++++++++++++++++-----
 drivers/platform/x86/asus-wmi.c      | 15 ++++++---------
 drivers/platform/x86/huawei-wmi.c    | 11 +++++++----
 drivers/platform/x86/lg-laptop.c     | 10 ++++++----
 drivers/platform/x86/system76_acpi.c | 18 ++++++++++--------
 drivers/platform/x86/thinkpad_acpi.c | 11 +++++++----
 include/acpi/battery.h               | 11 ++++++++---
 7 files changed, 60 insertions(+), 37 deletions(-)

--
2.30.2

Comments

Barnabás Pőcze Sept. 12, 2022, 4:42 p.m. UTC | #1 
Hi

2022. szeptember 12., hétfő 14:53 keltezéssel, Armin Wolf írta:

> Registering multiple instances of a battery hook is beneficial
> for drivers which can be instantiated multiple times. Until now,
> this would mean that such a driver would have to implement some
> logic to manage battery hooks.
> 
> Extend the battery hook handling instead.

I think this is already possible by embedding the acpi_battery_hook
object inside the driver's device specific data object, no?

Regards,
Barnabás Pőcze


> [...]
Armin Wolf Sept. 12, 2022, 5:29 p.m. UTC | #2 
Am 12.09.22 um 18:42 schrieb Barnabás Pőcze:

> Hi
>
> 2022. szeptember 12., hétfő 14:53 keltezéssel, Armin Wolf írta:
>
>> Registering multiple instances of a battery hook is beneficial
>> for drivers which can be instantiated multiple times. Until now,
>> this would mean that such a driver would have to implement some
>> logic to manage battery hooks.
>>
>> Extend the battery hook handling instead.
> I think this is already possible by embedding the acpi_battery_hook
> object inside the driver's device specific data object, no?
>
> Regards,
> Barnabás Pőcze
>
>
Yes, it indeed is. However afaik it is not possible to pass instance-specific
data to such an embedded battery hook. It could be possible by passing the
battery hook as an argument to add_battery()/remove_battery() and using container_of(),
but in my opinion this would be too much of a quick hack.

>> [...]
Hans de Goede Sept. 19, 2022, 11:18 a.m. UTC | #3 
Hi,

On 9/12/22 18:29, Armin Wolf wrote:
> Am 12.09.22 um 18:42 schrieb Barnabás Pőcze:
> 
>> Hi
>>
>> 2022. szeptember 12., hétfő 14:53 keltezéssel, Armin Wolf írta:
>>
>>> Registering multiple instances of a battery hook is beneficial
>>> for drivers which can be instantiated multiple times. Until now,
>>> this would mean that such a driver would have to implement some
>>> logic to manage battery hooks.
>>>
>>> Extend the battery hook handling instead.
>> I think this is already possible by embedding the acpi_battery_hook
>> object inside the driver's device specific data object, no?
>>
>> Regards,
>> Barnabás Pőcze
>>
>>
> Yes, it indeed is. However afaik it is not possible to pass instance-specific
> data to such an embedded battery hook. It could be possible by passing the
> battery hook as an argument to add_battery()/remove_battery() and using container_of(),
> but in my opinion this would be too much of a quick hack.

Actually thinking more about this (after my reviewed-by of 4/5) I believe
that leaving the lifetime management of the struct acpi_battery_hook hook
in the caller and then modifying 4/4 to pass the hook to the callback,
so that the callback can indeed do container_of to get its top-level
driver-data struct would be a better/cleaner solution then this patch +
patch 4/5 .

Doing things this way is quite normal in the kernel and doing a single
large alloc is better then a bunch of small allocs. In this case it does
not really matter, but if we do things like this over all drivers
eventually all the small mallocs add up.

Doing things this way would also reduce the amount of churn in this
series a bit.

Regards,

Hans
Barnabás Pőcze Sept. 19, 2022, 5:42 p.m. UTC | #4 
Hi

2022. szeptember 12., hétfő 19:29 keltezéssel, Armin Wolf <W_Armin@gmx.de> írta:

> Am 12.09.22 um 18:42 schrieb Barnabás Pőcze:
> 
> > Hi
> > 
> > 2022. szeptember 12., hétfő 14:53 keltezéssel, Armin Wolf írta:
> > 
> > > Registering multiple instances of a battery hook is beneficial
> > > for drivers which can be instantiated multiple times. Until now,
> > > this would mean that such a driver would have to implement some
> > > logic to manage battery hooks.
> > > 
> > > Extend the battery hook handling instead.
> > > I think this is already possible by embedding the acpi_battery_hook
> > > object inside the driver's device specific data object, no?
> > 
> > Regards,
> > Barnabás Pőcze
> 
> Yes, it indeed is. However afaik it is not possible to pass instance-specific
> data to such an embedded battery hook. It could be possible by passing the
> battery hook as an argument to add_battery()/remove_battery() and using container_of(),
> but in my opinion this would be too much of a quick hack.

Good point about the instance-specific data. However, regarding the second point,
I am with Hans. I do not really think it is that big of a hack. It is inheritance.


Regards,
Barnabás Pőcze
diff mbox series

Patch

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 4aea65f3d8c3..15bb5d868a56 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -696,19 +696,28 @@  void battery_hook_unregister(struct acpi_battery_hook *hook)
 	mutex_lock(&hook_mutex);

 	list_for_each_entry(battery, &acpi_battery_list, list) {
-		hook->remove_battery(battery->bat);
+		hook->ops->remove_battery(battery->bat);
 	}
 	list_del(&hook->list);

 	mutex_unlock(&hook_mutex);
 	pr_info("extension unregistered: %s\n", hook->name);
+	kfree(hook);
 }
 EXPORT_SYMBOL_GPL(battery_hook_unregister);

-void battery_hook_register(struct acpi_battery_hook *hook)
+struct acpi_battery_hook *battery_hook_register(const char *name,
+						const struct acpi_battery_hook_ops *ops)
 {
+	struct acpi_battery_hook *hook = kzalloc(sizeof(*hook), GFP_KERNEL);
 	struct acpi_battery *battery;

+	if (!hook)
+		return ERR_PTR(-ENOMEM);
+
+	hook->name = name;
+	hook->ops = ops;
+
 	mutex_lock(&hook_mutex);
 	INIT_LIST_HEAD(&hook->list);
 	list_add(&hook->list, &battery_hook_list);
@@ -719,11 +728,13 @@  void battery_hook_register(struct acpi_battery_hook *hook)
 	 * its attributes.
 	 */
 	list_for_each_entry(battery, &acpi_battery_list, list) {
-		hook->add_battery(battery->bat);
+		hook->ops->add_battery(battery->bat);
 	}
 	pr_info("new extension: %s\n", hook->name);

 	mutex_unlock(&hook_mutex);
+
+	return hook;
 }
 EXPORT_SYMBOL_GPL(battery_hook_register);

@@ -747,7 +758,7 @@  static void battery_hook_add_battery(struct acpi_battery *battery)
 	 * during the battery module initialization.
 	 */
 	list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
-		hook_node->add_battery(battery->bat);
+		hook_node->ops->add_battery(battery->bat);
 	}
 	mutex_unlock(&hook_mutex);
 }
@@ -762,7 +773,7 @@  static void battery_hook_remove_battery(struct acpi_battery *battery)
 	 * custom attributes from the battery.
 	 */
 	list_for_each_entry(hook, &battery_hook_list, list) {
-		hook->remove_battery(battery->bat);
+		hook->ops->remove_battery(battery->bat);
 	}
 	/* Then, just remove the battery from the list */
 	list_del(&battery->list);
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index d95170b7dba0..37d8649418f4 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -252,8 +252,8 @@  struct asus_wmi {
 	struct platform_profile_handler platform_profile_handler;
 	bool platform_profile_support;

-	// The RSOC controls the maximum charging percentage.
-	bool battery_rsoc_available;
+	// The RSOC battery hook controls the maximum charging percentage.
+	struct acpi_battery_hook *hook;

 	bool panel_overdrive_available;

@@ -916,25 +916,22 @@  static int asus_wmi_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook battery_hook = {
+static const struct acpi_battery_hook_ops battery_hook_ops = {
 	.add_battery = asus_wmi_battery_add,
 	.remove_battery = asus_wmi_battery_remove,
-	.name = "ASUS Battery Extension",
 };

 static void asus_wmi_battery_init(struct asus_wmi *asus)
 {
-	asus->battery_rsoc_available = false;
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_RSOC)) {
-		asus->battery_rsoc_available = true;
-		battery_hook_register(&battery_hook);
+		asus->hook = battery_hook_register("ASUS Battery Extension", &battery_hook_ops);
 	}
 }

 static void asus_wmi_battery_exit(struct asus_wmi *asus)
 {
-	if (asus->battery_rsoc_available)
-		battery_hook_unregister(&battery_hook);
+	if (!IS_ERR_OR_NULL(asus->hook))
+		battery_hook_unregister(asus->hook);
 }

 /* LEDs ***********************************************************************/
diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
index eac3e6b4ea11..6fd02b25a97b 100644
--- a/drivers/platform/x86/huawei-wmi.c
+++ b/drivers/platform/x86/huawei-wmi.c
@@ -62,6 +62,7 @@  struct huawei_wmi {
 	bool battery_available;
 	bool fn_lock_available;

+	struct acpi_battery_hook *hook;
 	struct huawei_wmi_debug debug;
 	struct input_dev *idev[2];
 	struct led_classdev cdev;
@@ -491,10 +492,9 @@  static int huawei_wmi_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook huawei_wmi_battery_hook = {
+static const struct acpi_battery_hook_ops huawei_wmi_battery_hook_ops = {
 	.add_battery = huawei_wmi_battery_add,
 	.remove_battery = huawei_wmi_battery_remove,
-	.name = "Huawei Battery Extension"
 };

 static void huawei_wmi_battery_setup(struct device *dev)
@@ -507,7 +507,8 @@  static void huawei_wmi_battery_setup(struct device *dev)
 		return;
 	}

-	battery_hook_register(&huawei_wmi_battery_hook);
+	huawei->hook = battery_hook_register("Huawei Battery Extension",
+					     &huawei_wmi_battery_hook_ops);
 	device_create_file(dev, &dev_attr_charge_control_thresholds);
 }

@@ -516,7 +517,9 @@  static void huawei_wmi_battery_exit(struct device *dev)
 	struct huawei_wmi *huawei = dev_get_drvdata(dev);

 	if (huawei->battery_available) {
-		battery_hook_unregister(&huawei_wmi_battery_hook);
+		if (!IS_ERR(huawei->hook))
+			battery_hook_unregister(huawei->hook);
+
 		device_remove_file(dev, &dev_attr_charge_control_thresholds);
 	}
 }
diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
index 332868b140ed..d8a61a07313e 100644
--- a/drivers/platform/x86/lg-laptop.c
+++ b/drivers/platform/x86/lg-laptop.c
@@ -73,6 +73,7 @@  static u32 inited;
 #define INIT_SPARSE_KEYMAP      0x80

 static int battery_limit_use_wmbb;
+static struct acpi_battery_hook *hook;
 static struct led_classdev kbd_backlight;
 static enum led_brightness get_kbd_backlight_level(void);

@@ -562,10 +563,9 @@  static int lg_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook battery_hook = {
+static const struct acpi_battery_hook_ops battery_hook_ops = {
 	.add_battery = lg_battery_add,
 	.remove_battery = lg_battery_remove,
-	.name = "LG Battery Extension",
 };

 static struct attribute *dev_attributes[] = {
@@ -750,7 +750,7 @@  static int acpi_add(struct acpi_device *device)
 	led_classdev_register(&pf_device->dev, &tpad_led);

 	wmi_input_setup();
-	battery_hook_register(&battery_hook);
+	hook = battery_hook_register("LG Battery Extension", &battery_hook_ops);

 	return 0;

@@ -768,7 +768,9 @@  static int acpi_remove(struct acpi_device *device)
 	led_classdev_unregister(&tpad_led);
 	led_classdev_unregister(&kbd_backlight);

-	battery_hook_unregister(&battery_hook);
+	if (!IS_ERR(hook))
+		battery_hook_unregister(hook);
+
 	wmi_input_destroy();
 	platform_device_unregister(pf_device);
 	pf_device = NULL;
diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index 958df41ad509..1ec22db32bd0 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -26,6 +26,7 @@ 

 struct system76_data {
 	struct acpi_device *acpi_dev;
+	struct acpi_battery_hook *hook;
 	struct led_classdev ap_led;
 	struct led_classdev kb_led;
 	enum led_brightness kb_brightness;
@@ -272,20 +273,21 @@  static int system76_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook system76_battery_hook = {
+static const struct acpi_battery_hook_ops system76_battery_hook_ops = {
 	.add_battery = system76_battery_add,
 	.remove_battery = system76_battery_remove,
-	.name = "System76 Battery Extension",
 };

-static void system76_battery_init(void)
+static void system76_battery_init(struct system76_data *data)
 {
-	battery_hook_register(&system76_battery_hook);
+	data->hook = battery_hook_register("System76 Battery Extension",
+					   &system76_battery_hook_ops);
 }

-static void system76_battery_exit(void)
+static void system76_battery_exit(struct system76_data *data)
 {
-	battery_hook_unregister(&system76_battery_hook);
+	if (!IS_ERR(data->hook))
+		battery_hook_unregister(data->hook);
 }

 // Get the airplane mode LED brightness
@@ -730,7 +732,7 @@  static int system76_add(struct acpi_device *acpi_dev)
 		if (err)
 			goto error;

-		system76_battery_init();
+		system76_battery_init(data);
 	}

 	return 0;
@@ -751,7 +753,7 @@  static int system76_remove(struct acpi_device *acpi_dev)
 	data = acpi_driver_data(acpi_dev);

 	if (data->has_open_ec) {
-		system76_battery_exit();
+		system76_battery_exit(data);
 		kfree(data->nfan);
 		kfree(data->ntmp);
 	}
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 8fbe21ebcc52..8adafc3c31fa 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9407,6 +9407,7 @@  struct tpacpi_battery_data {

 struct tpacpi_battery_driver_data {
 	struct tpacpi_battery_data batteries[3];
+	struct acpi_battery_hook *hook;
 	int individual_addressing;
 };

@@ -9914,10 +9915,9 @@  static int tpacpi_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook battery_hook = {
+static const struct acpi_battery_hook_ops battery_hook_ops = {
 	.add_battery = tpacpi_battery_add,
 	.remove_battery = tpacpi_battery_remove,
-	.name = "ThinkPad Battery Extension",
 };

 /* Subdriver init/exit */
@@ -9943,13 +9943,16 @@  static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
 					battery_quirk_table,
 					ARRAY_SIZE(battery_quirk_table));

-	battery_hook_register(&battery_hook);
+	battery_info.hook = battery_hook_register("ThinkPad Battery Extension",
+						  &battery_hook_ops);
+
 	return 0;
 }

 static void tpacpi_battery_exit(void)
 {
-	battery_hook_unregister(&battery_hook);
+	if (!IS_ERR(battery_info.hook))
+		battery_hook_unregister(battery_info.hook);
 }

 static struct ibm_struct battery_driver_data = {
diff --git a/include/acpi/battery.h b/include/acpi/battery.h
index b8d56b702c7a..b3c81abada1e 100644
--- a/include/acpi/battery.h
+++ b/include/acpi/battery.h
@@ -10,14 +10,19 @@ 
 #define ACPI_BATTERY_NOTIFY_INFO	0x81
 #define ACPI_BATTERY_NOTIFY_THRESHOLD   0x82

-struct acpi_battery_hook {
-	const char *name;
+struct acpi_battery_hook_ops {
 	int (*add_battery)(struct power_supply *battery);
 	int (*remove_battery)(struct power_supply *battery);
+};
+
+struct acpi_battery_hook {
+	const char *name;
+	const struct acpi_battery_hook_ops *ops;
 	struct list_head list;
 };

-void battery_hook_register(struct acpi_battery_hook *hook);
+struct acpi_battery_hook *battery_hook_register(const char *name,
+						const struct acpi_battery_hook_ops *hook);
 void battery_hook_unregister(struct acpi_battery_hook *hook);

 #endif