Re: [PATCH 4/4] platform/x86: thinkpad_acpi: support inhibit-charge

[Hans de Goede <hdegoede@redhat.com>]

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:
>