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