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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx

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@xxxxxxxxx>
Signed-off-by: Thomas Koch <linrunner@xxxxxxx>
Signed-off-by: Nicolo' Piazzalunga <nicolopiazzalunga@xxxxxxxxx>
---
  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,
  };








[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux