[PATCH 0/4] power: supply: add charge_behaviour property (force-discharge, inhibit-charge)

[PATCH 0/4] power: supply: add charge_behaviour property (force-discharge, inhibit-charge)

[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, 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/20211108192852.357473-1-linux@weissschuh.net/
https://lore.kernel.org/platform-driver-x86/21569a89-8303-8573-05fb-c2fec29983d1@gmail.com/

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
-- 
2.33.1

[PATCH 1/4] power: supply: add charge_behaviour attributes

[Thomas Weißschuh]

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/21569a89-8303-8573-05fb-c2fec29983d1@gmail.com/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 Documentation/ABI/testing/sysfs-class-power | 14 ++++++++++++++
 include/linux/power_supply.h                |  7 +++++++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index f7904efc4cfa..cece094764f8 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -455,6 +455,20 @@ Description:
 			      "Unknown", "Charging", "Discharging",
 			      "Not charging", "Full"
 
+What:		/sys/class/power_supply/<supply_name>/charge_behaviour
+Date:		November 2021
+Contact:	linux-pm@vger.kernel.org
+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:	linux-pm@vger.kernel.org
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 9ca1f120a211..70c333e86293 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -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,
 };
-- 
2.33.1

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

[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.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_sysfs.c | 51 +++++++++++++++++++++++
 include/linux/power_supply.h              |  9 ++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index c3d7cbcd4fad..171341bcef1d 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -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),
@@ -226,6 +232,51 @@ static enum power_supply_property dev_attr_psp(struct device_attribute *attr)
 	return  to_ps_attr(attr) - power_supply_attrs;
 }
 
+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 += sprintf(buf + count, "[%s] ",
+					 POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[i]);
+			match = true;
+		} else if (available) {
+			count += sprintf(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 (available_behaviours & BIT(i))
+		return i;
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(power_supply_charge_behaviour_parse);
+
 static ssize_t power_supply_show_usb_type(struct device *dev,
 					  const struct power_supply_desc *desc,
 					  union power_supply_propval *value,
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 70c333e86293..71f0379c2af8 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -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__ */
-- 
2.33.1

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

[Thomas Weißschuh]

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

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

---

This patch is based on https://lore.kernel.org/platform-driver-x86/c2504700-06e9-e7d8-80f7-de90b0b6dfb5@gmail.com/
---
 drivers/platform/x86/thinkpad_acpi.c | 103 +++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 9c632df734bb..e8c98e9aff71 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -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,
@@ -9653,6 +9747,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,
 };
 
-- 
2.33.1

[PATCH 4/4] platform/x86: thinkpad_acpi: support inhibit-charge

[Thomas Weißschuh]

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

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

---

This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/
---
 drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index e8c98e9aff71..7cd6475240b2 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -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;
+		if (ret)
 			return ret;
 		break;
 	default:
-- 
2.33.1

Re: [PATCH 4/4] platform/x86: thinkpad_acpi: support inhibit-charge

[Hans de Goede]

Hi Thomas,

Thank you for working on this!

On 11/13/21 11:42, Thomas Weißschuh wrote:
> This adds support for the inhibit-charge charge_behaviour through the
> embedded controller of ThinkPads.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> ---
> 
> This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index e8c98e9aff71..7cd6475240b2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -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)) {

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.

For the other parts of the set lets wait and see what Sebastian has to say.

Regards,

Hans



> +		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:
> 

Re: [PATCH 4/4] platform/x86: thinkpad_acpi: support inhibit-charge

[Thomas Weißschuh]

Hi Hans,

On 2021-11-16 11:58+0100, Hans de Goede wrote:
> Thank you for working on this!

Thanks for the review!

> On 11/13/21 11:42, Thomas Weißschuh wrote:
> > @@ -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.

Indeed, I'll fix this logic for v2.

Thanks,
Thomas

Re: [PATCH 0/4] power: supply: add charge_behaviour property (force-discharge, inhibit-charge)

[Thomas Koch]

Hi Thomas,

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

On 13.11.21 11:42, Thomas Weißschuh wrote:
> 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?
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"

Note: the reading discrepancy may be related to Hans' remarks [1].

[1]
https://lore.kernel.org/all/09a66da1-1a8b-a668-3179-81670303ea37@redhat.com/

>
> 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/20211108192852.357473-1-linux@weissschuh.net/
> https://lore.kernel.org/platform-driver-x86/21569a89-8303-8573-05fb-c2fec29983d1@gmail.com/
>
> 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
>
--
Freundliche Grüße / Kind regards,
Thomas Koch

Mail : linrunner@gmx.net
Web  : https://linrunner.de/tlp

Re: [External] [PATCH 3/4] platform/x86: thinkpad_acpi: support force-discharge

[Mark Pearson]

Hi Thomas,

On 2021-11-13 05:42, Thomas Weißschuh wrote:
> This adds support for the force-discharge charge_behaviour through the
> embedded controller of ThinkPads.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> ---
> 
> This patch is based on https://lore.kernel.org/platform-driver-x86/c2504700-06e9-e7d8-80f7-de90b0b6dfb5@gmail.com/>> ---
>  drivers/platform/x86/thinkpad_acpi.c | 103 +++++++++++++++++++++++++--
>  1 file changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 9c632df734bb..e8c98e9aff71 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -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,
> @@ -9653,6 +9747,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,
>  };
>  
> 
Sorry for the slow review - been busy this week. Thank you for
implementing this - it's great to be getting this into the kernel and
it's something we have had on our todo list for a little while now.

I can confirm the BDSG and BDSS APIs are correct along with the bit
field defines you have used. Not sure if that's helpful but as I have
access to some internal documents I figured I'd check :)

I'll note that bit 10 in BDSG should flag if the 'enable breaking by AC
detaching' feature is supported. You're not using it so I don't think
that's important - but figured I'd mention it because of the comment
that not all thinkpads support it.

Mark

Re: [External] [PATCH 4/4] platform/x86: thinkpad_acpi: support inhibit-charge

[Mark Pearson]

Hi Thomas,

On 2021-11-13 05:42, Thomas Weißschuh wrote:
> This adds support for the inhibit-charge charge_behaviour through the
> embedded controller of ThinkPads.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> ---
> 
> This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/>> ---
>  drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index e8c98e9aff71..7cd6475240b2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -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;
> +		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
Mark

Re: [External] [PATCH 4/4] platform/x86: thinkpad_acpi: support inhibit-charge

[Thomas Weißschuh]

Hi Mark,

On 2021-11-16 15:52-0500, Mark Pearson wrote:
> On 2021-11-13 05:42, Thomas Weißschuh wrote:
> > This adds support for the inhibit-charge charge_behaviour through the
> > embedded controller of ThinkPads.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > 
> > ---
> > 
> > This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/>> ---
> >  drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> > index e8c98e9aff71..7cd6475240b2 100644
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -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;
> > +		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]?

> 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.

I'll add that for v2.

> Hope that helps

It does, thanks!

Thomas

[0] https://lore.kernel.org/platform-driver-x86/9cebba85-f399-a7aa-91f7-237852338dc5@gmx.net/

Re: [External] [PATCH 4/4] platform/x86: thinkpad_acpi: support inhibit-charge

[Mark Pearson]

Hi Thomas

On 2021-11-16 18:44, Thomas Weißschuh wrote:
> Hi Mark,
> 
> On 2021-11-16 15:52-0500, Mark Pearson wrote:
>> On 2021-11-13 05:42, Thomas Weißschuh wrote:
>>> This adds support for the inhibit-charge charge_behaviour through the
>>> embedded controller of ThinkPads.
>>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>
>>> ---
>>>
>>> This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/ ---
>>>  drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++-
>>>  1 file changed, 53 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>> index e8c98e9aff71..7cd6475240b2 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -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;
>>> +		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]?
> 
>> 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.
> 
> I'll add that for v2.
> 
>> Hope that helps
> 
> It does, thanks!
> 
> Thomas
> 
> [0] https://lore.kernel.org/platform-driver-x86/9cebba85-f399-a7aa-91f7-237852338dc5@gmx.net/>> 
I got confirmation that:

<quote>
BDSS have to be used with specific battery. Please use with Primary(=1b)
or Secondary(2b) as Battery ID (Bit9-8)

Bit 9-8: Battery ID
= 0: Any battery
= 1: Primary battery
= 2: Secondary battery
</quote>

It seems you can't use BDSS with all batteries.
I'll confirm on BICS what the limitations are, they didn't update on
that piece yet I'm afraid. Unfortunately I don't think I have any
systems with two batteries to test myself.

Mark

Re: [PATCH 0/4] power: supply: add charge_behaviour property (force-discharge, inhibit-charge)

[Thomas Weißschuh]

On 2021-11-16 17:56+0100, Thomas Koch wrote:
> thank you very much for working on this. It is high time that we leave
> external kernel modules for ThinkPads behind us.
> 
> On 13.11.21 11:42, Thomas Weißschuh wrote:
> > 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?
> 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"

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

Re: [External] Re: [PATCH 0/4] power: supply: add charge_behaviour property (force-discharge, inhibit-charge)

[Mark Pearson]

Hi Thomas,


On 2021-11-17 12:57, Thomas Weißschuh wrote:
> On 2021-11-16 17:56+0100, Thomas Koch wrote:
>> thank you very much for working on this. It is high time that we leave
>> external kernel modules for ThinkPads behind us.
>>
>> On 13.11.21 11:42, Thomas Weißschuh wrote:
>>> 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?
>> 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"
> 
> 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?
> 
Confirmed - only one battery can be used with the BDSS command

Re: [PATCH 0/4] power: supply: add charge_behaviour property (force-discharge, inhibit-charge)

[Thomas Koch]

Hi Thomas,

On 17.11.21 18:57, Thomas Weißschuh wrote:
> On 2021-11-16 17:56+0100, Thomas Koch wrote:
>> thank you very much for working on this. It is high time that we leave
>> external kernel modules for ThinkPads behind us.
>>
>> On 13.11.21 11:42, Thomas Weißschuh wrote:
>>> 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?
>> 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"
>
> 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.

My experience confirms your consideration. The ThinkPad battery circuit
can handle exactly one battery at a time i.e.
- Charging, AC connected
- Forced discharging,  AC connected
- Discharging, AC disconnected
The other battery is always idle during this 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.

Please wait until i'm finished with testing your queued v2. I am getting
errors here and would first like to rule out homemade problems with my
kernel build and/or base version.

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


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

Mail : linrunner@gmx.net
Web  : https://linrunner.de/tlp