diff mbox series

[1/3] thinkpad_acpi: add support for force_discharge

Message ID c2504700-06e9-e7d8-80f7-de90b0b6dfb5@gmail.com (mailing list archive)
State New
Headers show
Series [1/3] thinkpad_acpi: add support for force_discharge | expand

Commit Message

Nicolo' Piazzalunga March 17, 2021, 2:01 p.m. UTC
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(-)

Comments

Hans de Goede April 7, 2021, 10:24 a.m. UTC | #1
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,
>  };
>  
>
Barnabás Pőcze April 7, 2021, 10:33 a.m. UTC | #2
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
Thomas Koch April 7, 2021, 12:19 p.m. UTC | #3
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,
>>   };
>>
>>
>
Mark Pearson April 7, 2021, 5:48 p.m. UTC | #4
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,
>>>   };
>>>
>>>
>>
>
Sebastian Reichel April 8, 2021, 1:51 p.m. UTC | #5
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
Thomas Koch April 8, 2021, 6:18 p.m. UTC | #6
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
>
Thomas Koch April 9, 2021, 6:33 p.m. UTC | #7
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
Mark Pearson April 12, 2021, 5:10 p.m. UTC | #8
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
Hans de Goede April 13, 2021, 8:05 a.m. UTC | #9
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
Thomas Koch April 17, 2021, 11:49 a.m. UTC | #10
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
>
Hans de Goede April 17, 2021, 5:03 p.m. UTC | #11
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
>>
>
Nicolo' Piazzalunga May 19, 2021, 2:45 p.m. UTC | #12
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
>>>
>>
>
diff mbox series

Patch

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,
 };