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 <li...@weissschuh.net> > > --- > > This patch is based on > https://lore.kernel.org/platform-driver-x86/d2808930-5840-6ffb-3a59-d235cdb1f...@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: > _______________________________________________ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
