[PATCH v3 0/3] hwmon: (applesmc) add charge_control_end_threshold support

[PATCH v3 0/3] hwmon: (applesmc) add charge_control_end_threshold support

[Jordan Brough]

This series adds charge_control_end_threshold to the applesmc driver for
Intel MacBooks, using the power_supply extension API.

Some of these machines have an SBS battery (drivers/acpi/sbs.c), which,
unlike battery.c, has no battery hook mechanism. Patch 2 moves the hook code
into a helper that both drivers use, and patch 3 adds the applesmc support.
Patch 1 is the acpi_ prefix rename of the existing hook functions that Rafael
asked for; it is separate because it touches 18 callers in other subsystems.

The series is based on linux-pm's bleeding-edge branch (4c18627a1dfb). Patch
3 depends on patch 2, so the series would need to go in through the ACPI
tree with an ack from the hwmon side, or I can resend patch 3 once patches 1
and 2 are in, whichever is easier.

Changes in v3 (thanks to Rafael for the review):
- Patch 1 is new: acpi_ prefix for the exported hook functions, with their
  callers updated.
- Patch 2:
  - Renamed battery_hook.c to battery_hooks.c, built only when ACPI_BATTERY
    or ACPI_SBS is, through a hidden ACPI_BATTERY_HOOKS symbol that both
    select.
  - Renamed the struct and the new functions as suggested, exported the new
    functions in the ACPI_BATTERY_HOOKS namespace, used mutex guards and
    updated the file header.
  - battery_hook_exit() is gone, so hooks now stay registered across a
    reload of battery.ko or sbs.ko.
- Patch 3:
  - BFCL is only written when the SMC has the key, and the BCLM write is read
    back, at the suggestion of Michal Szpakowski, whose MacBookPro13,1 has no
    BFCL.
  - No BFCL margin at a limit of 100.
  - The lower limit of 20 is conservative: 20 works and 10 is not accepted
    on the hardware I tried, but I did not find the exact floor. I am happy
    to change it.
  - Dropped the applesmc_hooked_battery tracking and the mutex in
    applesmc_bat_get_property().
  - The hook is only registered when CONFIG_ACPI_BATTERY_HOOKS is reachable.

Testing:
- MacBookAir6,2 (SBS battery), on an earlier revision: limits from 20 to 100
  match the SMC keys, charging stops at the limit, repeated module reloads
  caused no errors, and the threshold was unchanged after a suspend/resume.
- MacBookPro13,1 (SBS battery, no BFCL), by Michal Szpakowski, before the
  acpi_ rename: valid limits read back exactly, invalid ones are rejected,
  applesmc reload and an acpi-sbs unbind/rebind re-attach the attribute, and
  charging stops at the limit.
- Lenovo ideapad FLEX 4-1480 (Control Method battery, ideapad_laptop), on
  Fedora's 7.2.7 kernel with the series applied on top (a rebase of the
  patches, not this exact tree): the ideapad_laptop hook registers and its
  charge_types attribute appears on BAT1, charge_types can be read and
  written back unchanged, 25 battery unbind/bind cycles re-attach it each
  time, and unloading and reloading ideapad_laptop removes and re-adds it.
  There were no warnings in dmesg. That kernel did not have lockdep
  enabled.
- Built and linked with ACPI_BATTERY and ACPI_SBS as y/m/n and applesmc as
  y/m, and an x86 allmodconfig build of the touched files with W=1 shows no
  warnings; each patch builds on its own.
- Applies with git am on linux-pm bleeding-edge, and with git am --3way on
  hwmon-next.

Link: https://lore.kernel.org/r/20260918175052.85461-1-jordan@brough.org [v2]
Link: https://lore.kernel.org/r/20260913231410.416922-1-jordan@brough.org [v1]


Jordan Brough (3):
  ACPI: battery: add acpi_ prefix to the battery hook API
  ACPI: battery: add unified battery hook mechanism for ACPI and SBS
    batteries
  hwmon: (applesmc) add charge_control_end_threshold support

 drivers/acpi/Kconfig                         |   5 +
 drivers/acpi/Makefile                        |   1 +
 drivers/acpi/battery.c                       | 166 +------------------
 drivers/acpi/battery_hooks.c                 | 159 ++++++++++++++++++
 drivers/acpi/sbs.c                           |   8 +-
 drivers/hwmon/Kconfig                        |   1 +
 drivers/hwmon/applesmc.c                     | 146 ++++++++++++++++
 drivers/platform/x86/asus-wmi.c              |   4 +-
 drivers/platform/x86/ayaneo-ec.c             |   2 +-
 drivers/platform/x86/dell/dell-laptop.c      |   4 +-
 drivers/platform/x86/dell/dell-wmi-ddv.c     |   2 +-
 drivers/platform/x86/fujitsu-laptop.c        |   4 +-
 drivers/platform/x86/huawei-wmi.c            |   4 +-
 drivers/platform/x86/lenovo/ideapad-laptop.c |   4 +-
 drivers/platform/x86/lenovo/thinkpad_acpi.c  |   4 +-
 drivers/platform/x86/lenovo/wmi-other.c      |   4 +-
 drivers/platform/x86/lg-laptop.c             |   4 +-
 drivers/platform/x86/msi-ec.c                |   4 +-
 drivers/platform/x86/oxpec.c                 |   2 +-
 drivers/platform/x86/samsung-galaxybook.c    |   3 +-
 drivers/platform/x86/samsung-laptop.c        |   4 +-
 drivers/platform/x86/system76_acpi.c         |   4 +-
 drivers/platform/x86/toshiba_acpi.c          |   4 +-
 drivers/platform/x86/uniwill/uniwill-acpi.c  |   2 +-
 drivers/power/supply/cros_charge-control.c   |   2 +-
 include/acpi/battery.h                       |  16 +-
 26 files changed, 367 insertions(+), 196 deletions(-)
 create mode 100644 drivers/acpi/battery_hooks.c


base-commit: 4c18627a1dfbfe537580f831e60218644c94ce12
-- 
2.56.0

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

[Jordan Brough]

Intel MacBooks keep the battery charge limit in the SMC key BCLM, and
some also have BFCL, which sets the threshold for the MagSafe LED.

Expose BCLM as charge_control_end_threshold through the power_supply
extension API, attached to the battery with the ACPI battery hooks.

Below 100%, BFCL is set 3 points below BCLM so that the LED turns green
before charging stops. On a MacBookAir6,2 the LED turns green about 0.75
points above BFCL and charging stops about 0.25 points below BCLM, so
this leaves about 2 points; other models were not measured. BFCL is only
written if the SMC has the key, and a failure to write it is only logged.

The SMC ignores BCLM values it does not accept but still reports
success, so read the value back and return -EINVAL if it was not applied.

Limits below 20 are rejected. 20 is known to work and 10 is not accepted
by the SMC on the hardware tested; the exact floor in between was not
determined.

Serialize the writes with applesmc_bat_lock, and notify userspace of
changes with power_supply_changed().

Tested-by: Michal Szpakowski <michi.szpakowski@gmail.com>
Signed-off-by: Jordan Brough <jordan@brough.org>
---
 drivers/hwmon/Kconfig    |   1 +
 drivers/hwmon/applesmc.c | 146 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 147 insertions(+)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index fecff8610ea8..d627b4cf11e5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -380,6 +380,7 @@ config SENSORS_FAM15H_POWER
 config SENSORS_APPLESMC
 	tristate "Apple SMC (Motion sensor, light sensor, keyboard backlight)"
 	depends on INPUT && X86
+	depends on POWER_SUPPLY || POWER_SUPPLY=n
 	select NEW_LEDS
 	select LEDS_CLASS
 	help
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index d0baa10502f7..b6d9ca302730 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,20 @@
 
 #define TEMP_SENSOR_TYPE	"sp78"
 
+/*
+ * BCLM caps charging at a percentage. BFCL only sets when the charging LED
+ * switches from orange to green.
+ */
+#define BATTERY_CHARGE_LIMIT_KEY	"BCLM" /* r/w ui8 */
+#define BATTERY_CHARGE_LIMIT_LED_KEY	"BFCL" /* r/w ui8 */
+
+/*
+ * Points kept between BCLM and BFCL so the LED turns green before charging
+ * stops. Measured on a MacBookAir6,2, where a margin of 1 only just ties;
+ * 3 leaves headroom for other models.
+ */
+#define APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN 3
+
 /* List of keys used to read/write fan speeds */
 static const char *const fan_speed_fmt[] = {
 	"F%dAc",		/* actual speed */
@@ -131,6 +147,8 @@ 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 has_battery_charge_limit_led; /* has BFCL MagSafe LED charge limit */
 	bool init_complete;		/* true when fully initialized */
 	struct applesmc_entry *cache;	/* cached key entries */
 	const char **index;		/* temperature key index */
@@ -633,6 +651,12 @@ 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;
+	ret = applesmc_has_key(BATTERY_CHARGE_LIMIT_LED_KEY, &s->has_battery_charge_limit_led);
 	if (ret)
 		return ret;
 
@@ -724,6 +748,120 @@ static struct platform_driver applesmc_driver = {
 	},
 };
 
+static DEFINE_MUTEX(applesmc_bat_lock);
+
+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:
+		scoped_guard(mutex, &applesmc_bat_lock)
+			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, readback;
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		/* 20 is known to work and 10 is not accepted; the floor is unknown. */
+		if (val->intval < 20 || val->intval > 100)
+			return -EINVAL;
+
+		limit = val->intval;
+		/* At 100% charging is never cut short, so no margin is needed. */
+		led_limit = limit < 100 ?
+			limit - APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN : limit;
+
+		scoped_guard(mutex, &applesmc_bat_lock) {
+			ret = applesmc_write_key(BATTERY_CHARGE_LIMIT_KEY, &limit, 1);
+			if (ret)
+				return ret;
+
+			/*
+			 * The SMC silently ignores values it does not accept and
+			 * still reports success, so read the limit back.
+			 */
+			ret = applesmc_read_key(BATTERY_CHARGE_LIMIT_KEY, &readback, 1);
+			if (ret)
+				return ret;
+			if (readback != limit)
+				return -EINVAL;
+
+			if (smcreg.has_battery_charge_limit_led) {
+				ret = applesmc_write_key(BATTERY_CHARGE_LIMIT_LED_KEY,
+							 &led_limit, 1);
+				/* BCLM is already applied, so this is not an error. */
+				if (ret)
+					pr_warn("failed to update MagSafe LED threshold (BFCL): %d\n",
+						ret);
+			}
+		}
+
+		power_supply_changed(psy);
+		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 int applesmc_bat_add(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	return power_supply_register_extension(battery, &applesmc_bat_ext, &pdev->dev, NULL);
+}
+
+static int applesmc_bat_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	power_supply_unregister_extension(battery, &applesmc_bat_ext);
+	return 0;
+}
+
+static struct acpi_battery_hook applesmc_bat_hook = {
+	.name = "Apple SMC Battery Charge Control",
+	.add_battery = applesmc_bat_add,
+	.remove_battery = applesmc_bat_remove,
+};
+
 /*
  * applesmc_calibrate - Set our "resting" values.  Callers must
  * hold applesmc_lock.
@@ -1563,6 +1701,10 @@ static int __init applesmc_init(void)
 		goto out_light_ledclass;
 	}
 
+	if (smcreg.has_battery_charge_limit &&
+	    IS_REACHABLE(CONFIG_ACPI_BATTERY_HOOKS))
+		acpi_battery_hook_register(&applesmc_bat_hook);
+
 	return 0;
 
 out_light_ledclass:
@@ -1589,6 +1731,10 @@ static int __init applesmc_init(void)
 
 static void __exit applesmc_exit(void)
 {
+	if (smcreg.has_battery_charge_limit &&
+	    IS_REACHABLE(CONFIG_ACPI_BATTERY_HOOKS))
+		acpi_battery_hook_unregister(&applesmc_bat_hook);
+
 	hwmon_device_unregister(hwmon_dev);
 	applesmc_release_key_backlight();
 	applesmc_release_light_sensor();
-- 
2.56.0

Re: [PATCH v3 3/3] hwmon: (applesmc) add charge_control_end_threshold support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] hwmon: applesmc implements secondary power supply feature natively instead of using auxiliary device
- [Medium] hwmon: applesmc uses custom mutex instead of hwmon_lock() for external attributes
--

commit 7f74cc774d21a32964e467d3d8da2004ccb22a1e
Author: Jordan Brough <jordan@brough.org>

hwmon: (applesmc) add charge_control_end_threshold support

This commit exposes the MacBook battery charge limit (BCLM) as
charge_control_end_threshold through the power_supply extension API.
It attaches to the battery with ACPI battery hooks and serializes writes
with a newly introduced mutex.

> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index d0baa10502f7d..b6d9ca3027308 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
[ ... ]
> @@ -724,6 +748,120 @@ static struct platform_driver applesmc_driver = {
>  	},
>  };
>  
> +static DEFINE_MUTEX(applesmc_bat_lock);

[Severity: Medium]
Does this custom lock violate the hwmon subsystem locking guidelines? 

The guidelines mandate that drivers use hwmon_lock() and hwmon_unlock() for
serializing attributes registered by any means other than the info parameter 
of hwmon_device_register_with_info(). Since this lock is used to serialize 
power supply property access, it appears we should be using hwmon_lock() 
instead.

> +
> +static int applesmc_bat_get_property(struct power_supply *psy,
[ ... ]
> +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, readback;
> +	int ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
> +		/* 20 is known to work and 10 is not accepted; the floor is unknown. */
> +		if (val->intval < 20 || val->intval > 100)
> +			return -EINVAL;
> +
> +		limit = val->intval;
> +		/* At 100% charging is never cut short, so no margin is needed. */
> +		led_limit = limit < 100 ?
> +			limit - APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN : limit;
> +
> +		scoped_guard(mutex, &applesmc_bat_lock) {

[Severity: Medium]
As mentioned above, should this be using hwmon_lock() to serialize access
instead of the custom applesmc_bat_lock?

> +			ret = applesmc_write_key(BATTERY_CHARGE_LIMIT_KEY, &limit, 1);
[ ... ]
> +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 int applesmc_bat_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	return power_supply_register_extension(battery, &applesmc_bat_ext, &pdev->dev, NULL);
> +}

[Severity: Medium]
Is this direct registration of the power supply extension bypassing the hwmon
subsystem layering rules?

The hardware monitoring subsystem guidelines state that secondary
functionality (like power supply properties) should be implemented as an
auxiliary device driver, with the driver residing in the appropriate
subsystem directory, rather than being registered directly from the hwmon
driver.

> +
> +static int applesmc_bat_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	power_supply_unregister_extension(battery, &applesmc_bat_ext);
> +	return 0;
> +}
> +
> +static struct acpi_battery_hook applesmc_bat_hook = {
> +	.name = "Apple SMC Battery Charge Control",
> +	.add_battery = applesmc_bat_add,
> +	.remove_battery = applesmc_bat_remove,
> +};

[Severity: Medium]
Similar to the power supply extension above, should this ACPI battery hook
also be separated into an auxiliary device driver?

Registering it directly here in the hardware monitoring driver tightly couples
unrelated subsystems and bypasses proper review from the power supply
maintainers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930222650.1883805-1-jordan@brough.org?part=3