Skip to contentSkip to conversation

power: supply: add charge_behaviour property (force-discharge, inhibit-charge)

PATCH In review Thomas Weißschuh 5 participants 11 messages Nov 23, 2021 – Dec 21, 2021

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover power: supply: add charge_behaviour property (force-discharge, inhibit-charge)

Hi,

this series adds support for the charge_behaviour property to the power subsystem and thinkpad_acpi driver.

As thinkpad_acpi has to use the 'struct power_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of proper power_supply properties to implement this property.

Patch 1: Adds the power_supply documentation and basic public API Patch 2: Adds helpers to power_supply core to help drivers implement the
charge_behaviour attribute
Patch 3: Adds support for force-discharge to thinkpad_acpi. Patch 4: Adds support for inhibit-discharge to thinkpad_acpi.

Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)

Ognjen Galic:

Your S-o-b is on the original inhibit_charge and force_discharge patches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?

Sebastian Reichel:

Currently the series does not actually support the property as a proper powersupply property handled fully by power_supply_sysfs.c because there would be no user for this property.

Previous discussions about the API:

https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/

v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:

* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers in charge_behaviour_store()

Open points:

Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.

Thomas Weißschuh (4):
power: supply: add charge_behaviour attributes
power: supply: add helpers for charge_behaviour sysfs platform/x86: thinkpad_acpi: support force-discharge platform/x86: thinkpad_acpi: support inhibit-charge

 Documentation/ABI/testing/sysfs-class-power |  14 ++
 drivers/platform/x86/thinkpad_acpi.c        | 191 +++++++++++++++++++-
 drivers/power/supply/power_supply_sysfs.c   |  51 ++++++
 include/linux/power_supply.h                |  16 ++
 4 files changed, 268 insertions(+), 4 deletions(-)

base-commit: 66f4beaa6c1d28161f534471484b2daa2de1dce0

Signature · 1 line
2.34.0
#1 lore raw

PATCH v2 1/4 power: supply: add charge_behaviour attributes

This a revised version of
"[RFC] add standardized attributes for force_discharge and inhibit_charge" [0], incorporating discussion results.

The biggest change is the switch from two boolean attributes to a single enum attribute.

[0] https://lore.kernel.org/platform-driver-x86/[email protected]/

---
 Documentation/ABI/testing/sysfs-class-power | 14 ++++++++++++++
 include/linux/power_supply.h                |  7 +++++++
 2 files changed, 21 insertions(+)
Documentation/ABI/testing/sysfs-class-power +14 -0
index f7904efc4cfa..cece094764f8 100644
@@ -455,6 +455,20 @@ Description:
 			      "Unknown", "Charging", "Discharging",
 			      "Not charging", "Full"
 
+What:		/sys/class/power_supply/<supply_name>/charge_behaviour
+Date:		November 2021
+Contact:	[email protected]
+Description:
+		Represents the charging behaviour.
+
+		Access: Read, Write
+
+		Valid values:
+			================ ====================================
+			auto:            Charge normally, respect thresholds
+			inhibit-charge:  Do not charge while AC is attached
+			force-discharge: Force discharge while AC is attached
+
 What:		/sys/class/power_supply/<supply_name>/technology
 Date:		May 2007
 Contact:	[email protected]
include/linux/power_supply.h +7 -0
index 9ca1f120a211..70c333e86293 100644
@@ -132,6 +132,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
 	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
 	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
 	POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
@@ -202,6 +203,12 @@ enum power_supply_usb_type {
 	POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID,	/* Apple Charging Method */
 };
 
+enum power_supply_charge_behaviour {
+	POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0,
+	POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE,
+	POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE,
+};
+
 enum power_supply_notifier_events {
 	PSY_EVENT_PROP_CHANGED,
 };
Signature · 1 line
2.34.0
#2 lore raw

PATCH v2 2/4 power: supply: add helpers for charge_behaviour sysfs

In reply to Thomas Weißschuh

These helper functions can be used by drivers to implement their own sysfs-attributes.
This is useful for ACPI-drivers extending the default ACPI-battery with their own charge_behaviour attributes.

---
 drivers/power/supply/power_supply_sysfs.c | 55 +++++++++++++++++++++++
 include/linux/power_supply.h              |  9 ++++
 2 files changed, 64 insertions(+)
drivers/power/supply/power_supply_sysfs.c +55 -0
index c3d7cbcd4fad..5e3b8c15ddbe 100644
@@ -133,6 +133,12 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
 	[POWER_SUPPLY_SCOPE_DEVICE]	= "Device",
 };
 
+static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = {
+	[POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO]		= "auto",
+	[POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE]	= "inhibit-charge",
+	[POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE]	= "force-discharge",
+};
+
 static struct power_supply_attr power_supply_attrs[] = {
 	/* Properties of type `int' */
 	POWER_SUPPLY_ENUM_ATTR(STATUS),
@@ -484,3 +490,52 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 	return ret;
 }
+
+ssize_t power_supply_charge_behaviour_show(struct device *dev,
+					   unsigned int available_behaviours,
+					   enum power_supply_charge_behaviour current_behaviour,
+					   char *buf)
+{
+	bool match = false, available, active;
+	ssize_t count = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT); i++) {
+		available = available_behaviours & BIT(i);
+		active = i == current_behaviour;
+
+		if (available && active) {
+			count += sysfs_emit_at(buf, count, "[%s] ",
+					       POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[i]);
+			match = true;
+		} else if (available) {
+			count += sysfs_emit_at(buf, count, "%s ",
+					       POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[i]);
+		}
+	}
+
+	if (!match) {
+		dev_warn(dev, "driver reporting unsupported charge behaviour\n");
+		return -EINVAL;
+	}
+
+	if (count)
+		buf[count - 1] = '\n';
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(power_supply_charge_behaviour_show);
+
+int power_supply_charge_behaviour_parse(unsigned int available_behaviours, const char *buf)
+{
+	int i = sysfs_match_string(POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT, buf);
+
+	if (i < 0)
+		return i;
+
+	if (available_behaviours & BIT(i))
+		return i;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(power_supply_charge_behaviour_parse);
include/linux/power_supply.h +9 -0
index 70c333e86293..71f0379c2af8 100644
@@ -546,4 +546,13 @@ static inline
 void power_supply_remove_hwmon_sysfs(struct power_supply *psy) {}
 #endif
 
+#ifdef CONFIG_SYSFS
+ssize_t power_supply_charge_behaviour_show(struct device *dev,
+					   unsigned int available_behaviours,
+					   enum power_supply_charge_behaviour behaviour,
+					   char *buf);
+
+int power_supply_charge_behaviour_parse(unsigned int available_behaviours, const char *buf);
+#endif
+
 #endif /* __LINUX_POWER_SUPPLY_H__ */
Signature · 1 line
2.34.0
#3 lore raw

PATCH v2 3/4 platform/x86: thinkpad_acpi: support force-discharge

In reply to Thomas Weißschuh

This adds support for the force-discharge charge_behaviour through the embedded controller of ThinkPads.

---

This patch is based on https://lore.kernel.org/platform-driver-x86/[email protected]/

---
 drivers/platform/x86/thinkpad_acpi.c | 131 ++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 4 deletions(-)
drivers/platform/x86/thinkpad_acpi.c +127 -4
index 9c632df734bb..e3567cc686fa 100644
@@ -9319,6 +9319,8 @@ static struct ibm_struct mute_led_driver_data = {
 #define SET_START	"BCCS"
 #define GET_STOP	"BCSG"
 #define SET_STOP	"BCSS"
+#define GET_DISCHARGE	"BDSG"
+#define SET_DISCHARGE	"BDSS"
 
 enum {
 	BAT_ANY = 0,
@@ -9335,6 +9337,7 @@ enum {
 	/* This is used in the get/set helpers */
 	THRESHOLD_START,
 	THRESHOLD_STOP,
+	FORCE_DISCHARGE,
 };
 
 struct tpacpi_battery_data {
@@ -9342,6 +9345,7 @@ struct tpacpi_battery_data {
 	int start_support;
 	int charge_stop;
 	int stop_support;
+	unsigned int charge_behaviours;
 };
 
 struct tpacpi_battery_driver_data {
@@ -9399,6 +9403,12 @@ static int tpacpi_battery_get(int what, int battery, int *ret)
 		if (*ret == 0)
 			*ret = 100;
 		return 0;
+	case FORCE_DISCHARGE:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_DISCHARGE, ret, battery))
+			return -ENODEV;
+		/* The force discharge status is in bit 0 */
+		*ret = *ret & 0x01;
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;
@@ -9427,12 +9437,49 @@ static int tpacpi_battery_set(int what, int battery, int value)
 			return -ENODEV;
 		}
 		return 0;
+	case FORCE_DISCHARGE:
+		/* Force discharge is in bit 0,
+		 * break on AC attach is in bit 1 (won't work on some ThinkPads),
+		 * battery ID is in bits 8-9, 2 bits.
+		 */
+		if (ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_DISCHARGE, &ret, param))) {
+			pr_err("failed to set force discharge on %d", battery);
+			return -ENODEV;
+		}
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;
 	}
 }
 
+static int tpacpi_battery_set_validate(int what, int battery, int value)
+{
+	int ret, v;
+
+	ret = tpacpi_battery_set(what, battery, value);
+	if (ret < 0)
+		return ret;
+
+	ret = tpacpi_battery_get(what, battery, &v);
+	if (ret < 0)
+		return ret;
+
+	if (v == value)
+		return 0;
+
+	msleep(500);
+
+	ret = tpacpi_battery_get(what, battery, &v);
+	if (ret < 0)
+		return ret;
+
+	if (v == value)
+		return 0;
+
+	return -EIO;
+}
+
 static int tpacpi_battery_probe(int battery)
 {
 	int ret = 0;
@@ -9445,6 +9492,8 @@ static int tpacpi_battery_probe(int battery)
 	 * 2) Check for support
 	 * 3) Get the current stop threshold
 	 * 4) Check for support
+	 * 5) Get the current force discharge status
+	 * 6) Check for support
 	 */
 	if (acpi_has_method(hkey_handle, GET_START)) {
 		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {
@@ -9481,10 +9530,25 @@ static int tpacpi_battery_probe(int 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);
+	if (acpi_has_method(hkey_handle, GET_DISCHARGE)) {
+		if (ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_DISCHARGE, &ret, battery))) {
+			pr_err("Error probing battery discharge; %d\n", battery);
+			return -ENODEV;
+		}
+		/* Support is marked in bit 8 */
+		if (ret & BIT(8))
+			battery_info.batteries[battery].charge_behaviours |=
+				BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE);
+	}
+
+	battery_info.batteries[battery].charge_behaviours |=
+		BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO);
+
+	pr_info("battery %d registered (start %d, stop %d, behaviours: 0x%x)\n",
+		battery,
+		battery_info.batteries[battery].charge_start,
+		battery_info.batteries[battery].charge_stop,
+		battery_info.batteries[battery].charge_behaviours);
 
 	return 0;
 }
@@ -9619,6 +9683,28 @@ static ssize_t charge_control_end_threshold_show(struct device *device,
 	return tpacpi_battery_show(THRESHOLD_STOP, device, buf);
 }
 
+static ssize_t charge_behaviour_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	enum power_supply_charge_behaviour active = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+	struct power_supply *supply = to_power_supply(dev);
+	unsigned int available;
+	int ret, battery;
+
+	battery = tpacpi_battery_get_id(supply->desc->name);
+	available = battery_info.batteries[battery].charge_behaviours;
+
+	if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE)) {
+		if (tpacpi_battery_get(FORCE_DISCHARGE, battery, &ret))
+			return -ENODEV;
+		if (ret)
+			active = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
+	}
+
+	return power_supply_charge_behaviour_show(dev, available, active, buf);
+}
+
 static ssize_t charge_control_start_threshold_store(struct device *dev,
 				struct device_attribute *attr,
 				const char *buf, size_t count)
@@ -9633,8 +9719,44 @@ static ssize_t charge_control_end_threshold_store(struct device *dev,
 	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
 }
 
+static ssize_t charge_behaviour_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct power_supply *supply = to_power_supply(dev);
+	int selected, battery, ret = 0;
+	unsigned int available;
+
+	battery = tpacpi_battery_get_id(supply->desc->name);
+	available = battery_info.batteries[battery].charge_behaviours;
+	selected = power_supply_charge_behaviour_parse(available, buf);
+
+	if (selected < 0)
+		return selected;
+
+	switch (selected) {
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
+		if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE))
+			ret = tpacpi_battery_set_validate(FORCE_DISCHARGE, battery, 0);
+		if (ret < 0)
+			return ret;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
+		ret = tpacpi_battery_set_validate(FORCE_DISCHARGE, battery, 1);
+		if (ret < 0)
+			return ret;
+		break;
+	default:
+		dev_err(dev, "Unexpected charge behaviour: %d\n", selected);
+		return -EINVAL;
+	}
+
+	return count;
+}
+
 static DEVICE_ATTR_RW(charge_control_start_threshold);
 static DEVICE_ATTR_RW(charge_control_end_threshold);
+static DEVICE_ATTR_RW(charge_behaviour);
 static struct device_attribute dev_attr_charge_start_threshold = __ATTR(
 	charge_start_threshold,
 	0644,
@@ -9653,6 +9775,7 @@ static struct attribute *tpacpi_battery_attrs[] = {
 	&dev_attr_charge_control_end_threshold.attr,
 	&dev_attr_charge_start_threshold.attr,
 	&dev_attr_charge_stop_threshold.attr,
+	&dev_attr_charge_behaviour.attr,
 	NULL,
 };
 
Signature · 1 line
2.34.0
#4 lore raw

PATCH v2 4/4 platform/x86: thinkpad_acpi: support inhibit-charge

In reply to Thomas Weißschuh

This adds support for the inhibit-charge charge_behaviour through the embedded controller of ThinkPads.

---

This patch is based on https://lore.kernel.org/platform-driver-x86/[email protected]/

---
 drivers/platform/x86/thinkpad_acpi.c | 64 +++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 2 deletions(-)
drivers/platform/x86/thinkpad_acpi.c +62 -2
index e3567cc686fa..7f2f46c71482 100644
@@ -9321,6 +9321,8 @@ static struct ibm_struct mute_led_driver_data = {
 #define SET_STOP	"BCSS"
 #define GET_DISCHARGE	"BDSG"
 #define SET_DISCHARGE	"BDSS"
+#define GET_INHIBIT	"BICG"
+#define SET_INHIBIT	"BICS"
 
 enum {
 	BAT_ANY = 0,
@@ -9338,6 +9340,7 @@ enum {
 	THRESHOLD_START,
 	THRESHOLD_STOP,
 	FORCE_DISCHARGE,
+	INHIBIT_CHARGE,
 };
 
 struct tpacpi_battery_data {
@@ -9409,6 +9412,12 @@ static int tpacpi_battery_get(int what, int battery, int *ret)
 		/* The force discharge status is in bit 0 */
 		*ret = *ret & 0x01;
 		return 0;
+	case INHIBIT_CHARGE:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery))
+			return -ENODEV;
+		/* The inhibit charge status is in bit 0 */
+		*ret = *ret & 0x01;
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;
@@ -9447,6 +9456,22 @@ static int tpacpi_battery_set(int what, int battery, int value)
 			return -ENODEV;
 		}
 		return 0;
+	case INHIBIT_CHARGE:
+		/* When setting inhibit charge, we set a default value of
+		 * always breaking on AC detach and the effective time is set to
+		 * be permanent.
+		 * The battery ID is in bits 4-5, 2 bits,
+		 * the effective time is in bits 8-23, 2 bytes.
+		 * A time of FFFF indicates forever.
+		 */
+		param = value;
+		param |= battery << 4;
+		param |= 0xFFFF << 8;
+		if (ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_INHIBIT, &ret, param))) {
+			pr_err("failed to set inhibit charge on %d", battery);
+			return -ENODEV;
+		}
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;
@@ -9494,6 +9519,8 @@ static int tpacpi_battery_probe(int battery)
 	 * 4) Check for support
 	 * 5) Get the current force discharge status
 	 * 6) Check for support
+	 * 7) Get the current inhibit charge status
+	 * 8) Check for support
 	 */
 	if (acpi_has_method(hkey_handle, GET_START)) {
 		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {
@@ -9540,6 +9567,16 @@ static int tpacpi_battery_probe(int battery)
 			battery_info.batteries[battery].charge_behaviours |=
 				BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE);
 	}
+	if (acpi_has_method(hkey_handle, GET_INHIBIT)) {
+		if (ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, &ret, battery))) {
+			pr_err("Error probing battery inhibit charge; %d\n", battery);
+			return -ENODEV;
+		}
+		/* Support is marked in bit 5 */
+		if (ret & BIT(5))
+			battery_info.batteries[battery].charge_behaviours |=
+				BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE);
+	}
 
 	battery_info.batteries[battery].charge_behaviours |=
 		BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO);
@@ -9698,10 +9735,22 @@ static ssize_t charge_behaviour_show(struct device *dev,
 	if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE)) {
 		if (tpacpi_battery_get(FORCE_DISCHARGE, battery, &ret))
 			return -ENODEV;
-		if (ret)
+		if (ret) {
 			active = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
+			goto out;
+		}
+	}
+
+	if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)) {
+		if (tpacpi_battery_get(INHIBIT_CHARGE, battery, &ret))
+			return -ENODEV;
+		if (ret) {
+			active = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
+			goto out;
+		}
 	}
 
+out:
 	return power_supply_charge_behaviour_show(dev, available, active, buf);
 }
 
@@ -9738,11 +9787,22 @@ static ssize_t charge_behaviour_store(struct device *dev,
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
 		if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE))
 			ret = tpacpi_battery_set_validate(FORCE_DISCHARGE, battery, 0);
+		if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE))
+			ret = min(ret, tpacpi_battery_set_validate(INHIBIT_CHARGE, battery, 0));
 		if (ret < 0)
 			return ret;
 		break;
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
-		ret = tpacpi_battery_set_validate(FORCE_DISCHARGE, battery, 1);
+		if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE))
+			ret = tpacpi_battery_set_validate(INHIBIT_CHARGE, battery, 0);
+		ret = min(ret, tpacpi_battery_set_validate(FORCE_DISCHARGE, battery, 1));
+		if (ret < 0)
+			return ret;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
+		if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE))
+			ret = tpacpi_battery_set_validate(FORCE_DISCHARGE, battery, 0);
+		ret = min(ret, tpacpi_battery_set_validate(INHIBIT_CHARGE, battery, 1));
 		if (ret < 0)
 			return ret;
 		break;
Signature · 1 line
2.34.0
Tested-by on the v1 series
#5 lore raw
In reply to Thomas Weißschuh
Quoted 2 lines · Thomas Weißschuh

this series adds support for the charge_behaviour property to the power subsystem and thinkpad_acpi driver.

Wonderful! Thanks for working on this.

I can confirm inhibit-charge and force-discharge states work with patch v2 on v5.16-rc2 on a T430 (2342-CTO) with BIOS G1ETC2WW (2.82 ), EC G1HT36WW and a single battery. Most behavior is as expected:

- With force-discharge, status becomes "Discharging" and energy_now drops over time while AC remains connected.
- With inhibit-charge, status becomes "Unknown" and energy_now is stable over time, even when not fully charged.
- With auto, status becomes "Charging" and energy_now rises over time.
- charge_behaviour takes precedence over charge_control_{start,end}_threshold: status remains Discharging/Unknown when below the start threshold, either due to discharge or threshold change.
- charge_behaviour is preserved over soft reboot.
- inhibit-charge/auto are preserved across battery removal and reinsertion.
- inhibit-charge/auto are preserved across s2ram (S3).
- With force-discharge, if the battery is removed, the machine immediately powers off.

Some behavior is a little surprising:

- charge_behaviour can not be set to force-discharge if AC is disconnected (EIO). If charge_behaviour is force-discharge when AC is disconnected, it changes to auto, unlike inhibit-charge.
- charge_behavior force-discharge is not preserved across s2ram (S3), unlike inhibit-charge.
- charge_behaviour is not preserved across hard reset (unlike charge thresholds). Interestingly, it appears that inhibit-charge is preserved across power-off (no charging is observed while powered off) but not power-on, even though it is preserved across soft reboot, as noted above.

I assume the behavior is under the control of the EC, so these aren't criticisms of the patch. Just some observations.

Thanks again,
Kevin

Acked-by on the v1 series
#6 lore raw
In reply to Thomas Weißschuh

Hi,

Quoted 23 lines · Thomas Weißschuh

This series adds support for the charge_behaviour property to the power subsystem and thinkpad_acpi driver.

As thinkpad_acpi has to use the 'struct power_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of proper power_supply properties to implement this property.

Patch 1: Adds the power_supply documentation and basic public API Patch 2: Adds helpers to power_supply core to help drivers implement the
charge_behaviour attribute
Patch 3: Adds support for force-discharge to thinkpad_acpi. Patch 4: Adds support for inhibit-discharge to thinkpad_acpi.

Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)

Ognjen Galic:

Your S-o-b is on the original inhibit_charge and force_discharge patches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?

Quoted 5 lines

Sebastian Reichel:

Currently the series does not actually support the property as a proper powersupply property handled fully by power_supply_sysfs.c because there would be no user for this property.

I'm not too happy how the acpi-battery hooks work, but that's not your fault and this patchset does not really make the situation worse. So:

-- Sebastian

Quoted 38 lines

Previous discussions about the API:

https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/

v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:

* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers in charge_behaviour_store()

Open points:

Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.

Thomas Weißschuh (4):
power: supply: add charge_behaviour attributes
power: supply: add helpers for charge_behaviour sysfs platform/x86: thinkpad_acpi: support force-discharge platform/x86: thinkpad_acpi: support inhibit-charge

 Documentation/ABI/testing/sysfs-class-power |  14 ++
 drivers/platform/x86/thinkpad_acpi.c        | 191 +++++++++++++++++++-
 drivers/power/supply/power_supply_sysfs.c   |  51 ++++++
 include/linux/power_supply.h                |  16 ++
 4 files changed, 268 insertions(+), 4 deletions(-)

base-commit: 66f4beaa6c1d28161f534471484b2daa2de1dce0

Signature · 1 line
2.34.0
#7 lore raw

Hi,

Quoted 32 lines · Sebastian Reichel

Hi,

Quoted 23 lines · Thomas Weißschuh

This series adds support for the charge_behaviour property to the power subsystem and thinkpad_acpi driver.

As thinkpad_acpi has to use the 'struct power_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of proper power_supply properties to implement this property.

Patch 1: Adds the power_supply documentation and basic public API Patch 2: Adds helpers to power_supply core to help drivers implement the
charge_behaviour attribute
Patch 3: Adds support for force-discharge to thinkpad_acpi. Patch 4: Adds support for inhibit-discharge to thinkpad_acpi.

Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)

Ognjen Galic:

Your S-o-b is on the original inhibit_charge and force_discharge patches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?

Quoted 5 lines

Sebastian Reichel:

Currently the series does not actually support the property as a proper powersupply property handled fully by power_supply_sysfs.c because there would be no user for this property.

Quoted 5 lines

I'm not too happy how the acpi-battery hooks work, but that's not your fault and this patchset does not really make the situation worse. So:

I haven't looked at the thinkpad_apci.c bits closely yet (for this new version), but assuming those are ready for merging too, we need to discuss about how to merge this.

The thinkpad_acpi code has already seen quite a lot of changes in -next, so I would like the thinkpad_acpi changes to go upstream through the platform-drivers-x86.git tree to avoid conflicts.

As such I think it is best if you (Sebastian) can prepare an immutable branch with patch 1 + 2 for me to merge. Then even if patch 3 + 4 need more work, Thomas can just respin those on top of the immutable branch.

Alternatively I can take the entire series upstream through the platform-drivers-x86.git tree if that is ok with you (Sebastian).

Either way please let me know how you want to proceed with this.

Regards,

Hans

Quoted 38 lines
Quoted 38 lines

Previous discussions about the API:

https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/

v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:

* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers in charge_behaviour_store()

Open points:

Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.

Thomas Weißschuh (4):
power: supply: add charge_behaviour attributes
power: supply: add helpers for charge_behaviour sysfs platform/x86: thinkpad_acpi: support force-discharge platform/x86: thinkpad_acpi: support inhibit-charge

 Documentation/ABI/testing/sysfs-class-power |  14 ++
 drivers/platform/x86/thinkpad_acpi.c        | 191 +++++++++++++++++++-
 drivers/power/supply/power_supply_sysfs.c   |  51 ++++++
 include/linux/power_supply.h                |  16 ++
 4 files changed, 268 insertions(+), 4 deletions(-)

base-commit: 66f4beaa6c1d28161f534471484b2daa2de1dce0

Signature · 1 line
2.34.0
#8 lore raw
In reply to Sebastian Reichel

Hi,

Quoted 32 lines · Sebastian Reichel

Hi,

Quoted 23 lines · Thomas Weißschuh

This series adds support for the charge_behaviour property to the power subsystem and thinkpad_acpi driver.

As thinkpad_acpi has to use the 'struct power_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of proper power_supply properties to implement this property.

Patch 1: Adds the power_supply documentation and basic public API Patch 2: Adds helpers to power_supply core to help drivers implement the
charge_behaviour attribute
Patch 3: Adds support for force-discharge to thinkpad_acpi. Patch 4: Adds support for inhibit-discharge to thinkpad_acpi.

Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)

Ognjen Galic:

Your S-o-b is on the original inhibit_charge and force_discharge patches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?

Quoted 5 lines

Sebastian Reichel:

Currently the series does not actually support the property as a proper powersupply property handled fully by power_supply_sysfs.c because there would be no user for this property.

Quoted 5 lines

I'm not too happy how the acpi-battery hooks work, but that's not your fault and this patchset does not really make the situation worse. So:

Sebastian, what is the plan for taking this upstream ? Does your ack mean that you are ok with me taking the entire series upstream through the pdx86 tree?

Or do you plan to apply patches 1-2 through linux-power-supply.git; and in that case can you provide an inmmutable branch with those patches for me to merge into pdx86/for-next so that I can then apply patches 3 + 4 there ?

Note merging everything through the linux-power-supply.git tree is non ideal in this case because the thinkpad_acpi.c code already has a lot of changes in pdx86/for-next.

Regards,

Hans

Quoted 38 lines
Quoted 38 lines

Previous discussions about the API:

https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/

v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:

* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers in charge_behaviour_store()

Open points:

Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.

Thomas Weißschuh (4):
power: supply: add charge_behaviour attributes
power: supply: add helpers for charge_behaviour sysfs platform/x86: thinkpad_acpi: support force-discharge platform/x86: thinkpad_acpi: support inhibit-charge

 Documentation/ABI/testing/sysfs-class-power |  14 ++
 drivers/platform/x86/thinkpad_acpi.c        | 191 +++++++++++++++++++-
 drivers/power/supply/power_supply_sysfs.c   |  51 ++++++
 include/linux/power_supply.h                |  16 ++
 4 files changed, 268 insertions(+), 4 deletions(-)

base-commit: 66f4beaa6c1d28161f534471484b2daa2de1dce0

Signature · 1 line
2.34.0
#9 lore raw
In reply to Sebastian Reichel

Hi,

Quoted 32 lines · Sebastian Reichel

Hi,

Quoted 23 lines · Thomas Weißschuh

This series adds support for the charge_behaviour property to the power subsystem and thinkpad_acpi driver.

As thinkpad_acpi has to use the 'struct power_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of proper power_supply properties to implement this property.

Patch 1: Adds the power_supply documentation and basic public API Patch 2: Adds helpers to power_supply core to help drivers implement the
charge_behaviour attribute
Patch 3: Adds support for force-discharge to thinkpad_acpi. Patch 4: Adds support for inhibit-discharge to thinkpad_acpi.

Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)

Ognjen Galic:

Your S-o-b is on the original inhibit_charge and force_discharge patches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?

Quoted 5 lines

Sebastian Reichel:

Currently the series does not actually support the property as a proper powersupply property handled fully by power_supply_sysfs.c because there would be no user for this property.

Quoted 5 lines

I'm not too happy how the acpi-battery hooks work, but that's not your fault and this patchset does not really make the situation worse. So:

Sebastian, I have taken the liberty to assume that this means that you are ok with merging the entire series through the pdx86 tree (I've done a test-merge with linux-power-supply/for-next and there are no conflicts).

Thomas, Thank you for your patch-series, I've applied the series to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window.

Regards,

Hans

Quoted 41 lines

-- Sebastian

Quoted 38 lines

Previous discussions about the API:

https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/

v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:

* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers in charge_behaviour_store()

Open points:

Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.

Thomas Weißschuh (4):
power: supply: add charge_behaviour attributes
power: supply: add helpers for charge_behaviour sysfs platform/x86: thinkpad_acpi: support force-discharge platform/x86: thinkpad_acpi: support inhibit-charge

 Documentation/ABI/testing/sysfs-class-power |  14 ++
 drivers/platform/x86/thinkpad_acpi.c        | 191 +++++++++++++++++++-
 drivers/power/supply/power_supply_sysfs.c   |  51 ++++++
 include/linux/power_supply.h                |  16 ++
 4 files changed, 268 insertions(+), 4 deletions(-)

base-commit: 66f4beaa6c1d28161f534471484b2daa2de1dce0

Signature · 1 line
2.34.0
Tested-by on the v1 series
#10 lore raw
In reply to Thomas Weißschuh

Hi Thomas,

Quoted 58 lines · Thomas Weißschuh

Hi,

this series adds support for the charge_behaviour property to the power subsystem and thinkpad_acpi driver.

As thinkpad_acpi has to use the 'struct power_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of proper power_supply properties to implement this property.

Patch 1: Adds the power_supply documentation and basic public API Patch 2: Adds helpers to power_supply core to help drivers implement the
charge_behaviour attribute
Patch 3: Adds support for force-discharge to thinkpad_acpi. Patch 4: Adds support for inhibit-discharge to thinkpad_acpi.

Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)

Ognjen Galic:

Your S-o-b is on the original inhibit_charge and force_discharge patches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?

Sebastian Reichel:

Currently the series does not actually support the property as a proper powersupply property handled fully by power_supply_sysfs.c because there would be no user for this property.

Previous discussions about the API:

https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/

v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:

* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers in charge_behaviour_store()

Open points:

Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.

Thomas Weißschuh (4):
power: supply: add charge_behaviour attributes
power: supply: add helpers for charge_behaviour sysfs platform/x86: thinkpad_acpi: support force-discharge platform/x86: thinkpad_acpi: support inhibit-charge

Quoted 9 lines
  Documentation/ABI/testing/sysfs-class-power |  14 ++
  drivers/platform/x86/thinkpad_acpi.c        | 191 +++++++++++++++++++-
  drivers/power/supply/power_supply_sysfs.c   |  51 ++++++
  include/linux/power_supply.h                |  16 ++
  4 files changed, 268 insertions(+), 4 deletions(-)

base-commit: 66f4beaa6c1d28161f534471484b2daa2de1dce0

Reviewed-by
: Thomas Koch <[email protected]>

Works well on ThinkPad X220, T450s, X1C6 with the exception mentioned above.

The new API is included in TLP already [1].

[1]
https://github.com/linrunner/TLP/commit/f0bf18f847470ae495a68f9f0e30130b96348936

--
Freundliche Grüße / Kind regards,
Thomas Koch

Mail : [email protected]
Web : https://linrunner.de/tlp