[PATCH 0/2] hwmon: (applesmc) add native charge_control_end_threshold support

[PATCH 0/2] hwmon: (applesmc) add native charge_control_end_threshold support

[Jordan Brough]

This series adds native charge-limit support to applesmc, so that
generic userspace (UPower, KDE Powerdevil, GNOME Settings) can read
and write a MacBook's charging cutoff through the standard
charge_control_end_threshold sysfs attribute, with no vendor-specific
userspace daemon required.

Patch 1 is a prerequisite: some Macs register their battery through
the ACPI Smart Battery System driver (drivers/acpi/sbs.c) rather than
the ACPI Control Method Battery driver, which is the only one with an
existing hook mechanism for attaching extra power_supply properties.
Patch 1 adds a parallel, purely additive hook mechanism to sbs.c so
patch 2 can support both registration paths symmetrically.

Patch 2 adds the actual charge_control_end_threshold support to
applesmc, using the real SMC BCLM key (not the cosmetic BFCL key,
which only controls the MagSafe LED color and does not affect
charging behavior).

Tested on a MacBookAir6,2 (SBS-registered battery): sysfs attribute
appears, reads/writes round-trip correctly to the real SMC BCLM key,
and this was verified end-to-end through KDE's charge-limit slider
writing directly to sysfs. Also tested on a non-Mac laptop (CmBatt-
registered battery, no BCLM key) to confirm both new hook
registrations are true no-ops there, with no new sysfs attributes and
no behavioral change.

This was previously discussed on this list in 2020, proposed as a
generic sysfs interface for reading/writing arbitrary SMC keys by
name. Guenter raised concerns at the time about that approach adding
non-standard code to the driver:

  Link: https://www.spinics.net/lists/linux-hwmon/msg09839.html

This series takes a different approach: it exposes only the specific,
well-understood BCLM key through the standard power_supply_ext
charge_control_end_threshold property, rather than a generic raw-key
interface, and gates it at runtime on a probe of that specific key so
it stays inert on any hardware that doesn't implement it.

This is my first kernel submission; I'd welcome any correction on
process or style along with the technical review.

Jordan Brough (2):
  ACPI: sbs: add battery hook mechanism for SBS-registered batteries
  hwmon: (applesmc) add charge_control_end_threshold support

 drivers/acpi/sbs.c       | 147 ++++++++++++++++++++++++++++++++++-
 drivers/hwmon/applesmc.c | 164 +++++++++++++++++++++++++++++++++++++--
 include/acpi/battery.h   |  16 ++++
 3 files changed, 321 insertions(+), 6 deletions(-)


base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
-- 
2.55.0

[PATCH 1/2] ACPI: sbs: add battery hook mechanism for SBS-registered batteries

[Jordan Brough]

drivers/acpi/battery.c provides a battery_hook_register()/unregister()
mechanism that lets other drivers (e.g. hwmon drivers exposing vendor-
specific charge control) attach extra power_supply properties to an
ACPI Control Method Battery (HID PNP0C0A, "CmBatt").

Some machines instead expose their battery through the ACPI Smart
Battery System (HID ACPI0002, "SBS") driver in drivers/acpi/sbs.c,
which has no equivalent hook point. On these machines a hwmon driver
has no supported way to attach additional properties to the battery's
power_supply device.

Add a parallel sbs_battery_hook_register()/unregister() mechanism to
drivers/acpi/sbs.c, mirroring the existing CmBatt hook API in name and
behavior so that callers can support both battery registration paths
symmetrically. This is purely additive: no existing hook consumer or
code path in drivers/acpi/battery.c is touched, and sbs.c's own
behavior is unchanged for any driver that does not call the new API.

This is a prerequisite for hwmon/applesmc gaining
charge_control_end_threshold support, since the Intel MacBook hardware
being targeted registers its battery via SBS, not CmBatt.

Signed-off-by: Jordan Brough <jordan@brough.org>
---
 drivers/acpi/sbs.c     | 147 ++++++++++++++++++++++++++++++++++++++++-
 include/acpi/battery.h |  16 +++++
 2 files changed, 162 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index 86b7c797585..305bf62eae3 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -10,6 +10,7 @@
 #define pr_fmt(fmt) "ACPI: " fmt
 
 #include <linux/init.h>
+#include <linux/list.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -54,6 +55,7 @@ struct acpi_battery {
 	struct power_supply *bat;
 	struct power_supply_desc bat_desc;
 	struct acpi_sbs *sbs;
+	struct list_head list;		/* node in sbs_hook_battery_list */
 	unsigned long update_time;
 	char name[8];
 	char manufacturer_name[ACPI_SBS_BLOCK_MAX];
@@ -518,6 +520,145 @@ static int acpi_battery_read(struct acpi_battery *battery)
 	return result;
 }
 
+/*
+ * Battery hook support, mirroring the mechanism in drivers/acpi/battery.c
+ * for batteries registered there. That mechanism only sees batteries
+ * added via drivers/acpi/battery.c, not the ones this driver registers,
+ * so drivers wanting to attach optional functionality (e.g. an extra
+ * power_supply_ext) to an SBS battery need this separate copy; see the
+ * comment above the declarations in include/acpi/battery.h.
+ */
+static LIST_HEAD(sbs_hook_list);
+static LIST_HEAD(sbs_hook_battery_list);
+static DEFINE_MUTEX(sbs_hook_mutex);
+
+static void sbs_battery_hook_unregister_unlocked(struct acpi_battery_hook *hook)
+{
+	struct acpi_battery *battery;
+
+	/*
+	 * In order to remove a hook, we first need to
+	 * de-register all the batteries that are registered.
+	 */
+	list_for_each_entry(battery, &sbs_hook_battery_list, list) {
+		if (!hook->remove_battery(battery->bat, hook))
+			power_supply_changed(battery->bat);
+	}
+	list_del_init(&hook->list);
+}
+
+void sbs_battery_hook_unregister(struct acpi_battery_hook *hook)
+{
+	mutex_lock(&sbs_hook_mutex);
+	/*
+	 * Ignore already unregistered battery hooks. This might happen
+	 * if a battery hook was previously unloaded due to an error when
+	 * adding a new battery.
+	 */
+	if (!list_empty(&hook->list))
+		sbs_battery_hook_unregister_unlocked(hook);
+	mutex_unlock(&sbs_hook_mutex);
+}
+EXPORT_SYMBOL_GPL(sbs_battery_hook_unregister);
+
+void sbs_battery_hook_register(struct acpi_battery_hook *hook)
+{
+	struct acpi_battery *battery;
+
+	mutex_lock(&sbs_hook_mutex);
+	list_add(&hook->list, &sbs_hook_list);
+	/*
+	 * Now that the driver is registered, we need
+	 * to notify the hook that a battery is available
+	 * for each battery, so that the driver may add
+	 * its attributes.
+	 */
+	list_for_each_entry(battery, &sbs_hook_battery_list, list) {
+		if (hook->add_battery(battery->bat, hook)) {
+			/*
+			 * If a add-battery returns non-zero,
+			 * the registration of the hook has failed,
+			 * and we will not add it to the list of loaded
+			 * hooks.
+			 */
+			pr_err("hook failed to load: %s", hook->name);
+			sbs_battery_hook_unregister_unlocked(hook);
+			goto end;
+		}
+
+		power_supply_changed(battery->bat);
+	}
+	pr_info("new hook: %s\n", hook->name);
+end:
+	mutex_unlock(&sbs_hook_mutex);
+}
+EXPORT_SYMBOL_GPL(sbs_battery_hook_register);
+
+static void devm_sbs_battery_hook_unregister(void *data)
+{
+	struct acpi_battery_hook *hook = data;
+
+	sbs_battery_hook_unregister(hook);
+}
+
+int devm_sbs_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook)
+{
+	sbs_battery_hook_register(hook);
+
+	return devm_add_action_or_reset(dev, devm_sbs_battery_hook_unregister, hook);
+}
+EXPORT_SYMBOL_GPL(devm_sbs_battery_hook_register);
+
+/*
+ * This function gets called right after the battery sysfs
+ * attributes have been added, so that the drivers that
+ * define custom sysfs attributes can add their own.
+ */
+static void sbs_battery_hook_add_battery(struct acpi_battery *battery)
+{
+	struct acpi_battery_hook *hook_node, *tmp;
+
+	mutex_lock(&sbs_hook_mutex);
+	INIT_LIST_HEAD(&battery->list);
+	list_add(&battery->list, &sbs_hook_battery_list);
+	/*
+	 * Since we added a new battery to the list, we need to
+	 * iterate over the hooks and call add_battery for each
+	 * hook that was registered. This usually happens
+	 * when a battery gets hotplugged or initialized
+	 * during the battery module initialization.
+	 */
+	list_for_each_entry_safe(hook_node, tmp, &sbs_hook_list, list) {
+		if (hook_node->add_battery(battery->bat, hook_node)) {
+			/*
+			 * The notification of the hook has failed, to
+			 * prevent further errors we will unload the hook.
+			 */
+			pr_err("error in hook, unloading: %s",
+			       hook_node->name);
+			sbs_battery_hook_unregister_unlocked(hook_node);
+		}
+	}
+	mutex_unlock(&sbs_hook_mutex);
+}
+
+static void sbs_battery_hook_remove_battery(struct acpi_battery *battery)
+{
+	struct acpi_battery_hook *hook;
+
+	mutex_lock(&sbs_hook_mutex);
+	/*
+	 * Before removing the hook, we need to remove all
+	 * custom attributes from the battery.
+	 */
+	list_for_each_entry(hook, &sbs_hook_list, list) {
+		hook->remove_battery(battery->bat, hook);
+	}
+	/* Then, just remove the battery from the list */
+	list_del(&battery->list);
+	mutex_unlock(&sbs_hook_mutex);
+}
+
 /* Smart Battery */
 static int acpi_battery_add(struct acpi_sbs *sbs, int id)
 {
@@ -555,6 +696,8 @@ static int acpi_battery_add(struct acpi_sbs *sbs, int id)
 		goto end;
 	}
 
+	sbs_battery_hook_add_battery(battery);
+
       end:
 	pr_info("%s [%s]: Battery Slot [%s] (battery %s)\n",
 	       ACPI_SBS_DEVICE_NAME, acpi_device_bid(sbs->device),
@@ -566,8 +709,10 @@ static void acpi_battery_remove(struct acpi_sbs *sbs, int id)
 {
 	struct acpi_battery *battery = &sbs->battery[id];
 
-	if (battery->bat)
+	if (battery->bat) {
+		sbs_battery_hook_remove_battery(battery);
 		power_supply_unregister(battery->bat);
+	}
 }
 
 static int acpi_charger_add(struct acpi_sbs *sbs)
diff --git a/include/acpi/battery.h b/include/acpi/battery.h
index c93f16dfb94..ddbfa4ed135 100644
--- a/include/acpi/battery.h
+++ b/include/acpi/battery.h
@@ -18,8 +18,24 @@ struct acpi_battery_hook {
 	struct list_head list;
 };
 
+/*
+ * battery_hook_register() and friends only see batteries registered by
+ * drivers/acpi/battery.c, the ACPI Control Method Battery driver (ACPI HID
+ * "PNP0C0A"). Batteries registered by drivers/acpi/sbs.c, the ACPI Smart
+ * Battery System driver (ACPI HID "ACPI0002", common on hardware with
+ * SMBus/SBS fuel-gauge chips such as many Intel MacBooks), are invisible
+ * to them; use the sbs_battery_hook_* equivalents below for those. A
+ * caller wanting to support both kinds of hardware needs two separate
+ * struct acpi_battery_hook instances, one per registration call, since a
+ * given instance's embedded list node can only belong to one list at a
+ * time.
+ */
 void battery_hook_register(struct acpi_battery_hook *hook);
 void battery_hook_unregister(struct acpi_battery_hook *hook);
 int devm_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
 
+void sbs_battery_hook_register(struct acpi_battery_hook *hook);
+void sbs_battery_hook_unregister(struct acpi_battery_hook *hook);
+int devm_sbs_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
+
 #endif
-- 
2.55.0

[PATCH 2/2] hwmon: (applesmc) add charge_control_end_threshold support

[Jordan Brough]

Apple's SMC firmware exposes a real charging-cutoff control via the
BCLM (Battery Charge Limit Max) key, and a separate cosmetic BFCL
(Battery Final Charge Level) key that only controls when the MagSafe
LED switches from orange to green. Userspace tools and out-of-tree
kernel patches have historically conflated the two or exposed neither
through a standard interface, leaving desktop environments unable to
offer a native charge-limit control on Intel MacBooks.

Add native charge_control_end_threshold support to applesmc using the
power_supply_ext extension API, so that generic userspace (UPower,
KDE Powerdevil, GNOME Settings) can read and write the threshold
through the standard sysfs attribute with no vendor-specific daemon.
BFCL is kept in sync automatically (BCLM - 5) purely so the MagSafe
LED still changes color at a sensible point relative to the real
cutoff; it is not independently exposed.

Apple MacBooks register their battery through either the ACPI Control
Method Battery driver (CmBatt, drivers/acpi/battery.c) or the ACPI
Smart Battery System driver (SBS, drivers/acpi/sbs.c), depending on
model. Register against both hook mechanisms, guarded at compile/link
time with IS_REACHABLE() so this stays a no-op when the corresponding
subsystem isn't built in, and at runtime with a probe of the BCLM SMC
key so this is a no-op on any Mac (or non-Mac, given applesmc's DMI
match table) that doesn't actually implement it.

Tested on a MacBookAir6,2 (SBS-registered battery): the sysfs
attribute appears, reads/writes correctly propagate to the real SMC
BCLM key end-to-end via KDE's charge-limit slider, and BFCL tracks
BCLM - 5 as designed. Tested on a non-Mac laptop (CmBatt-registered
battery, no BCLM key) to confirm both new hook registrations are
true no-ops there: no new sysfs attributes, no behavioral change,
clean module load/unload.

Signed-off-by: Jordan Brough <jordan@brough.org>
---
 drivers/hwmon/applesmc.c | 164 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 159 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index d0baa10502f..39276cdba3d 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -33,6 +33,8 @@
 #include <linux/workqueue.h>
 #include <linux/err.h>
 #include <linux/bits.h>
+#include <linux/power_supply.h>
+#include <acpi/battery.h>
 #include <asm/barrier.h>
 
 /* data port used by Apple SMC */
@@ -76,6 +78,26 @@
 
 #define TEMP_SENSOR_TYPE	"sp78"
 
+/*
+ * BCLM caps charging at a percentage; it is the only key that affects
+ * charging behavior. BFCL only controls when the charging LED switches
+ * from orange to green and has no effect on charging itself.
+ */
+#define BATTERY_CHARGE_LIMIT_KEY	"BCLM" /* r/w ui8 */
+#define BATTERY_CHARGE_LIMIT_LED_KEY	"BFCL" /* r/w ui8 */
+
+/*
+ * Margin kept between BCLM and BFCL so the charging LED still reports
+ * "done" once BCLM halts charging below 100%. Left equal to BCLM, the
+ * two can race: charging can stop a moment before the SMC's internal
+ * (sub-percent) charge counter actually reaches BFCL, leaving the LED
+ * stuck on "charging" indefinitely since no further current ever flows
+ * to push it over the threshold. A margin of 1 was sufficient to avoid
+ * this on a MacBookAir6,2; 5 matches Apple's own firmware convention and
+ * gives headroom on hardware we haven't tested.
+ */
+#define APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN 5
+
 /* List of keys used to read/write fan speeds */
 static const char *const fan_speed_fmt[] = {
 	"F%dAc",		/* actual speed */
@@ -131,6 +153,7 @@ static struct applesmc_registers {
 	int num_light_sensors;		/* number of light sensors */
 	bool has_accelerometer;		/* has motion sensor */
 	bool has_key_backlight;		/* has keyboard backlight */
+	bool has_battery_charge_limit;	/* has BCLM battery charge limit */
 	bool init_complete;		/* true when fully initialized */
 	struct applesmc_entry *cache;	/* cached key entries */
 	const char **index;		/* temperature key index */
@@ -633,17 +656,21 @@ static int applesmc_init_smcreg_try(void)
 	if (ret)
 		return ret;
 	ret = applesmc_has_key(BACKLIGHT_KEY, &s->has_key_backlight);
+	if (ret)
+		return ret;
+	ret = applesmc_has_key(BATTERY_CHARGE_LIMIT_KEY, &s->has_battery_charge_limit);
 	if (ret)
 		return ret;
 
 	s->num_light_sensors = left_light_sensor + right_light_sensor;
 	s->init_complete = true;
 
-	pr_info("key=%d fan=%d temp=%d index=%d acc=%d lux=%d kbd=%d\n",
-	       s->key_count, s->fan_count, s->temp_count, s->index_count,
-	       s->has_accelerometer,
-	       s->num_light_sensors,
-	       s->has_key_backlight);
+	pr_info("key=%d fan=%d temp=%d index=%d acc=%d lux=%d kbd=%d bat=%d\n",
+		s->key_count, s->fan_count, s->temp_count, s->index_count,
+		s->has_accelerometer,
+		s->num_light_sensors,
+		s->has_key_backlight,
+		s->has_battery_charge_limit);
 
 	return 0;
 }
@@ -724,6 +751,120 @@ static struct platform_driver applesmc_driver = {
 	},
 };
 
+static int applesmc_bat_get_property(struct power_supply *psy,
+				     const struct power_supply_ext *ext,
+				     void *data,
+				     enum power_supply_property psp,
+				     union power_supply_propval *val)
+{
+	u8 limit;
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		ret = applesmc_read_key(BATTERY_CHARGE_LIMIT_KEY, &limit, 1);
+		if (ret)
+			return ret;
+		val->intval = limit;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int applesmc_bat_set_property(struct power_supply *psy,
+				     const struct power_supply_ext *ext,
+				     void *data,
+				     enum power_supply_property psp,
+				     const union power_supply_propval *val)
+{
+	u8 limit, led_limit;
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		if (val->intval < 20 || val->intval > 100)
+			return -EINVAL;
+
+		limit = val->intval;
+		ret = applesmc_write_key(BATTERY_CHARGE_LIMIT_KEY, &limit, 1);
+		if (ret)
+			return ret;
+
+		led_limit = limit > APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN ?
+			limit - APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN : limit;
+		applesmc_write_key(BATTERY_CHARGE_LIMIT_LED_KEY, &led_limit, 1);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int applesmc_bat_property_is_writeable(struct power_supply *psy,
+					      const struct power_supply_ext *ext,
+					      void *data,
+					      enum power_supply_property psp)
+{
+	return psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD;
+}
+
+static const enum power_supply_property applesmc_bat_props[] = {
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
+};
+
+static const struct power_supply_ext applesmc_bat_ext = {
+	.name			= "applesmc-charge-control",
+	.properties		= applesmc_bat_props,
+	.num_properties		= ARRAY_SIZE(applesmc_bat_props),
+	.get_property		= applesmc_bat_get_property,
+	.set_property		= applesmc_bat_set_property,
+	.property_is_writeable	= applesmc_bat_property_is_writeable,
+};
+
+static struct power_supply *applesmc_hooked_battery;
+
+static int applesmc_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	if (applesmc_hooked_battery)
+		return 0;
+
+	applesmc_hooked_battery = battery;
+	return power_supply_register_extension(battery, &applesmc_bat_ext, &pdev->dev, NULL);
+}
+
+static int applesmc_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	if (applesmc_hooked_battery != battery)
+		return 0;
+
+	power_supply_unregister_extension(battery, &applesmc_bat_ext);
+	applesmc_hooked_battery = NULL;
+	return 0;
+}
+
+/*
+ * Two separate hook registrations are required: Intel Macs register
+ * their battery through either drivers/acpi/battery.c (ACPI Control
+ * Method Battery) or drivers/acpi/sbs.c (ACPI Smart Battery System,
+ * common on hardware with SMBus/SBS fuel-gauge chips), depending on
+ * model, and each mechanism only sees batteries registered through
+ * itself. A single struct acpi_battery_hook cannot be registered with
+ * both, since its embedded list node can only belong to one list at a
+ * time. Whichever mechanism matches the actual hardware will hook the
+ * battery; the other simply never finds one to call back for.
+ */
+static struct acpi_battery_hook applesmc_battery_hook = {
+	.name = "Apple SMC Battery Charge Control",
+	.add_battery = applesmc_battery_add,
+	.remove_battery = applesmc_battery_remove,
+};
+
+static struct acpi_battery_hook applesmc_sbs_battery_hook = {
+	.name = "Apple SMC Battery Charge Control (SBS)",
+	.add_battery = applesmc_battery_add,
+	.remove_battery = applesmc_battery_remove,
+};
+
 /*
  * applesmc_calibrate - Set our "resting" values.  Callers must
  * hold applesmc_lock.
@@ -1563,6 +1704,13 @@ static int __init applesmc_init(void)
 		goto out_light_ledclass;
 	}
 
+	if (smcreg.has_battery_charge_limit) {
+		if (IS_REACHABLE(CONFIG_ACPI_BATTERY))
+			battery_hook_register(&applesmc_battery_hook);
+		if (IS_REACHABLE(CONFIG_ACPI_SBS))
+			sbs_battery_hook_register(&applesmc_sbs_battery_hook);
+	}
+
 	return 0;
 
 out_light_ledclass:
@@ -1589,6 +1737,12 @@ static int __init applesmc_init(void)
 
 static void __exit applesmc_exit(void)
 {
+	if (smcreg.has_battery_charge_limit) {
+		if (IS_REACHABLE(CONFIG_ACPI_BATTERY))
+			battery_hook_unregister(&applesmc_battery_hook);
+		if (IS_REACHABLE(CONFIG_ACPI_SBS))
+			sbs_battery_hook_unregister(&applesmc_sbs_battery_hook);
+	}
 	hwmon_device_unregister(hwmon_dev);
 	applesmc_release_key_backlight();
 	applesmc_release_light_sensor();
-- 
2.55.0

Re: [PATCH 1/2] ACPI: sbs: add battery hook mechanism for SBS-registered batteries

[Rafael J. Wysocki (Intel)]

On Mon, Sep 14, 2026 at 1:14 AM Jordan Brough <jordan@brough.org> wrote:
>
> drivers/acpi/battery.c provides a battery_hook_register()/unregister()
> mechanism that lets other drivers (e.g. hwmon drivers exposing vendor-
> specific charge control) attach extra power_supply properties to an
> ACPI Control Method Battery (HID PNP0C0A, "CmBatt").
>
> Some machines instead expose their battery through the ACPI Smart
> Battery System (HID ACPI0002, "SBS") driver in drivers/acpi/sbs.c,
> which has no equivalent hook point. On these machines a hwmon driver
> has no supported way to attach additional properties to the battery's
> power_supply device.
>
> Add a parallel sbs_battery_hook_register()/unregister() mechanism to
> drivers/acpi/sbs.c, mirroring the existing CmBatt hook API in name and
> behavior so that callers can support both battery registration paths
> symmetrically. This is purely additive: no existing hook consumer or
> code path in drivers/acpi/battery.c is touched, and sbs.c's own
> behavior is unchanged for any driver that does not call the new API.
>
> This is a prerequisite for hwmon/applesmc gaining
> charge_control_end_threshold support, since the Intel MacBook hardware
> being targeted registers its battery via SBS, not CmBatt.
>
> Signed-off-by: Jordan Brough <jordan@brough.org>

Acked-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>

and feel free to route this patch along with the hwmon changes depending on it.

Thanks!

> ---
>  drivers/acpi/sbs.c     | 147 ++++++++++++++++++++++++++++++++++++++++-
>  include/acpi/battery.h |  16 +++++
>  2 files changed, 162 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
> index 86b7c797585..305bf62eae3 100644
> --- a/drivers/acpi/sbs.c
> +++ b/drivers/acpi/sbs.c
> @@ -10,6 +10,7 @@
>  #define pr_fmt(fmt) "ACPI: " fmt
>
>  #include <linux/init.h>
> +#include <linux/list.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> @@ -54,6 +55,7 @@ struct acpi_battery {
>         struct power_supply *bat;
>         struct power_supply_desc bat_desc;
>         struct acpi_sbs *sbs;
> +       struct list_head list;          /* node in sbs_hook_battery_list */
>         unsigned long update_time;
>         char name[8];
>         char manufacturer_name[ACPI_SBS_BLOCK_MAX];
> @@ -518,6 +520,145 @@ static int acpi_battery_read(struct acpi_battery *battery)
>         return result;
>  }
>
> +/*
> + * Battery hook support, mirroring the mechanism in drivers/acpi/battery.c
> + * for batteries registered there. That mechanism only sees batteries
> + * added via drivers/acpi/battery.c, not the ones this driver registers,
> + * so drivers wanting to attach optional functionality (e.g. an extra
> + * power_supply_ext) to an SBS battery need this separate copy; see the
> + * comment above the declarations in include/acpi/battery.h.
> + */
> +static LIST_HEAD(sbs_hook_list);
> +static LIST_HEAD(sbs_hook_battery_list);
> +static DEFINE_MUTEX(sbs_hook_mutex);
> +
> +static void sbs_battery_hook_unregister_unlocked(struct acpi_battery_hook *hook)
> +{
> +       struct acpi_battery *battery;
> +
> +       /*
> +        * In order to remove a hook, we first need to
> +        * de-register all the batteries that are registered.
> +        */
> +       list_for_each_entry(battery, &sbs_hook_battery_list, list) {
> +               if (!hook->remove_battery(battery->bat, hook))
> +                       power_supply_changed(battery->bat);
> +       }
> +       list_del_init(&hook->list);
> +}
> +
> +void sbs_battery_hook_unregister(struct acpi_battery_hook *hook)
> +{
> +       mutex_lock(&sbs_hook_mutex);
> +       /*
> +        * Ignore already unregistered battery hooks. This might happen
> +        * if a battery hook was previously unloaded due to an error when
> +        * adding a new battery.
> +        */
> +       if (!list_empty(&hook->list))
> +               sbs_battery_hook_unregister_unlocked(hook);
> +       mutex_unlock(&sbs_hook_mutex);
> +}
> +EXPORT_SYMBOL_GPL(sbs_battery_hook_unregister);
> +
> +void sbs_battery_hook_register(struct acpi_battery_hook *hook)
> +{
> +       struct acpi_battery *battery;
> +
> +       mutex_lock(&sbs_hook_mutex);
> +       list_add(&hook->list, &sbs_hook_list);
> +       /*
> +        * Now that the driver is registered, we need
> +        * to notify the hook that a battery is available
> +        * for each battery, so that the driver may add
> +        * its attributes.
> +        */
> +       list_for_each_entry(battery, &sbs_hook_battery_list, list) {
> +               if (hook->add_battery(battery->bat, hook)) {
> +                       /*
> +                        * If a add-battery returns non-zero,
> +                        * the registration of the hook has failed,
> +                        * and we will not add it to the list of loaded
> +                        * hooks.
> +                        */
> +                       pr_err("hook failed to load: %s", hook->name);
> +                       sbs_battery_hook_unregister_unlocked(hook);
> +                       goto end;
> +               }
> +
> +               power_supply_changed(battery->bat);
> +       }
> +       pr_info("new hook: %s\n", hook->name);
> +end:
> +       mutex_unlock(&sbs_hook_mutex);
> +}
> +EXPORT_SYMBOL_GPL(sbs_battery_hook_register);
> +
> +static void devm_sbs_battery_hook_unregister(void *data)
> +{
> +       struct acpi_battery_hook *hook = data;
> +
> +       sbs_battery_hook_unregister(hook);
> +}
> +
> +int devm_sbs_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook)
> +{
> +       sbs_battery_hook_register(hook);
> +
> +       return devm_add_action_or_reset(dev, devm_sbs_battery_hook_unregister, hook);
> +}
> +EXPORT_SYMBOL_GPL(devm_sbs_battery_hook_register);
> +
> +/*
> + * This function gets called right after the battery sysfs
> + * attributes have been added, so that the drivers that
> + * define custom sysfs attributes can add their own.
> + */
> +static void sbs_battery_hook_add_battery(struct acpi_battery *battery)
> +{
> +       struct acpi_battery_hook *hook_node, *tmp;
> +
> +       mutex_lock(&sbs_hook_mutex);
> +       INIT_LIST_HEAD(&battery->list);
> +       list_add(&battery->list, &sbs_hook_battery_list);
> +       /*
> +        * Since we added a new battery to the list, we need to
> +        * iterate over the hooks and call add_battery for each
> +        * hook that was registered. This usually happens
> +        * when a battery gets hotplugged or initialized
> +        * during the battery module initialization.
> +        */
> +       list_for_each_entry_safe(hook_node, tmp, &sbs_hook_list, list) {
> +               if (hook_node->add_battery(battery->bat, hook_node)) {
> +                       /*
> +                        * The notification of the hook has failed, to
> +                        * prevent further errors we will unload the hook.
> +                        */
> +                       pr_err("error in hook, unloading: %s",
> +                              hook_node->name);
> +                       sbs_battery_hook_unregister_unlocked(hook_node);
> +               }
> +       }
> +       mutex_unlock(&sbs_hook_mutex);
> +}
> +
> +static void sbs_battery_hook_remove_battery(struct acpi_battery *battery)
> +{
> +       struct acpi_battery_hook *hook;
> +
> +       mutex_lock(&sbs_hook_mutex);
> +       /*
> +        * Before removing the hook, we need to remove all
> +        * custom attributes from the battery.
> +        */
> +       list_for_each_entry(hook, &sbs_hook_list, list) {
> +               hook->remove_battery(battery->bat, hook);
> +       }
> +       /* Then, just remove the battery from the list */
> +       list_del(&battery->list);
> +       mutex_unlock(&sbs_hook_mutex);
> +}
> +
>  /* Smart Battery */
>  static int acpi_battery_add(struct acpi_sbs *sbs, int id)
>  {
> @@ -555,6 +696,8 @@ static int acpi_battery_add(struct acpi_sbs *sbs, int id)
>                 goto end;
>         }
>
> +       sbs_battery_hook_add_battery(battery);
> +
>        end:
>         pr_info("%s [%s]: Battery Slot [%s] (battery %s)\n",
>                ACPI_SBS_DEVICE_NAME, acpi_device_bid(sbs->device),
> @@ -566,8 +709,10 @@ static void acpi_battery_remove(struct acpi_sbs *sbs, int id)
>  {
>         struct acpi_battery *battery = &sbs->battery[id];
>
> -       if (battery->bat)
> +       if (battery->bat) {
> +               sbs_battery_hook_remove_battery(battery);
>                 power_supply_unregister(battery->bat);
> +       }
>  }
>
>  static int acpi_charger_add(struct acpi_sbs *sbs)
> diff --git a/include/acpi/battery.h b/include/acpi/battery.h
> index c93f16dfb94..ddbfa4ed135 100644
> --- a/include/acpi/battery.h
> +++ b/include/acpi/battery.h
> @@ -18,8 +18,24 @@ struct acpi_battery_hook {
>         struct list_head list;
>  };
>
> +/*
> + * battery_hook_register() and friends only see batteries registered by
> + * drivers/acpi/battery.c, the ACPI Control Method Battery driver (ACPI HID
> + * "PNP0C0A"). Batteries registered by drivers/acpi/sbs.c, the ACPI Smart
> + * Battery System driver (ACPI HID "ACPI0002", common on hardware with
> + * SMBus/SBS fuel-gauge chips such as many Intel MacBooks), are invisible
> + * to them; use the sbs_battery_hook_* equivalents below for those. A
> + * caller wanting to support both kinds of hardware needs two separate
> + * struct acpi_battery_hook instances, one per registration call, since a
> + * given instance's embedded list node can only belong to one list at a
> + * time.
> + */
>  void battery_hook_register(struct acpi_battery_hook *hook);
>  void battery_hook_unregister(struct acpi_battery_hook *hook);
>  int devm_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
>
> +void sbs_battery_hook_register(struct acpi_battery_hook *hook);
> +void sbs_battery_hook_unregister(struct acpi_battery_hook *hook);
> +int devm_sbs_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
> +
>  #endif
> --
> 2.55.0
>

Re: [PATCH 1/2] ACPI: sbs: add battery hook mechanism for SBS-registered batteries

[Thomas Weißschuh]

On 2026-09-13 17:14:09-0600, Jordan Brough wrote:
(...)

> Add a parallel sbs_battery_hook_register()/unregister() mechanism to
> drivers/acpi/sbs.c, mirroring the existing CmBatt hook API in name and
> behavior so that callers can support both battery registration paths
> symmetrically. This is purely additive: no existing hook consumer or
> code path in drivers/acpi/battery.c is touched, and sbs.c's own
> behavior is unchanged for any driver that does not call the new API.

(...)

> ---
>  drivers/acpi/sbs.c     | 147 ++++++++++++++++++++++++++++++++++++++++-
>  include/acpi/battery.h |  16 +++++
>  2 files changed, 162 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
> index 86b7c797585..305bf62eae3 100644
> --- a/drivers/acpi/sbs.c
> +++ b/drivers/acpi/sbs.c

(...)

> +/*
> + * Battery hook support, mirroring the mechanism in drivers/acpi/battery.c
> + * for batteries registered there. That mechanism only sees batteries
> + * added via drivers/acpi/battery.c, not the ones this driver registers,
> + * so drivers wanting to attach optional functionality (e.g. an extra
> + * power_supply_ext) to an SBS battery need this separate copy; see the
> + * comment above the declarations in include/acpi/battery.h.
> + */

(...)

This completely duplicates the hooking logic in battery.c.
Also it forces drivers to register both kinds of hooks explicitly.

Couldn't we have a single shared hooking infrastructure and registry
which avoid both issues?

(...)


Thomas