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

[Thomas Koch <linrunner@gmx.net>]

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