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

[Mark Pearson <markpearson@lenovo.com>]

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