| Message ID | 20211113104225.141333-5-linux@weissschuh.net (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | power: supply: add charge_behaviour property (force-discharge, inhibit-charge) | expand |
Hi Thomas, Thank you for working on this! On 11/13/21 11: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@weissschuh.net> > > --- > > This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/ > --- > 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)) { The use of else-if here seems wrong, this suggests that batterys can never support both force-discharge and inhibit-charge behavior, which they can, so this means that active can now never get set to BEHAVIOUR_INHIBIT_CHARGE on batteries which support both. So AFAICT the else part of the else if should be dropped here, making this a new stand alone if block. For the other parts of the set lets wait and see what Sebastian has to say. Regards, Hans > + 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: >
Hi Hans, On 2021-11-16 11:58+0100, Hans de Goede wrote: > Thank you for working on this! Thanks for the review! > On 11/13/21 11:42, Thomas Weißschuh wrote: > > @@ -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)) { > > The use of else-if here seems wrong, this suggests that batterys can never > support both force-discharge and inhibit-charge behavior, which they can, so this > means that active can now never get set to BEHAVIOUR_INHIBIT_CHARGE on > batteries which support both. > > So AFAICT the else part of the else if should be dropped here, making this > a new stand alone if block. Indeed, I'll fix this logic for v2. Thanks, Thomas
Hi Thomas, 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@weissschuh.net> > > --- > > This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/>> --- > 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) 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. Hope that helps Mark
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@weissschuh.net> > > > > --- > > > > This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/>> --- > > 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@gmx.net/
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@weissschuh.net> >>> >>> --- >>> >>> This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/ --- >>> 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@gmx.net/>> 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
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:
This adds support for the inhibit-charge charge_behaviour through the embedded controller of ThinkPads. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- This patch is based on https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1fe16@gmail.com/ --- drivers/platform/x86/thinkpad_acpi.c | 55 +++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 2 deletions(-)