Hi Thomas On 2021-11-16 18:44, Thomas Weißschuh wrote: > Hi Mark, > > On 2021-11-16 15:52-0500, Mark Pearson wrote: >> On 2021-11-13 05:42, Thomas Weißschuh wrote: >>> This adds support for the inhibit-charge charge_behaviour through the >>> embedded controller of ThinkPads. >>> >>> Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx> >>> >>> --- >>> >>> This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@xxxxxxxxx/ --- >>> drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++- >>> 1 file changed, 53 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >>> index e8c98e9aff71..7cd6475240b2 100644 >>> --- a/drivers/platform/x86/thinkpad_acpi.c >>> +++ b/drivers/platform/x86/thinkpad_acpi.c >>> @@ -9321,6 +9321,8 @@ static struct ibm_struct mute_led_driver_data = { >>> #define SET_STOP "BCSS" >>> #define GET_DISCHARGE "BDSG" >>> #define SET_DISCHARGE "BDSS" >>> +#define GET_INHIBIT "PSSG" >>> +#define SET_INHIBIT "BICS" >>> >>> enum { >>> BAT_ANY = 0, >>> @@ -9338,6 +9340,7 @@ enum { >>> THRESHOLD_START, >>> THRESHOLD_STOP, >>> FORCE_DISCHARGE, >>> + INHIBIT_CHARGE, >>> }; >>> >>> struct tpacpi_battery_data { >>> @@ -9409,6 +9412,13 @@ static int tpacpi_battery_get(int what, int battery, int *ret) >>> /* The force discharge status is in bit 0 */ >>> *ret = *ret & 0x01; >>> return 0; >>> + case INHIBIT_CHARGE: >>> + /* This is actually reading peak shift state, like tpacpi-bat does */ >>> + if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery)) >>> + return -ENODEV; >>> + /* The inhibit charge status is in bit 0 */ >>> + *ret = *ret & 0x01; >>> + return 0; >>> default: >>> pr_crit("wrong parameter: %d", what); >>> return -EINVAL; >>> @@ -9447,6 +9457,22 @@ static int tpacpi_battery_set(int what, int battery, int value) >>> return -ENODEV; >>> } >>> return 0; >>> + case INHIBIT_CHARGE: >>> + /* When setting inhibit charge, we set a default value of >>> + * always breaking on AC detach and the effective time is set to >>> + * be permanent. >>> + * The battery ID is in bits 4-5, 2 bits, >>> + * the effective time is in bits 8-23, 2 bytes. >>> + * A time of FFFF indicates forever. >>> + */ >>> + param = value; >>> + param |= battery << 4; >>> + param |= 0xFFFF << 8; >>> + if (ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_INHIBIT, &ret, param))) { >>> + pr_err("failed to set inhibit charge on %d", battery); >>> + return -ENODEV; >>> + } >>> + return 0; >>> default: >>> pr_crit("wrong parameter: %d", what); >>> return -EINVAL; >>> @@ -9467,6 +9493,8 @@ static int tpacpi_battery_probe(int battery) >>> * 4) Check for support >>> * 5) Get the current force discharge status >>> * 6) Check for support >>> + * 7) Get the current inhibit charge status >>> + * 8) Check for support >>> */ >>> if (acpi_has_method(hkey_handle, GET_START)) { >>> if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) { >>> @@ -9513,6 +9541,16 @@ static int tpacpi_battery_probe(int battery) >>> battery_info.batteries[battery].charge_behaviours |= >>> BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE); >>> } >>> + if (acpi_has_method(hkey_handle, GET_INHIBIT)) { >>> + if (ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, &ret, battery))) { >>> + pr_err("Error probing battery inhibit charge; %d\n", battery); >>> + return -ENODEV; >>> + } >>> + /* Support is marked in bit 5 */ >>> + if (ret & BIT(5)) >>> + battery_info.batteries[battery].charge_behaviours |= >>> + BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE); >>> + } >>> >>> battery_info.batteries[battery].charge_behaviours |= >>> BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO); >>> @@ -9673,6 +9711,11 @@ static ssize_t charge_behaviour_show(struct device *dev, >>> return -ENODEV; >>> if (ret) >>> active = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE; >>> + } else if (available & BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)) { >>> + if (tpacpi_battery_get(INHIBIT_CHARGE, battery, &ret)) >>> + return -ENODEV; >>> + if (ret) >>> + active = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE; >>> } >>> >>> return power_supply_charge_behaviour_show(dev, available, active, buf); >>> @@ -9710,12 +9753,20 @@ static ssize_t charge_behaviour_store(struct device *dev, >>> switch (selected) { >>> case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO: >>> ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0); >>> - if (ret < 0) >>> + ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret; >>> + if (ret) >>> return ret; >>> break; >>> case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE: >>> ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 1); >>> - if (ret < 0) >>> + ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 0) || ret; >>> + if (ret) >>> + return ret; >>> + break; >>> + case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE: >>> + ret = tpacpi_battery_set(FORCE_DISCHARGE, battery, 0); >>> + ret = tpacpi_battery_set(INHIBIT_CHARGE, battery, 1) || ret; >>> + if (ret) >>> return ret; >>> break; >>> default: >>> >> >> I can confirm the bit fields are correct for these calls (as for the >> previous patch) > > Thanks! > > Could you doublecheck the behavior for multiple batteries to maybe find a > reason why BAT1 is not inhibited as reported by Thomas Koch [0]? > >> Couple of things to note, based on feedback previously from the FW team >> that I found when digging thru my battery related emails. >> >> "Lenovo doesn't officially support the use of these calls - they're >> intended for internal use" (at this point in time - there is some >> discussion about battery recalibration support but I don't have details >> I can share there yet). >> >> The FW team also noted that: >> >> "Actual battery charging/discharging behaviors are managed by ECFW. So >> the request of BDSS/BICS method may be rejected by ECFW for the current >> battery mode/status. You have to check if the request of the method is >> actually applied or not" >> >> So for the calls above (and for the BDSS calls in the previous patch) it >> would be good to do a read back of the setting to ensure it has >> completed successfully. > > I'll add that for v2. > >> Hope that helps > > It does, thanks! > > Thomas > > [0] https://lore.kernel.org/platform-driver-x86/9cebba85-f399-a7aa-91f7-237852338dc5@xxxxxxx/>> I got confirmation that: <quote> BDSS have to be used with specific battery. Please use with Primary(=1b) or Secondary(2b) as Battery ID (Bit9-8) Bit 9-8: Battery ID = 0: Any battery = 1: Primary battery = 2: Secondary battery </quote> It seems you can't use BDSS with all batteries. I'll confirm on BICS what the limitations are, they didn't update on that piece yet I'm afraid. Unfortunately I don't think I have any systems with two batteries to test myself. Mark
