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