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

[Hans de Goede <hdegoede@redhat.com>]

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