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

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

Signed-off-by Thomas Weißschuh <[email protected]>

---

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

 drivers/platform/x86/thinkpad_acpi.c | 103 +++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +98 -4

index 9c632df734bb..e8c98e9aff71 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,6 +9437,16 @@ 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 dischrage on %d", battery);
+			return -ENODEV;
+		}
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;

@@ -9445,6 +9465,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 +9503,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 +9656,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 +9692,43 @@ 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;
+	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:
+		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
+		if (ret < 0)
+			return ret;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
+		ret = tpacpi_battery_set(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,

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

Signed-off-by Thomas Weißschuh <[email protected]>

---

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

---
 drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +38 -0

index e8c98e9aff71..7cd6475240b2 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	"PSSG"
+#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,13 @@ 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:
+		/* This is actually reading peak shift state, like tpacpi-bat does */
+		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 +9457,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;

@@ -9467,6 +9493,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)) {

@@ -9513,6 +9541,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);

drivers/platform/x86/thinkpad_acpi.c +14 -2

+		if (tpacpi_battery_get(INHIBIT_CHARGE, battery, &ret))
+			return -ENODEV;
+		if (ret)
+			active = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
 	}
 
 	return power_supply_charge_behaviour_show(dev, available, active, buf);

@@ -9710,12 +9753,20 @@ static ssize_t charge_behaviour_store(struct device *dev,
 	switch (selected) {
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
 		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
-		if (ret < 0)
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;
+		if (ret)
 			return ret;
 		break;
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
 		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 1);
-		if (ret < 0)
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;
+		if (ret)
+			return ret;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
+		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 1) || ret;
+		if (ret)
 			return ret;
 		break;
 	default:

Thank you for working on this!

drivers/platform/x86/thinkpad_acpi.c static ssize_t charge_behaviour_show(struct device *dev, · line 9714

Quoted 5 lines

drivers/platform/x86/thinkpad_acpi.c +1 -0

@@ -9673,6 +9711,11 @@ static ssize_t charge_behaviour_show(struct device *dev,
 			return -ENODEV;
 		if (ret)
 			active = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
+	} else if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)) {

The use of else-if here seems wrong, this suggests that batterys can never support both force-discharge and inhibit-charge behavior, which they can, so this means that active can now never get set to BEHAVIOUR_INHIBIT_CHARGE on batteries which support both.

So AFAICT the else part of the else if should be dropped here, making this a new stand alone if block.

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

Signed-off-by Thomas Weißschuh <[email protected]>

---

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

 drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +46 -2

index e8c98e9aff71..7cd6475240b2 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	"PSSG"
+#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,13 @@ 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:
+		/* This is actually reading peak shift state, like tpacpi-bat does */
+		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 +9457,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;

@@ -9467,6 +9493,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)) {

@@ -9513,6 +9541,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);

@@ -9673,6 +9711,11 @@ static ssize_t charge_behaviour_show(struct device *dev,
 			return -ENODEV;
 		if (ret)
 			active = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
+	} else 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;
 	}
 
 	return power_supply_charge_behaviour_show(dev, available, active, buf);

@@ -9710,12 +9753,20 @@ static ssize_t charge_behaviour_store(struct device *dev,
 	switch (selected) {
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
 		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
-		if (ret < 0)
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;
+		if (ret)
 			return ret;
 		break;
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
 		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 1);
-		if (ret < 0)
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;

Quoted 126 lines · Thomas Weißschuh

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

Signed-off-by Thomas Weißschuh <[email protected]>

---

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

 drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +52 -2

index e8c98e9aff71..7cd6475240b2 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	"PSSG"
+#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,13 @@ 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:
+		/* This is actually reading peak shift state, like tpacpi-bat does */
+		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 +9457,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;

@@ -9467,6 +9493,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)) {

@@ -9513,6 +9541,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);

@@ -9673,6 +9711,11 @@ static ssize_t charge_behaviour_show(struct device *dev,
 			return -ENODEV;
 		if (ret)
 			active = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
+	} else 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;
 	}
 
 	return power_supply_charge_behaviour_show(dev, available, active, buf);

@@ -9710,12 +9753,20 @@ static ssize_t charge_behaviour_store(struct device *dev,
 	switch (selected) {
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
 		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
-		if (ret < 0)
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;
+		if (ret)
 			return ret;
 		break;
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
 		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 1);
-		if (ret < 0)
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;
+		if (ret)
+			return ret;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
+		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 1) || ret;

drivers/platform/x86/thinkpad_acpi.c static ssize_t charge_behaviour_store(struct device *dev, · line 9772

Quoted 5 lines

drivers/platform/x86/thinkpad_acpi.c +1 -0

+		if (ret)
 			return ret;
 		break;
 	default:

I can confirm the bit fields are correct for these calls (as for the previous patch)

Couple of things to note, based on feedback previously from the FW team that I found when digging thru my battery related emails.

"Lenovo doesn't officially support the use of these calls - they're intended for internal use" (at this point in time - there is some discussion about battery recalibration support but I don't have details I can share there yet).

The FW team also noted that:

"Actual battery charging/discharging behaviors are managed by ECFW. So the request of BDSS/BICS method may be rejected by ECFW for the current battery mode/status. You have to check if the request of the method is actually applied or not"

So for the calls above (and for the BDSS calls in the previous patch) it would be good to do a read back of the setting to ensure it has completed successfully.

Hope that helps

Hi Mark,

Quoted 127 lines · Mark Pearson

Quoted 126 lines · Thomas Weißschuh

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

Signed-off-by Thomas Weißschuh <[email protected]>

---

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

 drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +52 -2

index e8c98e9aff71..7cd6475240b2 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	"PSSG"
+#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,13 @@ 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:
+		/* This is actually reading peak shift state, like tpacpi-bat does */
+		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 +9457,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;

@@ -9467,6 +9493,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)) {

@@ -9513,6 +9541,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);

@@ -9673,6 +9711,11 @@ static ssize_t charge_behaviour_show(struct device *dev,
 			return -ENODEV;
 		if (ret)
 			active = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
+	} else 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;
 	}
 
 	return power_supply_charge_behaviour_show(dev, available, active, buf);

@@ -9710,12 +9753,20 @@ static ssize_t charge_behaviour_store(struct device *dev,
 	switch (selected) {
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
 		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
-		if (ret < 0)
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;
+		if (ret)
 			return ret;
 		break;
 	case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
 		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 1);
-		if (ret < 0)
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret;
+		if (ret)
+			return ret;
+		break;
+	case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
+		ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0);
+		ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 1) || ret;

Quoted 8 lines

drivers/platform/x86/thinkpad_acpi.c static ssize_t charge_behaviour_store(struct device *dev, · line 9772

Quoted 5 lines

drivers/platform/x86/thinkpad_acpi.c +1 -0

+		if (ret)
 			return ret;
 		break;
 	default:

I can confirm the bit fields are correct for these calls (as for the previous patch)

Thanks!

Could you doublecheck the behavior for multiple batteries to maybe find a reason why BAT1 is not inhibited as reported by Thomas Koch [0]?

Quoted 10 lines

Couple of things to note, based on feedback previously from the FW team that I found when digging thru my battery related emails.

"Lenovo doesn't officially support the use of these calls - they're intended for internal use" (at this point in time - there is some discussion about battery recalibration support but I don't have details I can share there yet).

The FW team also noted that:

Quoted 8 lines

"Actual battery charging/discharging behaviors are managed by ECFW. So the request of BDSS/BICS method may be rejected by ECFW for the current battery mode/status. You have to check if the request of the method is actually applied or not"

So for the calls above (and for the BDSS calls in the previous patch) it would be good to do a read back of the setting to ensure it has completed successfully.

I'll add that for v2.

Quoted 1 line

Hope that helps

It does, thanks!

Thomas

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

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, Nicolo' Piazzalunga, Thomas Koch:

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]/

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        | 154 +++++++++++++++++++-
  drivers/power/supply/power_supply_sysfs.c   |  51 +++++++
  include/linux/power_supply.h                |  16 ++
  4 files changed, 231 insertions(+), 4 deletions(-)

base-commit: 66f4beaa6c1d28161f534471484b2daa2de1dce0

thank you very much for working on this. It is high time that we leave external kernel modules for ThinkPads behind us.

Quoted 18 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)

Quoted 6 lines

Ognjen Galic, Nicolo' Piazzalunga, Thomas Koch:

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?

S-o-b/Co-developed-by/Tested-by is fine with me.

I tested your patches.

Hardware:

- ThinkPad X220, BAT0
- ThinkPad T450s, BAT0+BAT1
- ThinkPad X1C6, BAT0

Test Results:

1. force-discharge

Everythings works as expected
- Writing including disengaging w/ "auto" : OK
- Reading: OK

- Battery discharging: OK
- Disengaging with "auto": OK

2. inhibit-charge

Works as expected:
- Writing: OK

- Disengaging with "auto": OK

Discrepancies:
- Battery charge inhibited: BAT0 OK, BAT1 no effect e.g. continues charging
- Reading: always returns "auto"

Quoted 54 lines · Thomas Koch

thank you very much for working on this. It is high time that we leave external kernel modules for ThinkPads behind us.

Quoted 18 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)

Quoted 6 lines

Ognjen Galic, Nicolo' Piazzalunga, Thomas Koch:

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?

S-o-b/Co-developed-by/Tested-by is fine with me.

I tested your patches.

Hardware:

- ThinkPad X220, BAT0
- ThinkPad T450s, BAT0+BAT1
- ThinkPad X1C6, BAT0

Test Results:

1. force-discharge

Everythings works as expected
- Writing including disengaging w/ "auto" : OK
- Reading: OK

- Battery discharging: OK
- Disengaging with "auto": OK

2. inhibit-charge

Works as expected:
- Writing: OK

Quoted 6 lines

- Disengaging with "auto": OK

Discrepancies:
- Battery charge inhibited: BAT0 OK, BAT1 no effect e.g. continues charging
- Reading: always returns "auto"

I tested it on a T460s with two batteries and there inhibit-charge works fine for both batteries.
What does not work is setting force-discharge for both batteries at the same time.
This makes somewhat sense as on a physical level probably only one of them can be used at a time.

Mark Pearson: Could you confirm that this is the intended behaviour?

Quoted 54 lines · Thomas Koch

thank you very much for working on this. It is high time that we leave external kernel modules for ThinkPads behind us.

Quoted 18 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)

Quoted 6 lines

Ognjen Galic, Nicolo' Piazzalunga, Thomas Koch:

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?

S-o-b/Co-developed-by/Tested-by is fine with me.

I tested your patches.

Hardware:

- ThinkPad X220, BAT0
- ThinkPad T450s, BAT0+BAT1
- ThinkPad X1C6, BAT0

Test Results:

1. force-discharge

Everythings works as expected
- Writing including disengaging w/ "auto" : OK
- Reading: OK

- Battery discharging: OK
- Disengaging with "auto": OK

2. inhibit-charge

Works as expected:
- Writing: OK

Quoted 6 lines

- Disengaging with "auto": OK

Discrepancies:
- Battery charge inhibited: BAT0 OK, BAT1 no effect e.g. continues charging
- Reading: always returns "auto"

I tested it on a T460s with two batteries and there inhibit-charge works fine for both batteries.
What does not work is setting force-discharge for both batteries at the same time.
This makes somewhat sense as on a physical level probably only one of them can be used at a time.

Mark Pearson: Could you confirm that this is the intended behaviour?

In my changes queued for v2 of the series[0] I added validation of the written settings and an EIO is now reported if the settings were not applied, so this should help userspace handle this situatoin.

The plan is to submit v2 after the first round of review for the core PM changes.

[0] https://git.sr.ht/~t-8ch/linux/tree/charge-control