[PATCH 1/3] thinkpad_acpi: add support for force_discharge

[PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Nicolo' Piazzalunga]

Lenovo ThinkPad systems have a feature that lets you
force the battery to discharge when AC is attached.

This patch implements that feature and exposes it via the generic
ACPI battery driver in the generic location:

/sys/class/power_supply/BATx/force_discharge

Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
Signed-off-by: Thomas Koch <linrunner@gmx.net>
Signed-off-by: Nicolo' Piazzalunga <nicolopiazzalunga@gmail.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 59 ++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 9c4df41687a3..6c7dca3a10d2 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9317,6 +9317,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,
@@ -9333,6 +9335,7 @@ enum {
 	/* This is used in the get/set helpers */
 	THRESHOLD_START,
 	THRESHOLD_STOP,
+	FORCE_DISCHARGE
 };
 
 struct tpacpi_battery_data {
@@ -9340,6 +9343,7 @@ struct tpacpi_battery_data {
 	int start_support;
 	int charge_stop;
 	int stop_support;
+	int discharge_support;
 };
 
 struct tpacpi_battery_driver_data {
@@ -9397,6 +9401,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;
@@ -9425,6 +9435,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;
@@ -9443,6 +9463,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)) {
@@ -9479,11 +9501,16 @@ static int tpacpi_battery_probe(int battery)
 			return -ENODEV;
 		}
 	}
-	pr_info("battery %d registered (start %d, stop %d)",
+	if (acpi_has_method(hkey_handle, GET_DISCHARGE))
+		if (!ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_DISCHARGE, &ret, battery)))
+			/* Support is marked in bit 8 */
+			battery_info.batteries[battery].discharge_support = ret & BIT(8);
+
+	pr_info("battery %d registered (start %d, stop %d, force: %d)",
 			battery,
 			battery_info.batteries[battery].charge_start,
-			battery_info.batteries[battery].charge_stop);
-
+			battery_info.batteries[battery].charge_stop,
+			battery_info.batteries[battery].discharge_support);
 	return 0;
 }
 
@@ -9569,6 +9596,15 @@ static ssize_t tpacpi_battery_store(int what,
 		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
 			return -EINVAL;
 		return count;
+	case FORCE_DISCHARGE:
+		if (!battery_info.batteries[battery].discharge_support)
+			return -ENODEV;
+		/* The only valid values are 1 and 0 */
+		if (value != 0 && value != 1)
+			return -EINVAL;
+		if (tpacpi_battery_set(FORCE_DISCHARGE, battery, value))
+			return -ENODEV;
+		return count;
 	default:
 		pr_crit("Wrong parameter: %d", what);
 		return -EINVAL;
@@ -9617,6 +9653,13 @@ static ssize_t charge_control_end_threshold_show(struct device *device,
 	return tpacpi_battery_show(THRESHOLD_STOP, device, buf);
 }
 
+static ssize_t force_discharge_show(struct device *device,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return tpacpi_battery_show(FORCE_DISCHARGE, device, buf);
+}
+
 static ssize_t charge_control_start_threshold_store(struct device *dev,
 				struct device_attribute *attr,
 				const char *buf, size_t count)
@@ -9631,8 +9674,16 @@ static ssize_t charge_control_end_threshold_store(struct device *dev,
 	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
 }
 
+static ssize_t force_discharge_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	return tpacpi_battery_store(FORCE_DISCHARGE, dev, buf, count);
+}
+
 static DEVICE_ATTR_RW(charge_control_start_threshold);
 static DEVICE_ATTR_RW(charge_control_end_threshold);
+static DEVICE_ATTR_RW(force_discharge);
 static struct device_attribute dev_attr_charge_start_threshold = __ATTR(
 	charge_start_threshold,
 	0644,
@@ -9645,12 +9696,12 @@ static struct device_attribute dev_attr_charge_stop_threshold = __ATTR(
 	charge_control_end_threshold_show,
 	charge_control_end_threshold_store
 );
-
 static struct attribute *tpacpi_battery_attrs[] = {
 	&dev_attr_charge_control_start_threshold.attr,
 	&dev_attr_charge_control_end_threshold.attr,
 	&dev_attr_charge_start_threshold.attr,
 	&dev_attr_charge_stop_threshold.attr,
+	&dev_attr_force_discharge.attr,
 	NULL,
 };
 
-- 
2.25.1

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Hans de Goede]

Hi Nicola,

Thank you for your patch series.

I'm not sure what to do with these. I have a couple of concerns here:

1. These features are useful, but not super useful and as such I wonder
how often they are used and this how well tested the firmware is wrt these.
I have added Mark and Nitin from Lenovo to the Cc. Mark, Nitin, can you
comment on if it is ok from a firmware pov to try and use the following
battery related ACPI methods on all thinkpads? :

#define GET_DISCHARGE	"BDSG"
#define SET_DISCHARGE	"BDSS"
#define GET_INHIBIT	"PSSG"
#define SET_INHIBIT	"BICS"


2. If we add support for this to the kernel we should probably
first agree on standardized power-supply class property names for
these, rather then coming up with our own names. ATM we register
2 names for the charge start threshold, the one which the thinkpad_acpi
code invented and the standardized name which was later added.

I've added Sebastian, the power-supply class / driver maintainer to
the Cc. for this. Sebastian Nicolo wants to add support for 2 new
features as power-supply properties:

--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
...
+Battery forced discharging
+--------------------------
+
+sysfs attribute:
+/sys/class/power_supply/BATx/force_discharge
+
+Setting this attribute to 1 forces the battery to discharge while AC is attached.
+Setting it to 0 terminates forced discharging.
+
+Battery charge inhibiting
+--------------------------
+
+sysfs attribute:
+/sys/class/power_supply/BATx/inhibit_discharge
+
+Setting this attribute to 1 stops charging of the battery as a manual override
+over the threshold attributes. Setting it to 0 terminates the override.

Sebastian, I believe that this should be changes to instead be documented
in: Documentation/ABI/testing/sysfs-class-power
and besides the rename I was wondering if you have any remarks on the proposed
API before Nicolo sends out a v2 ?

Regards,

Hans


On 3/17/21 3:01 PM, Nicolo' Piazzalunga wrote:
> Lenovo ThinkPad systems have a feature that lets you
> force the battery to discharge when AC is attached.
> 
> This patch implements that feature and exposes it via the generic
> ACPI battery driver in the generic location:
> 
> /sys/class/power_supply/BATx/force_discharge
> 
> Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
> Signed-off-by: Thomas Koch <linrunner@gmx.net>
> Signed-off-by: Nicolo' Piazzalunga <nicolopiazzalunga@gmail.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 59 ++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 9c4df41687a3..6c7dca3a10d2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9317,6 +9317,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,
> @@ -9333,6 +9335,7 @@ enum {
>  	/* This is used in the get/set helpers */
>  	THRESHOLD_START,
>  	THRESHOLD_STOP,
> +	FORCE_DISCHARGE
>  };
>  
>  struct tpacpi_battery_data {
> @@ -9340,6 +9343,7 @@ struct tpacpi_battery_data {
>  	int start_support;
>  	int charge_stop;
>  	int stop_support;
> +	int discharge_support;
>  };
>  
>  struct tpacpi_battery_driver_data {
> @@ -9397,6 +9401,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;
> @@ -9425,6 +9435,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;
> @@ -9443,6 +9463,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)) {
> @@ -9479,11 +9501,16 @@ static int tpacpi_battery_probe(int battery)
>  			return -ENODEV;
>  		}
>  	}
> -	pr_info("battery %d registered (start %d, stop %d)",
> +	if (acpi_has_method(hkey_handle, GET_DISCHARGE))
> +		if (!ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_DISCHARGE, &ret, battery)))
> +			/* Support is marked in bit 8 */
> +			battery_info.batteries[battery].discharge_support = ret & BIT(8);
> +
> +	pr_info("battery %d registered (start %d, stop %d, force: %d)",
>  			battery,
>  			battery_info.batteries[battery].charge_start,
> -			battery_info.batteries[battery].charge_stop);
> -
> +			battery_info.batteries[battery].charge_stop,
> +			battery_info.batteries[battery].discharge_support);
>  	return 0;
>  }
>  
> @@ -9569,6 +9596,15 @@ static ssize_t tpacpi_battery_store(int what,
>  		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
>  			return -EINVAL;
>  		return count;
> +	case FORCE_DISCHARGE:
> +		if (!battery_info.batteries[battery].discharge_support)
> +			return -ENODEV;
> +		/* The only valid values are 1 and 0 */
> +		if (value != 0 && value != 1)
> +			return -EINVAL;
> +		if (tpacpi_battery_set(FORCE_DISCHARGE, battery, value))
> +			return -ENODEV;
> +		return count;
>  	default:
>  		pr_crit("Wrong parameter: %d", what);
>  		return -EINVAL;
> @@ -9617,6 +9653,13 @@ static ssize_t charge_control_end_threshold_show(struct device *device,
>  	return tpacpi_battery_show(THRESHOLD_STOP, device, buf);
>  }
>  
> +static ssize_t force_discharge_show(struct device *device,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	return tpacpi_battery_show(FORCE_DISCHARGE, device, buf);
> +}
> +
>  static ssize_t charge_control_start_threshold_store(struct device *dev,
>  				struct device_attribute *attr,
>  				const char *buf, size_t count)
> @@ -9631,8 +9674,16 @@ static ssize_t charge_control_end_threshold_store(struct device *dev,
>  	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
>  }
>  
> +static ssize_t force_discharge_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	return tpacpi_battery_store(FORCE_DISCHARGE, dev, buf, count);
> +}
> +
>  static DEVICE_ATTR_RW(charge_control_start_threshold);
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
> +static DEVICE_ATTR_RW(force_discharge);
>  static struct device_attribute dev_attr_charge_start_threshold = __ATTR(
>  	charge_start_threshold,
>  	0644,
> @@ -9645,12 +9696,12 @@ static struct device_attribute dev_attr_charge_stop_threshold = __ATTR(
>  	charge_control_end_threshold_show,
>  	charge_control_end_threshold_store
>  );
> -
>  static struct attribute *tpacpi_battery_attrs[] = {
>  	&dev_attr_charge_control_start_threshold.attr,
>  	&dev_attr_charge_control_end_threshold.attr,
>  	&dev_attr_charge_start_threshold.attr,
>  	&dev_attr_charge_stop_threshold.attr,
> +	&dev_attr_force_discharge.attr,
>  	NULL,
>  };
>  
> 

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Barnabás Pőcze]

Hi


2021. április 7., szerda 12:24 keltezéssel, Hans de Goede írta:

> Hi Nicola,
>
> Thank you for your patch series.
>
> I'm not sure what to do with these. I have a couple of concerns here:
>
> 1. These features are useful, but not super useful and as such I wonder
> how often they are used and this how well tested the firmware is wrt these.
> I have added Mark and Nitin from Lenovo to the Cc. Mark, Nitin, can you
> comment on if it is ok from a firmware pov to try and use the following
> battery related ACPI methods on all thinkpads? :
>
> #define GET_DISCHARGE	"BDSG"
> #define SET_DISCHARGE	"BDSS"
> #define GET_INHIBIT	"PSSG"
> #define SET_INHIBIT	"BICS"
>
>
> 2. If we add support for this to the kernel we should probably
> first agree on standardized power-supply class property names for
> these, rather then coming up with our own names. ATM we register
> 2 names for the charge start threshold, the one which the thinkpad_acpi
> code invented and the standardized name which was later added.
>
> I've added Sebastian, the power-supply class / driver maintainer to
> the Cc. for this. Sebastian Nicolo wants to add support for 2 new
> features as power-supply properties:
>
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> ...
> +Battery forced discharging
> +--------------------------
> +
> +sysfs attribute:
> +/sys/class/power_supply/BATx/force_discharge
> +
> +Setting this attribute to 1 forces the battery to discharge while AC is attached.
> +Setting it to 0 terminates forced discharging.
> +
> +Battery charge inhibiting
> +--------------------------
> +
> +sysfs attribute:
> +/sys/class/power_supply/BATx/inhibit_discharge
> +
> +Setting this attribute to 1 stops charging of the battery as a manual override
> +over the threshold attributes. Setting it to 0 terminates the override.
>

"inhibit_**discharge**"
"stops **charging** of the battery"

I'm wondering if it should be "inhibit_charge" or something like that?


> Sebastian, I believe that this should be changes to instead be documented
> in: Documentation/ABI/testing/sysfs-class-power
> and besides the rename I was wondering if you have any remarks on the proposed
> API before Nicolo sends out a v2 ?
>
> Regards,
>
> Hans
>
>
> On 3/17/21 3:01 PM, Nicolo' Piazzalunga wrote:
> > Lenovo ThinkPad systems have a feature that lets you
> > force the battery to discharge when AC is attached.
> >
> > This patch implements that feature and exposes it via the generic
> > ACPI battery driver in the generic location:
> >
> > /sys/class/power_supply/BATx/force_discharge
> >
> > Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
> > Signed-off-by: Thomas Koch <linrunner@gmx.net>
> > Signed-off-by: Nicolo' Piazzalunga <nicolopiazzalunga@gmail.com>
> [...]


Regards,
Barnabás Pőcze

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Thomas Koch]

Hi Hans,



 > 1. These features are useful, but not super useful and as such I wonder

 > how often they are used and this how well tested the firmware is wrt
these.

 > I have added Mark and Nitin from Lenovo to the Cc. Mark, Nitin, can you

 > comment on if it is ok from a firmware pov to try and use the following

 > battery related ACPI methods on all thinkpads? :
 > #define GET_DISCHARGE	"BDSG"


 > #define SET_DISCHARGE	"BDSS"


 > #define GET_INHIBIT	"PSSG"


 > #define SET_INHIBIT	"BICS"



These ACPI methods are present in (with very few exceptions) all
ThinkPads released since 2012. I am curious to hear what Mark and Nitin
have to say, never read anything official about it.



Since 2012 there is also userspace tool tpacpi-bat [1] employing them
along with those for the start/stop threshold.



My own tool TLP makes use of tpacpi-bat for force_discharge also since
2012. From my experience in TLP support i can say there's a significant
user base and those who use thresholds also want to use force_discharge
for recalibration from time to time.


The patches at hand work flawlessly on my small ThinkPad collection.

[1] https://github.com/teleshoes/tpacpi-bat



--

Freundliche Grüße / Kind regards,

Thomas Koch



Mail : linrunner@gmx.net

Web  : https://linrunner.de/tlp


On 07.04.21 12:24, Hans de Goede wrote:
> Hi Nicola,
>
> Thank you for your patch series.
>
> I'm not sure what to do with these. I have a couple of concerns here:
>
> 1. These features are useful, but not super useful and as such I wonder
> how often they are used and this how well tested the firmware is wrt these.
> I have added Mark and Nitin from Lenovo to the Cc. Mark, Nitin, can you
> comment on if it is ok from a firmware pov to try and use the following
> battery related ACPI methods on all thinkpads? :
>
> #define GET_DISCHARGE	"BDSG"
> #define SET_DISCHARGE	"BDSS"
> #define GET_INHIBIT	"PSSG"
> #define SET_INHIBIT	"BICS"
>
>
> 2. If we add support for this to the kernel we should probably
> first agree on standardized power-supply class property names for
> these, rather then coming up with our own names. ATM we register
> 2 names for the charge start threshold, the one which the thinkpad_acpi
> code invented and the standardized name which was later added.
>
> I've added Sebastian, the power-supply class / driver maintainer to
> the Cc. for this. Sebastian Nicolo wants to add support for 2 new
> features as power-supply properties:
>
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> ...
> +Battery forced discharging
> +--------------------------
> +
> +sysfs attribute:
> +/sys/class/power_supply/BATx/force_discharge
> +
> +Setting this attribute to 1 forces the battery to discharge while AC is attached.
> +Setting it to 0 terminates forced discharging.
> +
> +Battery charge inhibiting
> +--------------------------
> +
> +sysfs attribute:
> +/sys/class/power_supply/BATx/inhibit_discharge
> +
> +Setting this attribute to 1 stops charging of the battery as a manual override
> +over the threshold attributes. Setting it to 0 terminates the override.
>
> Sebastian, I believe that this should be changes to instead be documented
> in: Documentation/ABI/testing/sysfs-class-power
> and besides the rename I was wondering if you have any remarks on the proposed
> API before Nicolo sends out a v2 ?
>
> Regards,
>
> Hans
>
>
> On 3/17/21 3:01 PM, Nicolo' Piazzalunga wrote:
>> Lenovo ThinkPad systems have a feature that lets you
>> force the battery to discharge when AC is attached.
>>
>> This patch implements that feature and exposes it via the generic
>> ACPI battery driver in the generic location:
>>
>> /sys/class/power_supply/BATx/force_discharge
>>
>> Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
>> Signed-off-by: Thomas Koch <linrunner@gmx.net>
>> Signed-off-by: Nicolo' Piazzalunga <nicolopiazzalunga@gmail.com>
>> ---
>>   drivers/platform/x86/thinkpad_acpi.c | 59 ++++++++++++++++++++++++++--
>>   1 file changed, 55 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 9c4df41687a3..6c7dca3a10d2 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -9317,6 +9317,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,
>> @@ -9333,6 +9335,7 @@ enum {
>>   	/* This is used in the get/set helpers */
>>   	THRESHOLD_START,
>>   	THRESHOLD_STOP,
>> +	FORCE_DISCHARGE
>>   };
>>
>>   struct tpacpi_battery_data {
>> @@ -9340,6 +9343,7 @@ struct tpacpi_battery_data {
>>   	int start_support;
>>   	int charge_stop;
>>   	int stop_support;
>> +	int discharge_support;
>>   };
>>
>>   struct tpacpi_battery_driver_data {
>> @@ -9397,6 +9401,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;
>> @@ -9425,6 +9435,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;
>> @@ -9443,6 +9463,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)) {
>> @@ -9479,11 +9501,16 @@ static int tpacpi_battery_probe(int battery)
>>   			return -ENODEV;
>>   		}
>>   	}
>> -	pr_info("battery %d registered (start %d, stop %d)",
>> +	if (acpi_has_method(hkey_handle, GET_DISCHARGE))
>> +		if (!ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_DISCHARGE, &ret, battery)))
>> +			/* Support is marked in bit 8 */
>> +			battery_info.batteries[battery].discharge_support = ret & BIT(8);
>> +
>> +	pr_info("battery %d registered (start %d, stop %d, force: %d)",
>>   			battery,
>>   			battery_info.batteries[battery].charge_start,
>> -			battery_info.batteries[battery].charge_stop);
>> -
>> +			battery_info.batteries[battery].charge_stop,
>> +			battery_info.batteries[battery].discharge_support);
>>   	return 0;
>>   }
>>
>> @@ -9569,6 +9596,15 @@ static ssize_t tpacpi_battery_store(int what,
>>   		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
>>   			return -EINVAL;
>>   		return count;
>> +	case FORCE_DISCHARGE:
>> +		if (!battery_info.batteries[battery].discharge_support)
>> +			return -ENODEV;
>> +		/* The only valid values are 1 and 0 */
>> +		if (value != 0 && value != 1)
>> +			return -EINVAL;
>> +		if (tpacpi_battery_set(FORCE_DISCHARGE, battery, value))
>> +			return -ENODEV;
>> +		return count;
>>   	default:
>>   		pr_crit("Wrong parameter: %d", what);
>>   		return -EINVAL;
>> @@ -9617,6 +9653,13 @@ static ssize_t charge_control_end_threshold_show(struct device *device,
>>   	return tpacpi_battery_show(THRESHOLD_STOP, device, buf);
>>   }
>>
>> +static ssize_t force_discharge_show(struct device *device,
>> +				struct device_attribute *attr,
>> +				char *buf)
>> +{
>> +	return tpacpi_battery_show(FORCE_DISCHARGE, device, buf);
>> +}
>> +
>>   static ssize_t charge_control_start_threshold_store(struct device *dev,
>>   				struct device_attribute *attr,
>>   				const char *buf, size_t count)
>> @@ -9631,8 +9674,16 @@ static ssize_t charge_control_end_threshold_store(struct device *dev,
>>   	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
>>   }
>>
>> +static ssize_t force_discharge_store(struct device *dev,
>> +				struct device_attribute *attr,
>> +				const char *buf, size_t count)
>> +{
>> +	return tpacpi_battery_store(FORCE_DISCHARGE, dev, buf, count);
>> +}
>> +
>>   static DEVICE_ATTR_RW(charge_control_start_threshold);
>>   static DEVICE_ATTR_RW(charge_control_end_threshold);
>> +static DEVICE_ATTR_RW(force_discharge);
>>   static struct device_attribute dev_attr_charge_start_threshold = __ATTR(
>>   	charge_start_threshold,
>>   	0644,
>> @@ -9645,12 +9696,12 @@ static struct device_attribute dev_attr_charge_stop_threshold = __ATTR(
>>   	charge_control_end_threshold_show,
>>   	charge_control_end_threshold_store
>>   );
>> -
>>   static struct attribute *tpacpi_battery_attrs[] = {
>>   	&dev_attr_charge_control_start_threshold.attr,
>>   	&dev_attr_charge_control_end_threshold.attr,
>>   	&dev_attr_charge_start_threshold.attr,
>>   	&dev_attr_charge_stop_threshold.attr,
>> +	&dev_attr_force_discharge.attr,
>>   	NULL,
>>   };
>>
>>
>

Re: [External] Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Mark Pearson]

Hi Thomas, Hans and Nicolo

On 07/04/2021 08:19, Thomas Koch wrote:
> Hi Hans,
> 
>> 1. These features are useful, but not super useful and as such I wonder
>> how often they are used and this how well tested the firmware is wrt
>> these.
> 
>> I have added Mark and Nitin from Lenovo to the Cc. Mark, Nitin, can you
>> comment on if it is ok from a firmware pov to try and use the following
>> battery related ACPI methods on all thinkpads? :
>> #define GET_DISCHARGE    "BDSG"
>> #define SET_DISCHARGE    "BDSS"
>> #define GET_INHIBIT    "PSSG"
>> #define SET_INHIBIT    "BICS"
> 
> These ACPI methods are present in (with very few exceptions) all
> ThinkPads released since 2012. I am curious to hear what Mark and Nitin
> have to say, never read anything official about it.

I'm afraid I've not come across these myself before, but will go and ask
the firmware team.
<For my internal reference LO-1115>

It would be good to confirm the implementation details if I can. I found
recently that some of the temperature sensors that are read in by
thinkpad_acpi from the EC RAM are not temp sensors (patch coming
soon....hopefully later today). Hopefully I can check the internal spec
and give a thumbs up on the implementation - even if we're not allowed
to share the actual paperwork (maybe one day....)

> 
> Since 2012 there is also userspace tool tpacpi-bat [1] employing them
> along with those for the start/stop threshold.
> 
> My own tool TLP makes use of tpacpi-bat for force_discharge also since
> 2012. From my experience in TLP support i can say there's a significant
> user base and those who use thresholds also want to use force_discharge
> for recalibration from time to time.

This probably isn't the right place for the discussion, but I've been
meaning to dig into battery management but never really get the time. I
know in the windows world that ThinkVantage has extra hooks for setting
thresholds and I wanted to see what we can do on the Linux side. If
there is anything that would be particularly helpful that is missing
please let me know.

Thanks
Mark

> 
> 
> The patches at hand work flawlessly on my small ThinkPad collection.
> [1] https://github.com/teleshoes/tpacpi-bat
> 
> 
> 
> -- 
> 
> Freundliche Grüße / Kind regards,
> 
> Thomas Koch
> 
> 
> 
> Mail : linrunner@gmx.net
> 
> Web  : https://linrunner.de/tlp
> 
> 
> On 07.04.21 12:24, Hans de Goede wrote:
>> Hi Nicola,
>>
>> Thank you for your patch series.
>>
>> I'm not sure what to do with these. I have a couple of concerns here:
>>
>> 1. These features are useful, but not super useful and as such I wonder
>> how often they are used and this how well tested the firmware is wrt
>> these.
>> I have added Mark and Nitin from Lenovo to the Cc. Mark, Nitin, can you
>> comment on if it is ok from a firmware pov to try and use the following
>> battery related ACPI methods on all thinkpads? :
>>
>> #define GET_DISCHARGE    "BDSG"
>> #define SET_DISCHARGE    "BDSS"
>> #define GET_INHIBIT    "PSSG"
>> #define SET_INHIBIT    "BICS"
>>
>>
>> 2. If we add support for this to the kernel we should probably
>> first agree on standardized power-supply class property names for
>> these, rather then coming up with our own names. ATM we register
>> 2 names for the charge start threshold, the one which the thinkpad_acpi
>> code invented and the standardized name which was later added.
>>
>> I've added Sebastian, the power-supply class / driver maintainer to
>> the Cc. for this. Sebastian Nicolo wants to add support for 2 new
>> features as power-supply properties:
>>
>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> ...
>> +Battery forced discharging
>> +--------------------------
>> +
>> +sysfs attribute:
>> +/sys/class/power_supply/BATx/force_discharge
>> +
>> +Setting this attribute to 1 forces the battery to discharge while AC
>> is attached.
>> +Setting it to 0 terminates forced discharging.
>> +
>> +Battery charge inhibiting
>> +--------------------------
>> +
>> +sysfs attribute:
>> +/sys/class/power_supply/BATx/inhibit_discharge
>> +
>> +Setting this attribute to 1 stops charging of the battery as a manual
>> override
>> +over the threshold attributes. Setting it to 0 terminates the override.
>>
>> Sebastian, I believe that this should be changes to instead be documented
>> in: Documentation/ABI/testing/sysfs-class-power
>> and besides the rename I was wondering if you have any remarks on the
>> proposed
>> API before Nicolo sends out a v2 ?
>>
>> Regards,
>>
>> Hans
>>
>>
>> On 3/17/21 3:01 PM, Nicolo' Piazzalunga wrote:
>>> Lenovo ThinkPad systems have a feature that lets you
>>> force the battery to discharge when AC is attached.
>>>
>>> This patch implements that feature and exposes it via the generic
>>> ACPI battery driver in the generic location:
>>>
>>> /sys/class/power_supply/BATx/force_discharge
>>>
>>> Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
>>> Signed-off-by: Thomas Koch <linrunner@gmx.net>
>>> Signed-off-by: Nicolo' Piazzalunga <nicolopiazzalunga@gmail.com>
>>> ---
>>>   drivers/platform/x86/thinkpad_acpi.c | 59 ++++++++++++++++++++++++++--
>>>   1 file changed, 55 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>>> b/drivers/platform/x86/thinkpad_acpi.c
>>> index 9c4df41687a3..6c7dca3a10d2 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -9317,6 +9317,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,
>>> @@ -9333,6 +9335,7 @@ enum {
>>>       /* This is used in the get/set helpers */
>>>       THRESHOLD_START,
>>>       THRESHOLD_STOP,
>>> +    FORCE_DISCHARGE
>>>   };
>>>
>>>   struct tpacpi_battery_data {
>>> @@ -9340,6 +9343,7 @@ struct tpacpi_battery_data {
>>>       int start_support;
>>>       int charge_stop;
>>>       int stop_support;
>>> +    int discharge_support;
>>>   };
>>>
>>>   struct tpacpi_battery_driver_data {
>>> @@ -9397,6 +9401,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;
>>> @@ -9425,6 +9435,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;
>>> @@ -9443,6 +9463,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)) {
>>> @@ -9479,11 +9501,16 @@ static int tpacpi_battery_probe(int battery)
>>>               return -ENODEV;
>>>           }
>>>       }
>>> -    pr_info("battery %d registered (start %d, stop %d)",
>>> +    if (acpi_has_method(hkey_handle, GET_DISCHARGE))
>>> +        if (!ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_DISCHARGE,
>>> &ret, battery)))
>>> +            /* Support is marked in bit 8 */
>>> +            battery_info.batteries[battery].discharge_support = ret
>>> & BIT(8);
>>> +
>>> +    pr_info("battery %d registered (start %d, stop %d, force: %d)",
>>>               battery,
>>>               battery_info.batteries[battery].charge_start,
>>> -            battery_info.batteries[battery].charge_stop);
>>> -
>>> +            battery_info.batteries[battery].charge_stop,
>>> +            battery_info.batteries[battery].discharge_support);
>>>       return 0;
>>>   }
>>>
>>> @@ -9569,6 +9596,15 @@ static ssize_t tpacpi_battery_store(int what,
>>>           if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
>>>               return -EINVAL;
>>>           return count;
>>> +    case FORCE_DISCHARGE:
>>> +        if (!battery_info.batteries[battery].discharge_support)
>>> +            return -ENODEV;
>>> +        /* The only valid values are 1 and 0 */
>>> +        if (value != 0 && value != 1)
>>> +            return -EINVAL;
>>> +        if (tpacpi_battery_set(FORCE_DISCHARGE, battery, value))
>>> +            return -ENODEV;
>>> +        return count;
>>>       default:
>>>           pr_crit("Wrong parameter: %d", what);
>>>           return -EINVAL;
>>> @@ -9617,6 +9653,13 @@ static ssize_t
>>> charge_control_end_threshold_show(struct device *device,
>>>       return tpacpi_battery_show(THRESHOLD_STOP, device, buf);
>>>   }
>>>
>>> +static ssize_t force_discharge_show(struct device *device,
>>> +                struct device_attribute *attr,
>>> +                char *buf)
>>> +{
>>> +    return tpacpi_battery_show(FORCE_DISCHARGE, device, buf);
>>> +}
>>> +
>>>   static ssize_t charge_control_start_threshold_store(struct device
>>> *dev,
>>>                   struct device_attribute *attr,
>>>                   const char *buf, size_t count)
>>> @@ -9631,8 +9674,16 @@ static ssize_t
>>> charge_control_end_threshold_store(struct device *dev,
>>>       return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
>>>   }
>>>
>>> +static ssize_t force_discharge_store(struct device *dev,
>>> +                struct device_attribute *attr,
>>> +                const char *buf, size_t count)
>>> +{
>>> +    return tpacpi_battery_store(FORCE_DISCHARGE, dev, buf, count);
>>> +}
>>> +
>>>   static DEVICE_ATTR_RW(charge_control_start_threshold);
>>>   static DEVICE_ATTR_RW(charge_control_end_threshold);
>>> +static DEVICE_ATTR_RW(force_discharge);
>>>   static struct device_attribute dev_attr_charge_start_threshold =
>>> __ATTR(
>>>       charge_start_threshold,
>>>       0644,
>>> @@ -9645,12 +9696,12 @@ static struct device_attribute
>>> dev_attr_charge_stop_threshold = __ATTR(
>>>       charge_control_end_threshold_show,
>>>       charge_control_end_threshold_store
>>>   );
>>> -
>>>   static struct attribute *tpacpi_battery_attrs[] = {
>>>       &dev_attr_charge_control_start_threshold.attr,
>>>       &dev_attr_charge_control_end_threshold.attr,
>>>       &dev_attr_charge_start_threshold.attr,
>>>       &dev_attr_charge_stop_threshold.attr,
>>> +    &dev_attr_force_discharge.attr,
>>>       NULL,
>>>   };
>>>
>>>
>>
> 

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3306 bytes --]

Hi,

On Wed, Apr 07, 2021 at 10:33:41AM +0000, Barnabás Pőcze wrote:
> 2021. április 7., szerda 12:24 keltezéssel, Hans de Goede írta:
> > 2. If we add support for this to the kernel we should probably
> > first agree on standardized power-supply class property names for
> > these, rather then coming up with our own names. ATM we register
> > 2 names for the charge start threshold, the one which the thinkpad_acpi
> > code invented and the standardized name which was later added.
> >
> > I've added Sebastian, the power-supply class / driver maintainer to
> > the Cc. for this. Sebastian Nicolo wants to add support for 2 new
> > features as power-supply properties:
> >
> > --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > ...
> > +Battery forced discharging
> > +--------------------------
> > +
> > +sysfs attribute:
> > +/sys/class/power_supply/BATx/force_discharge
> > +
> > +Setting this attribute to 1 forces the battery to discharge while AC is attached.
> > +Setting it to 0 terminates forced discharging.
> > +
> > +Battery charge inhibiting
> > +--------------------------
> > +
> > +sysfs attribute:
> > +/sys/class/power_supply/BATx/inhibit_discharge
> > +
> > +Setting this attribute to 1 stops charging of the battery as a manual override
> > +over the threshold attributes. Setting it to 0 terminates the override.
> >
> 
> "inhibit_**discharge**"
> "stops **charging** of the battery"
> 
> I'm wondering if it should be "inhibit_charge" or something like that?

Text and file name also seem to have reverse meaning for me. I
assume the text is the correct one, since it does not seem to
make sense inhibiting discharge. That would result in instant
poweroff on AC loss?

> > Sebastian, I believe that this should be changes to instead be documented
> > in: Documentation/ABI/testing/sysfs-class-power
> > and besides the rename I was wondering if you have any remarks on the proposed
> > API before Nicolo sends out a v2 ?

IIUIC you have 'force_discharge', which basically means the system
is running from battery power despite an AC adapter being connected
and 'inhibit_discharge', which inhibits charging, so system does not
charge battery when AC is connected, but uses AC to supply itself
(so battery is idle)?

We already have this kind of features on embedded systems (which
often provide all kind of charger details). Those drivers solve
this by having a writable 'status' property in the charger device:

What:           /sys/class/power_supply/<supply_name>/status
Date:           May 2007
Contact:        linux-pm@vger.kernel.org
Description:
                Represents the charging status of the battery. Normally this
                is read-only reporting although for some supplies this can be
                used to enable/disable charging to the battery.
 
                Access: Read, Write
 
                Valid values:
                              "Unknown", "Charging", "Discharging",
                              "Not charging", "Full"

If I do not miss anything writing "Discharging" is the same as forced
discharge and "Not Charging" (AKA Idle) is the same as your inhibit feature.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Thomas Koch]

Hi,

 >> "inhibit_**discharge**"

 >> "stops **charging** of the battery"

 >> I'm wondering if it should be "inhibit_charge" or something like that?
 > Text and file name also seem to have reverse meaning for me. I

 > assume the text is the correct one, since it does not seem to

 > make sense inhibiting discharge. That would result in instant

 > poweroff on AC loss?


Fortunately that's only a typo in the docs file. The actual sysfs node
implemented by patch 2/3 is

	/sys/class/power_supply/BATx/inhibit_charge


--

Freundliche Grüße / Kind regards,

Thomas Koch



Mail : linrunner@gmx.net

Web  : https://linrunner.de/tlp


On 08.04.21 15:51, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Apr 07, 2021 at 10:33:41AM +0000, Barnabás Pőcze wrote:
>> 2021. április 7., szerda 12:24 keltezéssel, Hans de Goede írta:
>>> 2. If we add support for this to the kernel we should probably
>>> first agree on standardized power-supply class property names for
>>> these, rather then coming up with our own names. ATM we register
>>> 2 names for the charge start threshold, the one which the thinkpad_acpi
>>> code invented and the standardized name which was later added.
>>>
>>> I've added Sebastian, the power-supply class / driver maintainer to
>>> the Cc. for this. Sebastian Nicolo wants to add support for 2 new
>>> features as power-supply properties:
>>>
>>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> ...
>>> +Battery forced discharging
>>> +--------------------------
>>> +
>>> +sysfs attribute:
>>> +/sys/class/power_supply/BATx/force_discharge
>>> +
>>> +Setting this attribute to 1 forces the battery to discharge while AC is attached.
>>> +Setting it to 0 terminates forced discharging.
>>> +
>>> +Battery charge inhibiting
>>> +--------------------------
>>> +
>>> +sysfs attribute:
>>> +/sys/class/power_supply/BATx/inhibit_discharge
>>> +
>>> +Setting this attribute to 1 stops charging of the battery as a manual override
>>> +over the threshold attributes. Setting it to 0 terminates the override.
>>>
>>
>> "inhibit_**discharge**"
>> "stops **charging** of the battery"
>>
>> I'm wondering if it should be "inhibit_charge" or something like that?
>
> Text and file name also seem to have reverse meaning for me. I
> assume the text is the correct one, since it does not seem to
> make sense inhibiting discharge. That would result in instant
> poweroff on AC loss?
>
>>> Sebastian, I believe that this should be changes to instead be documented
>>> in: Documentation/ABI/testing/sysfs-class-power
>>> and besides the rename I was wondering if you have any remarks on the proposed
>>> API before Nicolo sends out a v2 ?
>
> IIUIC you have 'force_discharge', which basically means the system
> is running from battery power despite an AC adapter being connected
> and 'inhibit_discharge', which inhibits charging, so system does not
> charge battery when AC is connected, but uses AC to supply itself
> (so battery is idle)?
>
> We already have this kind of features on embedded systems (which
> often provide all kind of charger details). Those drivers solve
> this by having a writable 'status' property in the charger device:
>
> What:           /sys/class/power_supply/<supply_name>/status
> Date:           May 2007
> Contact:        linux-pm@vger.kernel.org
> Description:
>                  Represents the charging status of the battery. Normally this
>                  is read-only reporting although for some supplies this can be
>                  used to enable/disable charging to the battery.
>
>                  Access: Read, Write
>
>                  Valid values:
>                                "Unknown", "Charging", "Discharging",
>                                "Not charging", "Full"
>
> If I do not miss anything writing "Discharging" is the same as forced
> discharge and "Not Charging" (AKA Idle) is the same as your inhibit feature.
>
> -- Sebastian
>

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Thomas Koch]

Hi,

On 08.04.21 15:51, Sebastian Reichel wrote:

> IIUIC you have 'force_discharge', which basically means the system
> is running from battery power despite an AC adapter being connected
> and 'inhibit_discharge', which inhibits charging, so system does not
> charge battery when AC is connected, but uses AC to supply itself
> (so battery is idle)?
>
> We already have this kind of features on embedded systems (which
> often provide all kind of charger details). Those drivers solve
> this by having a writable 'status' property in the charger device:
>
> What:           /sys/class/power_supply/<supply_name>/status
> Date:           May 2007
> Contact:        linux-pm@vger.kernel.org
> Description:
>                  Represents the charging status of the battery. Normally this
>                  is read-only reporting although for some supplies this can be
>                  used to enable/disable charging to the battery.
>
>                  Access: Read, Write
>
>                  Valid values:
>                                "Unknown", "Charging", "Discharging",
>                                "Not charging", "Full"
>
> If I do not miss anything writing "Discharging" is the same as forced
> discharge and "Not Charging" (AKA Idle) is the same as your inhibit feature.

There are ThinkPads with two batteries (BAT0, BAT1) and the hardware
allows to select which one to discharge. An approach through
/sys/class/power_supply/AC/status won't cover this.

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

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

Re: [External] Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Mark Pearson]

On 07/04/2021 15:01, Ognjen Galić wrote:
> 
> On 07/04/2021 19:48, Mark Pearson wrote:
>> Hi Thomas, Hans and Nicolo
>>
>> On 07/04/2021 08:19, Thomas Koch wrote:
>>> Hi Hans,
>>>
>>>> 1. These features are useful, but not super useful and as such I wonder
>>>> how often they are used and this how well tested the firmware is wrt
>>>> these.
>>>> I have added Mark and Nitin from Lenovo to the Cc. Mark, Nitin, can you
>>>> comment on if it is ok from a firmware pov to try and use the following
>>>> battery related ACPI methods on all thinkpads? :
>>>> #define GET_DISCHARGE    "BDSG"
>>>> #define SET_DISCHARGE    "BDSS"
>>>> #define GET_INHIBIT    "PSSG"
>>>> #define SET_INHIBIT    "BICS"
>>> These ACPI methods are present in (with very few exceptions) all
>>> ThinkPads released since 2012. I am curious to hear what Mark and Nitin
>>> have to say, never read anything official about it.
>> I'm afraid I've not come across these myself before, but will go and ask
>> the firmware team.
>> <For my internal reference LO-1115>
> 
> I received the documents a few years back directly from a Chinese
> contact with all the methods
> and parameters described. I can mail the document to you privately if
> that's needed.
> 
>> It would be good to confirm the implementation details if I can. I found
>> recently that some of the temperature sensors that are read in by
>> thinkpad_acpi from the EC RAM are not temp sensors (patch coming
>> soon....hopefully later today). Hopefully I can check the internal spec
>> and give a thumbs up on the implementation - even if we're not allowed
>> to share the actual paperwork (maybe one day....)
>>
>>> Since 2012 there is also userspace tool tpacpi-bat [1] employing them
>>> along with those for the start/stop threshold.
>>>
>>> My own tool TLP makes use of tpacpi-bat for force_discharge also since
>>> 2012. From my experience in TLP support i can say there's a significant
>>> user base and those who use thresholds also want to use force_discharge
>>> for recalibration from time to time.
>> This probably isn't the right place for the discussion, but I've been
>> meaning to dig into battery management but never really get the time. I
>> know in the windows world that ThinkVantage has extra hooks for setting
>> thresholds and I wanted to see what we can do on the Linux side. If
>> there is anything that would be particularly helpful that is missing
>> please let me know.
>>
>> Thanks
>> Mark
>>
>>>
>>> The patches at hand work flawlessly on my small ThinkPad collection.
>>> [1] https://github.com/teleshoes/tpacpi-bat
>>>
Just as a follow-up - I got some more details on the four ACPI methods
from the firmware team and it all matches up with the code (and indeed
the implementation in tpcacpi-bat).

Mark

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Hans de Goede]

Hi,

On 4/9/21 8:33 PM, Thomas Koch wrote:
> Hi,
> 
> On 08.04.21 15:51, Sebastian Reichel wrote:
> 
>> IIUIC you have 'force_discharge', which basically means the system
>> is running from battery power despite an AC adapter being connected
>> and 'inhibit_discharge', which inhibits charging, so system does not
>> charge battery when AC is connected, but uses AC to supply itself
>> (so battery is idle)?
>>
>> We already have this kind of features on embedded systems (which
>> often provide all kind of charger details). Those drivers solve
>> this by having a writable 'status' property in the charger device:
>>
>> What:           /sys/class/power_supply/<supply_name>/status
>> Date:           May 2007
>> Contact:        linux-pm@vger.kernel.org
>> Description:
>>                  Represents the charging status of the battery. Normally this
>>                  is read-only reporting although for some supplies this can be
>>                  used to enable/disable charging to the battery.
>>
>>                  Access: Read, Write
>>
>>                  Valid values:
>>                                "Unknown", "Charging", "Discharging",
>>                                "Not charging", "Full"
>>
>> If I do not miss anything writing "Discharging" is the same as forced
>> discharge and "Not Charging" (AKA Idle) is the same as your inhibit feature.
> 
> There are ThinkPads with two batteries (BAT0, BAT1) and the hardware
> allows to select which one to discharge. An approach through
> /sys/class/power_supply/AC/status won't cover this.

The mentioned status strings come from /sys/class/power_supply/VAT#/status,
rather then from /sys/class/power_supply/AC/status.

There is one problem though, which is that the status attribute is being
managed by drivers/acpi/battery.c. There is infra for a driver like
the thinkpad_apci driver to add new attributes to a power_supply but
AFAIK there is no infra to say intercept writes to an attribute where
the reading is handled by another driver.

I guess we could add some special hook to allow another driver to
intercept status writes.

Sebastian, what is your take on this ?

Regards,

Hans

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Thomas Koch]

Hi Hans,

from a userspace perspective, I don't think it's optimal to combine the
two features and the status. For example, how do I find out which one is
available?

I have to test the writability of status and then still don't know which
one is available. Seeing if force_discharge or inhibit_charge are there
is much simpler.

And then enabling that: triggering force_discharge by writing
"Discharging" is ok. But for inhibit_charge we would need a new status,
something like "Charging inhibited". This then causes problems for the
existing userspace, says: upowerd could not handle it. You remember the
"Not charging" patch from Ognen?


--

Freundliche Grüße / Kind regards,

Thomas Koch



Mail : linrunner@gmx.net

Web  : https://linrunner.de/tlp


On 13.04.21 10:05, Hans de Goede wrote:
> Hi,
>
> On 4/9/21 8:33 PM, Thomas Koch wrote:
>> Hi,
>>
>> On 08.04.21 15:51, Sebastian Reichel wrote:
>>
>>> IIUIC you have 'force_discharge', which basically means the system
>>> is running from battery power despite an AC adapter being connected
>>> and 'inhibit_discharge', which inhibits charging, so system does not
>>> charge battery when AC is connected, but uses AC to supply itself
>>> (so battery is idle)?
>>>
>>> We already have this kind of features on embedded systems (which
>>> often provide all kind of charger details). Those drivers solve
>>> this by having a writable 'status' property in the charger device:
>>>
>>> What:           /sys/class/power_supply/<supply_name>/status
>>> Date:           May 2007
>>> Contact:        linux-pm@vger.kernel.org
>>> Description:
>>>                   Represents the charging status of the battery. Normally this
>>>                   is read-only reporting although for some supplies this can be
>>>                   used to enable/disable charging to the battery.
>>>
>>>                   Access: Read, Write
>>>
>>>                   Valid values:
>>>                                 "Unknown", "Charging", "Discharging",
>>>                                 "Not charging", "Full"
>>>
>>> If I do not miss anything writing "Discharging" is the same as forced
>>> discharge and "Not Charging" (AKA Idle) is the same as your inhibit feature.
>>
>> There are ThinkPads with two batteries (BAT0, BAT1) and the hardware
>> allows to select which one to discharge. An approach through
>> /sys/class/power_supply/AC/status won't cover this.
>
> The mentioned status strings come from /sys/class/power_supply/VAT#/status,
> rather then from /sys/class/power_supply/AC/status.
>
> There is one problem though, which is that the status attribute is being
> managed by drivers/acpi/battery.c. There is infra for a driver like
> the thinkpad_apci driver to add new attributes to a power_supply but
> AFAIK there is no infra to say intercept writes to an attribute where
> the reading is handled by another driver.
>
> I guess we could add some special hook to allow another driver to
> intercept status writes.
>
> Sebastian, what is your take on this ?
>
> Regards,
>
> Hans
>

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Hans de Goede]

Hi Thomas,

On 4/17/21 1:49 PM, Thomas Koch wrote:
> Hi Hans,
> 
> from a userspace perspective, I don't think it's optimal to combine the
> two features and the status. For example, how do I find out which one is
> available?
> 
> I have to test the writability of status and then still don't know which
> one is available. Seeing if force_discharge or inhibit_charge are there
> is much simpler.
> 
> And then enabling that: triggering force_discharge by writing
> "Discharging" is ok. But for inhibit_charge we would need a new status,
> something like "Charging inhibited". This then causes problems for the
> existing userspace, says: upowerd could not handle it. You remember the
> "Not charging" patch from Ognen?

I think you have valid points both wrt the discoverability of the
feature availability, as well as about a "Charging inhibited" status
value possibly (likely) causing problems for userspace.

With that said, you really need to discuss this with Sebastian. I'm fine
with the thinkpad_acpi patches, but as I already said I want to avoid
in essence defining new power_supply class userspace API inside
drivers/platform/x86 .  Last time we did that it ended poorly and more
in general it is a bad idea.

So you first need to agree on a set of power_supply class sysfs attributes
for this and document those in the power_supply class docs, before I can
merge the thinkpad_acpi patches.

And the agreeing on and documenting is something which you need to discuss
with Sebastian (the power_supply maintainer).

Regards,

Hans



> -- 
> 
> Freundliche Grüße / Kind regards,
> 
> Thomas Koch
> 
> 
> 
> Mail : linrunner@gmx.net
> 
> Web  : https://linrunner.de/tlp
> 
> 
> On 13.04.21 10:05, Hans de Goede wrote:
>> Hi,
>>
>> On 4/9/21 8:33 PM, Thomas Koch wrote:
>>> Hi,
>>>
>>> On 08.04.21 15:51, Sebastian Reichel wrote:
>>>
>>>> IIUIC you have 'force_discharge', which basically means the system
>>>> is running from battery power despite an AC adapter being connected
>>>> and 'inhibit_discharge', which inhibits charging, so system does not
>>>> charge battery when AC is connected, but uses AC to supply itself
>>>> (so battery is idle)?
>>>>
>>>> We already have this kind of features on embedded systems (which
>>>> often provide all kind of charger details). Those drivers solve
>>>> this by having a writable 'status' property in the charger device:
>>>>
>>>> What:           /sys/class/power_supply/<supply_name>/status
>>>> Date:           May 2007
>>>> Contact:        linux-pm@vger.kernel.org
>>>> Description:
>>>>                   Represents the charging status of the battery. Normally this
>>>>                   is read-only reporting although for some supplies this can be
>>>>                   used to enable/disable charging to the battery.
>>>>
>>>>                   Access: Read, Write
>>>>
>>>>                   Valid values:
>>>>                                 "Unknown", "Charging", "Discharging",
>>>>                                 "Not charging", "Full"
>>>>
>>>> If I do not miss anything writing "Discharging" is the same as forced
>>>> discharge and "Not Charging" (AKA Idle) is the same as your inhibit feature.
>>>
>>> There are ThinkPads with two batteries (BAT0, BAT1) and the hardware
>>> allows to select which one to discharge. An approach through
>>> /sys/class/power_supply/AC/status won't cover this.
>>
>> The mentioned status strings come from /sys/class/power_supply/VAT#/status,
>> rather then from /sys/class/power_supply/AC/status.
>>
>> There is one problem though, which is that the status attribute is being
>> managed by drivers/acpi/battery.c. There is infra for a driver like
>> the thinkpad_apci driver to add new attributes to a power_supply but
>> AFAIK there is no infra to say intercept writes to an attribute where
>> the reading is handled by another driver.
>>
>> I guess we could add some special hook to allow another driver to
>> intercept status writes.
>>
>> Sebastian, what is your take on this ?
>>
>> Regards,
>>
>> Hans
>>
> 

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Nicolo' Piazzalunga]

Hi Sebastian,

given the discussion below, would you agree on introducing attributes

force_discharge
inhibit_charge

rather than using 'status' property?
or how should we proceed?

Regards,
Nicolo'

On 4/17/21 7:03 PM, Hans de Goede wrote:
> Hi Thomas,
> 
> On 4/17/21 1:49 PM, Thomas Koch wrote:
>> Hi Hans,
>>
>> from a userspace perspective, I don't think it's optimal to combine the
>> two features and the status. For example, how do I find out which one is
>> available?
>>
>> I have to test the writability of status and then still don't know which
>> one is available. Seeing if force_discharge or inhibit_charge are there
>> is much simpler.
>>
>> And then enabling that: triggering force_discharge by writing
>> "Discharging" is ok. But for inhibit_charge we would need a new status,
>> something like "Charging inhibited". This then causes problems for the
>> existing userspace, says: upowerd could not handle it. You remember the
>> "Not charging" patch from Ognen?
> 
> I think you have valid points both wrt the discoverability of the
> feature availability, as well as about a "Charging inhibited" status
> value possibly (likely) causing problems for userspace.
> 
> With that said, you really need to discuss this with Sebastian. I'm fine
> with the thinkpad_acpi patches, but as I already said I want to avoid
> in essence defining new power_supply class userspace API inside
> drivers/platform/x86 .  Last time we did that it ended poorly and more
> in general it is a bad idea.
> 
> So you first need to agree on a set of power_supply class sysfs attributes
> for this and document those in the power_supply class docs, before I can
> merge the thinkpad_acpi patches.
> 
> And the agreeing on and documenting is something which you need to discuss
> with Sebastian (the power_supply maintainer).
> 
> Regards,
> 
> Hans
> 
> 
> 
>> -- 
>>
>> Freundliche Grüße / Kind regards,
>>
>> Thomas Koch
>>
>>
>>
>> Mail : linrunner@gmx.net
>>
>> Web  : https://linrunner.de/tlp
>>
>>
>> On 13.04.21 10:05, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 4/9/21 8:33 PM, Thomas Koch wrote:
>>>> Hi,
>>>>
>>>> On 08.04.21 15:51, Sebastian Reichel wrote:
>>>>
>>>>> IIUIC you have 'force_discharge', which basically means the system
>>>>> is running from battery power despite an AC adapter being connected
>>>>> and 'inhibit_discharge', which inhibits charging, so system does not
>>>>> charge battery when AC is connected, but uses AC to supply itself
>>>>> (so battery is idle)?
>>>>>
>>>>> We already have this kind of features on embedded systems (which
>>>>> often provide all kind of charger details). Those drivers solve
>>>>> this by having a writable 'status' property in the charger device:
>>>>>
>>>>> What:           /sys/class/power_supply/<supply_name>/status
>>>>> Date:           May 2007
>>>>> Contact:        linux-pm@vger.kernel.org
>>>>> Description:
>>>>>                   Represents the charging status of the battery. Normally this
>>>>>                   is read-only reporting although for some supplies this can be
>>>>>                   used to enable/disable charging to the battery.
>>>>>
>>>>>                   Access: Read, Write
>>>>>
>>>>>                   Valid values:
>>>>>                                 "Unknown", "Charging", "Discharging",
>>>>>                                 "Not charging", "Full"
>>>>>
>>>>> If I do not miss anything writing "Discharging" is the same as forced
>>>>> discharge and "Not Charging" (AKA Idle) is the same as your inhibit feature.
>>>>
>>>> There are ThinkPads with two batteries (BAT0, BAT1) and the hardware
>>>> allows to select which one to discharge. An approach through
>>>> /sys/class/power_supply/AC/status won't cover this.
>>>
>>> The mentioned status strings come from /sys/class/power_supply/VAT#/status,
>>> rather then from /sys/class/power_supply/AC/status.
>>>
>>> There is one problem though, which is that the status attribute is being
>>> managed by drivers/acpi/battery.c. There is infra for a driver like
>>> the thinkpad_apci driver to add new attributes to a power_supply but
>>> AFAIK there is no infra to say intercept writes to an attribute where
>>> the reading is handled by another driver.
>>>
>>> I guess we could add some special hook to allow another driver to
>>> intercept status writes.
>>>
>>> Sebastian, what is your take on this ?
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>
> 

Re: [External] Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Mark Pearson]

Hi Hans

On 2021-04-07 06:24, Hans de Goede wrote:
> Hi Nicola,
> 
> Thank you for your patch series.
> 
> I'm not sure what to do with these. I have a couple of concerns here:
> 
> 1. These features are useful, but not super useful and as such I wonder
> how often they are used and this how well tested the firmware is wrt these.
> I have added Mark and Nitin from Lenovo to the Cc. Mark, Nitin, can you
> comment on if it is ok from a firmware pov to try and use the following
> battery related ACPI methods on all thinkpads? :
> 
> #define GET_DISCHARGE	"BDSG"
> #define SET_DISCHARGE	"BDSS"
> #define GET_INHIBIT	"PSSG"
> #define SET_INHIBIT	"BICS"
> 
I followed up with the firmware team on this - and  was told these are 
intended to be internal only methods. They are not something the FW team 
expected to support or to be used by customers. From a Lenovo point of 
view these are not recommended and not supported APIs. Not sure where 
you got the API details from but they probably shouldn't have been 
disclosed :)

Obviously not our call if they get included or not, however they should 
come with a big flag that says "Don't use these unless you know what you 
are doing". Our recommendation is not to include them.

Do let me know if there are some important use cases for these so I can 
go back to the FW team and discuss supporting them properly.

Mark

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Nicolò Piazzalunga]

Hi,

On 9/27/21 3:59 PM, Mark Pearson wrote:
> Do let me know if there are some important use cases for these so I can go back to the FW team and discuss supporting them properly.

The important use cases are force discharge and inhibit charge.
These at present are dealt with using tpacpi-bat, which relies on (out of tree) acpi_call.
See also your previous reply.

On 4/12/21 7:10 PM, Mark Pearson wrote:
> Just as a follow-up - I got some more details on the four ACPI methods
> from the firmware team and it all matches up with the code (and indeed
> the implementation in tpcacpi-bat).

Best,
Nicolò

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Hans de Goede]

Hi Nicolò,

On 9/27/21 5:00 PM, Nicolò Piazzalunga wrote:
> Hi,
> 
> On 9/27/21 3:59 PM, Mark Pearson wrote:
>> Do let me know if there are some important use cases for these so I can go back to the FW team and discuss supporting them properly.
> 
> The important use cases are force discharge and inhibit charge.
> These at present are dealt with using tpacpi-bat, which relies on (out of tree) acpi_call.
> See also your previous reply.

I can see how those can be useful in certain circumstances.

I can also understand how Lenovo does not want these to be
available by default everywhere.

I think a good compromise would be to add a bool module option
which defaults to false to enable these.

Assuming Mark is ok with that, this is still blocked on agreeing
on standard power_supply class property names for these 2 features.

Can you perhaps write a (RFC) patch adding proposed standardized
attributes for this to:

Documentation/ABI/testing/sysfs-class-power

As well as to the enum power_supply_property {}
enum in: include/linux/power_supply.h

And to the power_supply_attrs[] array in
drivers/power/supply/power_supply_sysfs.c

?

And then send that the Sebastian Reichel with the linux-pm
and platform-driver-x86 lists in the Cc? 

Regards,

Hans

Re: [External] Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Mark Pearson]

On 2021-09-27 11:12, Hans de Goede wrote:
> Hi Nicolò,
> 
> On 9/27/21 5:00 PM, Nicolò Piazzalunga wrote:
>> Hi,
>>
>> On 9/27/21 3:59 PM, Mark Pearson wrote:
>>> Do let me know if there are some important use cases for these so I can go back to the FW team and discuss supporting them properly.
>>
>> The important use cases are force discharge and inhibit charge.
>> These at present are dealt with using tpacpi-bat, which relies on (out of tree) acpi_call.
>> See also your previous reply.
> 
> I can see how those can be useful in certain circumstances.
> 
> I can also understand how Lenovo does not want these to be
> available by default everywhere.
> 
> I think a good compromise would be to add a bool module option
> which defaults to false to enable these.
> 
> Assuming Mark is ok with that, this is still blocked on agreeing
> on standard power_supply class property names for these 2 features.
> 
I'm OK with it. Thanks :)

Mark

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Thomas Koch]

Hi Hans,

On 27.09.21 17:12, Hans de Goede wrote:
> Hi Nicolò,
>
> On 9/27/21 5:00 PM, Nicolò Piazzalunga wrote:
>> Hi,
>>
>> On 9/27/21 3:59 PM, Mark Pearson wrote:
>>> Do let me know if there are some important use cases for these so I can go back to the FW team and discuss supporting them properly.
>>
>> The important use cases are force discharge and inhibit charge.
>> These at present are dealt with using tpacpi-bat, which relies on (out of tree) acpi_call.
>> See also your previous reply.
>
> I can see how those can be useful in certain circumstances.
>
> I can also understand how Lenovo does not want these to be
> available by default everywhere.
>
> I think a good compromise would be to add a bool module option
> which defaults to false to enable these.

 From the user perspective, I don't agree that this is a good
compromise. Users simply want to recalibrate their battery. Having to
set the module option beforehand is an unnecessary hurdle imho.

Of course a module option again leads to support overhead in "user
space". Then tlp-stat -b would have to inform the user that
force_discharge is unfortunately not available, but he should try to set
the module option. What would that be good for?

While I respect Mark's official opinion, I would like to counter with my
experience from 10 years of TLP development and support:

The calls for force_discharge work unmodified since the 2012 ThinkPads
(T420/X220) on all models that also support charge thresholds.

They also work reliably, otherwise the issue tracker at tpacpi-bat and
TLP would be full of user issues.

inhibit_charge is probably used rather rarely, at least no TLP user has
asked for it.

@Mark: what is Lenovo's position on the calls for charge thresholds
already included in thinkpad_acpi? Are they also internal?

>
> Assuming Mark is ok with that, this is still blocked on agreeing
> on standard power_supply class property names for these 2 features.
>
> Can you perhaps write a (RFC) patch adding proposed standardized
> attributes for this to:
>
> Documentation/ABI/testing/sysfs-class-power
>
> As well as to the enum power_supply_property {}
> enum in: include/linux/power_supply.h
>
> And to the power_supply_attrs[] array in
> drivers/power/supply/power_supply_sysfs.c
>
> ?
>
> And then send that the Sebastian Reichel with the linux-pm
> and platform-driver-x86 lists in the Cc?
>
> Regards,
>
> Hans
>

--

Freundliche Grüße / Kind regards,

Thomas Koch



Mail : linrunner@gmx.net

Web  : https://linrunner.de/tlp

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Hans de Goede]

Hi,

On 9/29/21 7:47 AM, Thomas Koch wrote:
> Hi Hans,
> 
> On 27.09.21 17:12, Hans de Goede wrote:
>> Hi Nicolò,
>>
>> On 9/27/21 5:00 PM, Nicolò Piazzalunga wrote:
>>> Hi,
>>>
>>> On 9/27/21 3:59 PM, Mark Pearson wrote:
>>>> Do let me know if there are some important use cases for these so I can go back to the FW team and discuss supporting them properly.
>>>
>>> The important use cases are force discharge and inhibit charge.
>>> These at present are dealt with using tpacpi-bat, which relies on (out of tree) acpi_call.
>>> See also your previous reply.
>>
>> I can see how those can be useful in certain circumstances.
>>
>> I can also understand how Lenovo does not want these to be
>> available by default everywhere.
>>
>> I think a good compromise would be to add a bool module option
>> which defaults to false to enable these.
> 
> From the user perspective, I don't agree that this is a good
> compromise. Users simply want to recalibrate their battery.

But can't they already do this by just unplugging the AC and
then let the machine die ?

How is this different, does it somehow magically flicks on the
AC just before the battery deep-discharge protection kicks in
so no unsaved work is lost ?

I have never used this, but that seems unlikely.

Or is the advantage of this that the AC being present results in
no nagging low battery warnings as well as no auto-shutdown
near the end ?

If the advantage of this is indeed the no nagging + auto-shutdown
a better fix would be to do some userspace work to allow a
full, undisturbed, discharge while not on AC for calibration
purposes. That would also work on any vendor laptop, which would
be a big added bonus.

I do believe that Lenovo does want to support doing a calibration
cycle under Linux. This whole thread got resurfaced / revived by
Mark because of discussions with Lenovo about offering better
battery management options under Linux.

> Having to
> set the module option beforehand is an unnecessary hurdle imho.
> 
> Of course a module option again leads to support overhead in "user
> space". Then tlp-stat -b would have to inform the user that
> force_discharge is unfortunately not available, but he should try to set
> the module option. What would that be good for?
> 
> While I respect Mark's official opinion, I would like to counter with my
> experience from 10 years of TLP development and support:
> 
> The calls for force_discharge work unmodified since the 2012 ThinkPads
> (T420/X220) on all models that also support charge thresholds.
> 
> They also work reliably, otherwise the issue tracker at tpacpi-bat and
> TLP would be full of user issues.
> 
> inhibit_charge is probably used rather rarely, at least no TLP user has
> asked for it.

Ok, so I think we should look at the end goal here, which is doing
a battery calibration run, which AFAIK means discharging until
the machine turns off (by itself rather then an emergency shutdown
by the OS), followed by charging to 100%/full in one go.

So as mentioned before if we can disable the emergency shutdown
then we should be able to do this on all vendor laptops.

But I guess the real question here is, what do the Lenovo tools
under Windows do?

Mark, I assume that the Lenovo battery management tool under Windows
does allow doing a calibration, can you figure out how this works?

Maybe that does actually use force_disharge and things got mixed up
when you asked the firmware team about all 3 features in one email,
and the other 2 are only for testing purposes?

> @Mark: what is Lenovo's position on the calls for charge thresholds
> already included in thinkpad_acpi? Are they also internal?

I'm not Mark, but I believe that these are officially supported,
as mentioned before Lenovo does want to offer better battery management
options under Linux.

Regards,

Hans

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Thomas Koch]

Hi Hans,


On 29.09.21 11:55, Hans de Goede wrote:

 > Hi,

 >

 > On 9/29/21 7:47 AM, Thomas Koch wrote:

 >> Hi Hans,

 >>

 >> On 27.09.21 17:12, Hans de Goede wrote:

 >>> Hi Nicolò,

 >>>

 >>> On 9/27/21 5:00 PM, Nicolò Piazzalunga wrote:

 >>>> Hi,

 >>>>

 >>>> On 9/27/21 3:59 PM, Mark Pearson wrote:

 >>>>> Do let me know if there are some important use cases for these so
I can go back to the FW team and discuss supporting them properly.

 >>>>

 >>>> The important use cases are force discharge and inhibit charge.

 >>>> These at present are dealt with using tpacpi-bat, which relies on
(out of tree) acpi_call.

 >>>> See also your previous reply.

 >>>

 >>> I can see how those can be useful in certain circumstances.

 >>>

 >>> I can also understand how Lenovo does not want these to be

 >>> available by default everywhere.

 >>>

 >>> I think a good compromise would be to add a bool module option

 >>> which defaults to false to enable these.

 >>

 >>  From the user perspective, I don't agree that this is a good

 >> compromise. Users simply want to recalibrate their battery.

 >

 > But can't they already do this by just unplugging the AC and

 > then let the machine die ?Of course, but who wants that?

 > How is this different, does it somehow magically flicks on the

 > AC just before the battery deep-discharge protection kicks in

 > so no unsaved work is lost ?

But this is exactly how it works. There is no risk of data loss.



The EC resets the force_discharge flag at the end of the discharge and
the ThinkPad is seamlessly switched back to AC power. Just magical. No
power interruptions.



Maybe now you understand why Linux users want this so badly?


 > I have never used this, but that seems unlikelyYet it works.

 > Or is the advantage of this that the AC being present results in

 > no nagging low battery warnings as well as no auto-shutdown

 > near the end ?

 >

 > If the advantage of this is indeed the no nagging + auto-shutdown

 > a better fix would be to do some userspace work to allow a

 > full, undisturbed, discharge while not on AC for calibration

 > purposes. That would also work on any vendor laptop, which would

 > be a big added bonus.

 >

 > I do believe that Lenovo does want to support doing a calibration

 > cycle under Linux. This whole thread got resurfaced / revived by

 > Mark because of discussions with Lenovo about offering better

 > battery management options under Linux.>> Having to

 >> set the module option beforehand is an unnecessary hurdle imho.

 >>

 >> Of course a module option again leads to support overhead in "user

 >> space". Then tlp-stat -b would have to inform the user that

 >> force_discharge is unfortunately not available, but he should try to set

 >> the module option. What would that be good for?

 >>

 >> While I respect Mark's official opinion, I would like to counter with my

 >> experience from 10 years of TLP development and support:

 >>

 >> The calls for force_discharge work unmodified since the 2012 ThinkPads

 >> (T420/X220) on all models that also support charge thresholds.

 >>

 >> They also work reliably, otherwise the issue tracker at tpacpi-bat and

 >> TLP would be full of user issues.

 >>

 >> inhibit_charge is probably used rather rarely, at least no TLP user has

 >> asked for it.

 >

 > Ok, so I think we should look at the end goal here, which is doing

 > a battery calibration run, which AFAIK means discharging until

 > the machine turns off (by itself rather then an emergency shutdown

 > by the OS), followed by charging to 100%/full in one go.


Fine. There you have it, the patch is the working kernel solution for
your end goal and even more.



'tlp recalibrate' already does the user space job

handling



/sys/class/power_supply/BATx/force_discharge = 1 (TLP 1.4)



and includes setting



/sys/class/power_supply/AC/charge_control_end_threshold = 100



for a full recharge.



Mark may initiate GUI support if he so desires.

 > So as mentioned before if we can disable the emergency shutdown

 > then we should be able to do this on all vendor laptops.

 >

 > But I guess the real question here is, what do the Lenovo tools

 > under Windows do?

 >

 > Mark, I assume that the Lenovo battery management tool under Windows

 > does allow doing a calibration, can you figure out how this works?

 >

 > Maybe that does actually use force_disharge and things got mixed up

 > when you asked the firmware team about all 3 features in one email,

 > and the other 2 are only for testing purposes?

 >

 >> @Mark: what is Lenovo's position on the calls for charge thresholds

 >> already included in thinkpad_acpi? Are they also internal?

 >

 > I'm not Mark, but I believe that these are officially supported,

 > as mentioned before Lenovo does want to offer better battery management

 > options under Linux.

 >

 > Regards,

 >

 > Hans

 >


Freundliche Grüße / Kind regards,

Thomas Koch



Mail : linrunner@gmx.net

Web  : https://linrunner.de/tlp

Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Hans de Goede]

Hi,

On 9/29/21 12:45 PM, Thomas Koch wrote:
> Hi Hans,
> 
> 
> On 29.09.21 11:55, Hans de Goede wrote:
> 
>> Hi,
> 
>>
> 
>> On 9/29/21 7:47 AM, Thomas Koch wrote:
> 
>>> Hi Hans,
> 
>>>
> 
>>> On 27.09.21 17:12, Hans de Goede wrote:
> 
>>>> Hi Nicolò,
> 
>>>>
> 
>>>> On 9/27/21 5:00 PM, Nicolò Piazzalunga wrote:
> 
>>>>> Hi,
> 
>>>>>
> 
>>>>> On 9/27/21 3:59 PM, Mark Pearson wrote:
> 
>>>>>> Do let me know if there are some important use cases for these so
> I can go back to the FW team and discuss supporting them properly.
> 
>>>>>
> 
>>>>> The important use cases are force discharge and inhibit charge.
> 
>>>>> These at present are dealt with using tpacpi-bat, which relies on
> (out of tree) acpi_call.
> 
>>>>> See also your previous reply.
> 
>>>>
> 
>>>> I can see how those can be useful in certain circumstances.
> 
>>>>
> 
>>>> I can also understand how Lenovo does not want these to be
> 
>>>> available by default everywhere.
> 
>>>>
> 
>>>> I think a good compromise would be to add a bool module option
> 
>>>> which defaults to false to enable these.
> 
>>>
> 
>>>  From the user perspective, I don't agree that this is a good
> 
>>> compromise. Users simply want to recalibrate their battery.
> 
>>
> 
>> But can't they already do this by just unplugging the AC and
> 
>> then let the machine die ?Of course, but who wants that?
> 
>> How is this different, does it somehow magically flicks on the
> 
>> AC just before the battery deep-discharge protection kicks in
> 
>> so no unsaved work is lost ?
> 
> But this is exactly how it works. There is no risk of data loss.
> 
> 
> 
> The EC resets the force_discharge flag at the end of the discharge and
> the ThinkPad is seamlessly switched back to AC power. Just magical. No
> power interruptions.

Interesting. So what I get from this is that we really only want
force_discharge support. At least that is the major one. So maybe
we should focus on just that one.

Anyways lets wait and see what Mark has to say about this. As
I mentioned before doing battery calibration certainly is something
which is desirable to do under Windows too, so perhaps this option
is actually fine and it was the other 2 which are more
"testing only" firmware features.

Regards,

Hans

Re: [External] Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Mark Pearson]

On 2021-09-29 06:56, Hans de Goede wrote:
> Hi,
> 
> On 9/29/21 12:45 PM, Thomas Koch wrote:
>> Hi Hans,
>>
>>
>> On 29.09.21 11:55, Hans de Goede wrote:
>>
>>> Hi,
>>
>>>
>>
>>> On 9/29/21 7:47 AM, Thomas Koch wrote:
>>
>>>> Hi Hans,
>>
>>>>
>>
>>>> On 27.09.21 17:12, Hans de Goede wrote:
>>
>>>>> Hi Nicolò,
>>
>>>>>
>>
>>>>> On 9/27/21 5:00 PM, Nicolò Piazzalunga wrote:
>>
>>>>>> Hi,
>>
>>>>>>
>>
>>>>>> On 9/27/21 3:59 PM, Mark Pearson wrote:
>>
>>>>>>> Do let me know if there are some important use cases for these so
>> I can go back to the FW team and discuss supporting them properly.
>>
>>>>>>
>>
>>>>>> The important use cases are force discharge and inhibit charge.
>>
>>>>>> These at present are dealt with using tpacpi-bat, which relies on
>> (out of tree) acpi_call.
>>
>>>>>> See also your previous reply.
>>
>>>>>
>>
>>>>> I can see how those can be useful in certain circumstances.
>>
>>>>>
>>
>>>>> I can also understand how Lenovo does not want these to be
>>
>>>>> available by default everywhere.
>>
>>>>>
>>
>>>>> I think a good compromise would be to add a bool module option
>>
>>>>> which defaults to false to enable these.
>>
>>>>
>>
>>>>    From the user perspective, I don't agree that this is a good
>>
>>>> compromise. Users simply want to recalibrate their battery.
>>
>>>
>>
>>> But can't they already do this by just unplugging the AC and
>>
>>> then let the machine die ?Of course, but who wants that?
>>
>>> How is this different, does it somehow magically flicks on the
>>
>>> AC just before the battery deep-discharge protection kicks in
>>
>>> so no unsaved work is lost ?
>>
>> But this is exactly how it works. There is no risk of data loss.
>>
>>
>>
>> The EC resets the force_discharge flag at the end of the discharge and
>> the ThinkPad is seamlessly switched back to AC power. Just magical. No
>> power interruptions.
> 
> Interesting. So what I get from this is that we really only want
> force_discharge support. At least that is the major one. So maybe
> we should focus on just that one.
> 
> Anyways lets wait and see what Mark has to say about this. As
> I mentioned before doing battery calibration certainly is something
> which is desirable to do under Windows too, so perhaps this option
> is actually fine and it was the other 2 which are more
> "testing only" firmware features.
> 
> Regards,
> 
> Hans
> 
Hi,

I don't personally have any insight - I'm going to have to get feedback 
from the FW team on this. This is all new to me.

Thomas - thanks for the insight into how it is used. That will be really 
helpful to explain what we're looking for. As Hans mentioned we have an 
exercise to try and improve some of the battery related pieces for our 
platforms (largely driven by the TCO 9.0 requirements where I'm happy 
that the product teams are thinking of Linux before the fact rather than 
after) Battery calibration wasn't one of the things on my list, but it 
sounds interesting and useful. I don't think we have it in Windows - but 
I'm guessing and will confirm.
I also don't have any insight into how/why this was implemented on the 
older Thinkpads I'm afraid.

There was a follow up comment made from the FW team that there may be 
conditions where setting this may conflict with what the EC firmware is 
doing, so it will fail. It sounded like a corner-case to me but I don't 
have the details so I'll need to understand any concerns and go from there

Mark