power: supply: add charge_behaviour property (force-discharge, inhibit-charge)
Subsystems (2)
cover power: supply: add charge_behaviour property (force-discharge, inhibit-charge)
Hi,
this series adds support for the charge_behaviour property to the power subsystem and thinkpad_acpi driver.
As thinkpad_acpi has to use the 'struct power_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of proper power_supply properties to implement this property.
Patch 1: Adds the power_supply documentation and basic public API Patch 2: Adds helpers to power_supply core to help drivers implement the
charge_behaviour attribute
Patch 3: Adds support for force-discharge to thinkpad_acpi. Patch 4: Adds support for inhibit-discharge to thinkpad_acpi.
Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)
Ognjen Galic:
Your S-o-b is on the original inhibit_charge and force_discharge patches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?
Sebastian Reichel:
Currently the series does not actually support the property as a proper powersupply property handled fully by power_supply_sysfs.c because there would be no user for this property.
Previous discussions about the API:
https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/
v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:
* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers in charge_behaviour_store()
Open points:
Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.
Thomas Weißschuh (4):
power: supply: add charge_behaviour attributes
power: supply: add helpers for charge_behaviour sysfs platform/x86: thinkpad_acpi: support force-discharge platform/x86: thinkpad_acpi: support inhibit-charge
Documentation/ABI/testing/sysfs-class-power | 14 ++ drivers/platform/x86/thinkpad_acpi.c | 191 +++++++++++++++++++- drivers/power/supply/power_supply_sysfs.c | 51 ++++++ include/linux/power_supply.h | 16 ++ 4 files changed, 268 insertions(+), 4 deletions(-)
base-commit: 66f4beaa6c1d28161f534471484b2daa2de1dce0
Signature · 1 line
2.34.0
PATCH v2 1/4 power: supply: add charge_behaviour attributes
This a revised version of
"[RFC] add standardized attributes for force_discharge and inhibit_charge" [0], incorporating discussion results.
The biggest change is the switch from two boolean attributes to a single enum attribute.
[0] https://lore.kernel.org/platform-driver-x86/[email protected]/
--- Documentation/ABI/testing/sysfs-class-power | 14 ++++++++++++++ include/linux/power_supply.h | 7 +++++++ 2 files changed, 21 insertions(+)
Documentation/ABI/testing/sysfs-class-power +14 -0
@@ -455,6 +455,20 @@ Description: "Unknown", "Charging", "Discharging", "Not charging", "Full" +What: /sys/class/power_supply/<supply_name>/charge_behaviour +Date: November 2021 +Contact: [email protected] +Description: + Represents the charging behaviour. + + Access: Read, Write + + Valid values: + ================ ==================================== + auto: Charge normally, respect thresholds + inhibit-charge: Do not charge while AC is attached + force-discharge: Force discharge while AC is attached + What: /sys/class/power_supply/<supply_name>/technology Date: May 2007 Contact: [email protected]
include/linux/power_supply.h +7 -0
@@ -132,6 +132,7 @@ enum power_supply_property {POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX ,POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD ,/* in percents! */ POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD ,/* in percents! */ +POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR ,POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT ,POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT , POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
@@ -202,6 +203,12 @@ enum power_supply_usb_type { POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID, /* Apple Charging Method */ }; +enum power_supply_charge_behaviour { +POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO =0 , +POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE , +POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE , +}; +enum power_supply_notifier_events {PSY_EVENT_PROP_CHANGED , };
Signature · 1 line
2.34.0
PATCH v2 2/4 power: supply: add helpers for charge_behaviour sysfs
In reply to Thomas WeißschuhThese helper functions can be used by drivers to implement their own sysfs-attributes.
This is useful for ACPI-drivers extending the default ACPI-battery with their own charge_behaviour attributes.
--- drivers/power/supply/power_supply_sysfs.c | 55 +++++++++++++++++++++++ include/linux/power_supply.h | 9 ++++ 2 files changed, 64 insertions(+)
drivers/power/supply/power_supply_sysfs.c +55 -0
@@ -133,6 +133,12 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = { [POWER_SUPPLY_SCOPE_DEVICE] = "Device", }; +static const char *const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT [] = { + [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO ] ="auto" , + [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE ] ="inhibit-charge" , + [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE ] ="force-discharge" , +}; +static struct power_supply_attr power_supply_attrs[] = {/* Properties of type `int' */ POWER_SUPPLY_ENUM_ATTR (STATUS ),
@@ -484,3 +490,52 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)return ret; } + +ssize_t power_supply_charge_behaviour_show (struct device *dev, +unsigned int available_behaviours, +enum power_supply_charge_behaviour current_behaviour, +char *buf) +{ +bool match = false, available, active; +ssize_t count =0 ; +int i; + +for (i =0 ; i <ARRAY_SIZE (POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT ); i++) { + available = available_behaviours &BIT (i); + active = i == current_behaviour; + +if (available && active) { + count +=sysfs_emit_at (buf, count,"[%s] " , +POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT [i]); + match = true; + }else if (available) { + count +=sysfs_emit_at (buf, count,"%s " , +POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT [i]); + } + } + +if (!match) { +dev_warn (dev,"driver reporting unsupported charge behaviour\n" ); +return -EINVAL ; + } + +if (count) + buf[count -1 ] ='\n' ; + +return count; +} +EXPORT_SYMBOL_GPL (power_supply_charge_behaviour_show); + +int power_supply_charge_behaviour_parse (unsigned int available_behaviours,const char *buf) +{ +int i =sysfs_match_string (POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT , buf); + +if (i <0 ) +return i; + +if (available_behaviours &BIT (i)) +return i; + +return -EINVAL ; +} +EXPORT_SYMBOL_GPL (power_supply_charge_behaviour_parse);
include/linux/power_supply.h +9 -0
@@ -546,4 +546,13 @@ static inlinevoid power_supply_remove_hwmon_sysfs (struct power_supply *psy) {}#endif +#ifdef CONFIG_SYSFS +ssize_t power_supply_charge_behaviour_show (struct device *dev, +unsigned int available_behaviours, +enum power_supply_charge_behaviour behaviour, +char *buf); + +int power_supply_charge_behaviour_parse (unsigned int available_behaviours,const char *buf); +#endif +#endif /* __LINUX_POWER_SUPPLY_H__ */
Signature · 1 line
2.34.0
PATCH v2 3/4 platform/x86: thinkpad_acpi: support force-discharge
In reply to Thomas WeißschuhThis adds support for the force-discharge charge_behaviour through the embedded controller of ThinkPads.
---
This patch is based on https://lore.kernel.org/platform-driver-x86/[email protected]/
--- drivers/platform/x86/thinkpad_acpi.c | 131 ++++++++++++++++++++++++++- 1 file changed, 127 insertions(+), 4 deletions(-)
drivers/platform/x86/thinkpad_acpi.c +127 -4
@@ -9319,6 +9319,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 ,
@@ -9335,6 +9337,7 @@ enum {/* This is used in the get/set helpers */ THRESHOLD_START ,THRESHOLD_STOP , + FORCE_DISCHARGE, }; struct tpacpi_battery_data {
@@ -9342,6 +9345,7 @@ struct tpacpi_battery_data {int start_support;int charge_stop;int stop_support; +unsigned int charge_behaviours; };struct tpacpi_battery_driver_data {
@@ -9399,6 +9403,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 ;
@@ -9427,12 +9437,49 @@ 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 discharge on %d" , battery); +return -ENODEV ; + } +return 0 ;default :pr_crit ("wrong parameter: %d" , what);return -EINVAL ; } } +static int tpacpi_battery_set_validate (int what,int battery,int value) +{ +int ret, v; + + ret =tpacpi_battery_set (what, battery, value); +if (ret <0 ) +return ret; + + ret =tpacpi_battery_get (what, battery, &v); +if (ret <0 ) +return ret; + +if (v == value) +return 0 ; + +msleep (500 ); + + ret =tpacpi_battery_get (what, battery, &v); +if (ret <0 ) +return ret; + +if (v == value) +return 0 ; + +return -EIO ; +} +static int tpacpi_battery_probe (int battery) {int ret =0 ;
@@ -9445,6 +9492,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)) {
@@ -9481,10 +9530,25 @@ static int tpacpi_battery_probe(int battery)return -ENODEV ; } } -pr_info ("battery %d registered (start %d, stop %d)" , - battery, - battery_info.batteries[battery].charge_start, - battery_info.batteries[battery].charge_stop); +if (acpi_has_method (hkey_handle,GET_DISCHARGE )) { +if (ACPI_FAILURE (tpacpi_battery_acpi_eval (GET_DISCHARGE , &ret, battery))) { +pr_err ("Error probing battery discharge; %d\n" , battery); +return -ENODEV ; + } +/* Support is marked in bit 8 */ +if (ret &BIT (8 )) + battery_info.batteries[battery].charge_behaviours |= +BIT (POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE ); + } + + battery_info.batteries[battery].charge_behaviours |= +BIT (POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO ); + +pr_info ("battery %d registered (start %d, stop %d, behaviours: 0x%x)\n" , + battery, + battery_info.batteries[battery].charge_start, + battery_info.batteries[battery].charge_stop, + battery_info.batteries[battery].charge_behaviours);return 0 ; }
@@ -9619,6 +9683,28 @@ static ssize_t charge_control_end_threshold_show(struct device *device,return tpacpi_battery_show (THRESHOLD_STOP , device, buf); } +static ssize_t charge_behaviour_show (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +enum power_supply_charge_behaviour active =POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO ; +struct power_supply *supply =to_power_supply (dev); +unsigned int available; +int ret, battery; + + battery =tpacpi_battery_get_id (supply->desc->name); + available = battery_info.batteries[battery].charge_behaviours; + +if (available &BIT (POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE )) { +if (tpacpi_battery_get (FORCE_DISCHARGE , battery, &ret)) +return -ENODEV ; +if (ret) + active =POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE ; + } + +return power_supply_charge_behaviour_show (dev, available, active, buf); +} +static ssize_t charge_control_start_threshold_store (struct device *dev,struct device_attribute *attr,const char *buf,size_t count)
@@ -9633,8 +9719,44 @@ static ssize_t charge_control_end_threshold_store(struct device *dev,return tpacpi_battery_store (THRESHOLD_STOP , dev, buf, count); } +static ssize_t charge_behaviour_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +struct power_supply *supply =to_power_supply (dev); +int selected, battery, ret =0 ; +unsigned int available; + + battery =tpacpi_battery_get_id (supply->desc->name); + available = battery_info.batteries[battery].charge_behaviours; + selected =power_supply_charge_behaviour_parse (available, buf); + +if (selected <0 ) +return selected; + +switch (selected) { +case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO : +if (available &BIT (POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE )) + ret =tpacpi_battery_set_validate (FORCE_DISCHARGE , battery,0 ); +if (ret <0 ) +return ret; +break ; +case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE : + ret =tpacpi_battery_set_validate (FORCE_DISCHARGE , battery,1 ); +if (ret <0 ) +return ret; +break ; +default : +dev_err (dev,"Unexpected charge behaviour: %d\n" , selected); +return -EINVAL ; + } + +return count; +} +static DEVICE_ATTR_RW (charge_control_start_threshold);static DEVICE_ATTR_RW (charge_control_end_threshold); +static DEVICE_ATTR_RW (charge_behaviour); static struct device_attribute dev_attr_charge_start_threshold = __ATTR( charge_start_threshold, 0644,
@@ -9653,6 +9775,7 @@ static struct attribute *tpacpi_battery_attrs[] = { &dev_attr_charge_control_end_threshold.attr, &dev_attr_charge_start_threshold.attr, &dev_attr_charge_stop_threshold.attr, + &dev_attr_charge_behaviour.attr, NULL, };
Signature · 1 line
2.34.0
PATCH v2 4/4 platform/x86: thinkpad_acpi: support inhibit-charge
In reply to Thomas WeißschuhThis adds support for the inhibit-charge charge_behaviour through the embedded controller of ThinkPads.
---
This patch is based on https://lore.kernel.org/platform-driver-x86/[email protected]/
--- drivers/platform/x86/thinkpad_acpi.c | 64 +++++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 2 deletions(-)
drivers/platform/x86/thinkpad_acpi.c +62 -2
@@ -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 "BICG" +#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,12 @@ 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 : + 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 +9456,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 ;
@@ -9494,6 +9519,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)) {
@@ -9540,6 +9567,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 );
@@ -9698,10 +9735,22 @@ static ssize_t charge_behaviour_show(struct device *dev,if (available &BIT (POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE )) {if (tpacpi_battery_get (FORCE_DISCHARGE , battery, &ret))return -ENODEV ; -if (ret) +if (ret) { active =POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE ; + goto out; + } + } + +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 ; + goto out; + } } +out:return power_supply_charge_behaviour_show (dev, available, active, buf); }
@@ -9738,11 +9787,22 @@ static ssize_t charge_behaviour_store(struct device *dev,case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO :if (available &BIT (POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE )) ret =tpacpi_battery_set_validate (FORCE_DISCHARGE , battery,0 ); +if (available &BIT (POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE )) + ret =min (ret,tpacpi_battery_set_validate (INHIBIT_CHARGE , battery,0 ));if (ret <0 )return ret;break ;case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE : - ret =tpacpi_battery_set_validate (FORCE_DISCHARGE , battery,1 ); +if (available &BIT (POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE )) + ret =tpacpi_battery_set_validate (INHIBIT_CHARGE , battery,0 ); + ret =min (ret,tpacpi_battery_set_validate (FORCE_DISCHARGE , battery,1 )); +if (ret <0 ) +return ret; +break ; +case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE : +if (available &BIT (POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE )) + ret =tpacpi_battery_set_validate (FORCE_DISCHARGE , battery,0 ); + ret =min (ret,tpacpi_battery_set_validate (INHIBIT_CHARGE , battery,1 ));if (ret <0 )return ret;break ;
Signature · 1 line
2.34.0
Quoted 2 lines · Thomas Weißschuh
this series adds support for the
charge_behaviourproperty to the power subsystem andthinkpad_acpidriver.
Wonderful! Thanks for working on this.
I can confirm inhibit-charge and force-discharge states work with patch v2 on v5.16-rc2 on a T430 (2342-CTO) with BIOS G1ETC2WW (2.82 ), EC G1HT36WW and a single battery. Most behavior is as expected:
- With force-discharge, status becomes "Discharging" and energy_now drops over time while AC remains connected.
- With inhibit-charge, status becomes "Unknown" and energy_now is stable over time, even when not fully charged.
- With auto, status becomes "Charging" and energy_now rises over time.
- charge_behaviour takes precedence over charge_control_{start,end}_threshold: status remains Discharging/Unknown when below the start threshold, either due to discharge or threshold change.
- charge_behaviour is preserved over soft reboot.
- inhibit-charge/auto are preserved across battery removal and reinsertion.
- inhibit-charge/auto are preserved across s2ram (S3).
- With force-discharge, if the battery is removed, the machine immediately powers off.
Some behavior is a little surprising:
- charge_behaviour can not be set to force-discharge if AC is disconnected (EIO). If charge_behaviour is force-discharge when AC is disconnected, it changes to auto, unlike inhibit-charge.
- charge_behavior force-discharge is not preserved across s2ram (S3), unlike inhibit-charge.
- charge_behaviour is not preserved across hard reset (unlike charge thresholds). Interestingly, it appears that inhibit-charge is preserved across power-off (no charging is observed while powered off) but not power-on, even though it is preserved across soft reboot, as noted above.
I assume the behavior is under the control of the EC, so these aren't criticisms of the patch. Just some observations.
Thanks again,
Kevin
Hi,
Quoted 23 lines · Thomas Weißschuh
This series adds support for the
charge_behaviourproperty to the power subsystem andthinkpad_acpidriver.As
thinkpad_acpihas to use the 'structpower_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of properpower_supplyproperties to implement this property.Patch 1: Adds the
power_supplydocumentation and basic public API Patch 2: Adds helpers topower_supplycore to help drivers implement the
charge_behaviourattribute
Patch 3: Adds support for force-discharge tothinkpad_acpi. Patch 4: Adds support for inhibit-discharge tothinkpad_acpi.Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)
Ognjen Galic:
Your S-o-b is on the original
inhibit_chargeandforce_dischargepatches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?
Quoted 5 lines
Sebastian Reichel:
Currently the series does not actually support the property as a proper powersupply property handled fully by
power_supply_sysfs.cbecause there would be no user for this property.
I'm not too happy how the acpi-battery hooks work, but that's not your fault and this patchset does not really make the situation worse. So:
-- Sebastian
Quoted 38 lines
Previous discussions about the API:
https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers incharge_behaviour_store()Open points:
Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.Thomas Weißschuh (4):
power: supply: addcharge_behaviourattributes
power: supply: add helpers forcharge_behavioursysfs platform/x86:thinkpad_acpi: support force-discharge platform/x86:thinkpad_acpi: support inhibit-chargeDocumentation/ABI/testing/sysfs-class-power | 14 ++ drivers/platform/x86/thinkpad_acpi.c | 191 +++++++++++++++++++- drivers/power/supply/power_supply_sysfs.c | 51 ++++++ include/linux/power_supply.h | 16 ++ 4 files changed, 268 insertions(+), 4 deletions(-)base-commit:
66f4beaa6c1d28161f534471484b2daa2de1dce0Signature · 1 line
2.34.0
Hi,
Quoted 32 lines · Sebastian Reichel
Hi,
Quoted 23 lines · Thomas Weißschuh
This series adds support for the
charge_behaviourproperty to the power subsystem andthinkpad_acpidriver.As
thinkpad_acpihas to use the 'structpower_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of properpower_supplyproperties to implement this property.Patch 1: Adds the
power_supplydocumentation and basic public API Patch 2: Adds helpers topower_supplycore to help drivers implement the
charge_behaviourattribute
Patch 3: Adds support for force-discharge tothinkpad_acpi. Patch 4: Adds support for inhibit-discharge tothinkpad_acpi.Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)
Ognjen Galic:
Your S-o-b is on the original
inhibit_chargeandforce_dischargepatches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?Quoted 5 lines
Sebastian Reichel:
Currently the series does not actually support the property as a proper powersupply property handled fully by
power_supply_sysfs.cbecause there would be no user for this property.
Quoted 5 lines
I'm not too happy how the acpi-battery hooks work, but that's not your fault and this patchset does not really make the situation worse. So:
I haven't looked at the thinkpad_apci.c bits closely yet (for this new version), but assuming those are ready for merging too, we need to discuss about how to merge this.
The thinkpad_acpi code has already seen quite a lot of changes in -next, so I would like the thinkpad_acpi changes to go upstream through the platform-drivers-x86.git tree to avoid conflicts.
As such I think it is best if you (Sebastian) can prepare an immutable branch with patch 1 + 2 for me to merge. Then even if patch 3 + 4 need more work, Thomas can just respin those on top of the immutable branch.
Alternatively I can take the entire series upstream through the platform-drivers-x86.git tree if that is ok with you (Sebastian).
Either way please let me know how you want to proceed with this.
Regards,
Hans
Quoted 38 lines
Quoted 38 lines
Previous discussions about the API:
https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers incharge_behaviour_store()Open points:
Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.Thomas Weißschuh (4):
power: supply: addcharge_behaviourattributes
power: supply: add helpers forcharge_behavioursysfs platform/x86:thinkpad_acpi: support force-discharge platform/x86:thinkpad_acpi: support inhibit-chargeDocumentation/ABI/testing/sysfs-class-power | 14 ++ drivers/platform/x86/thinkpad_acpi.c | 191 +++++++++++++++++++- drivers/power/supply/power_supply_sysfs.c | 51 ++++++ include/linux/power_supply.h | 16 ++ 4 files changed, 268 insertions(+), 4 deletions(-)base-commit:
66f4beaa6c1d28161f534471484b2daa2de1dce0Signature · 1 line
2.34.0
Hi,
Quoted 32 lines · Sebastian Reichel
Hi,
Quoted 23 lines · Thomas Weißschuh
This series adds support for the
charge_behaviourproperty to the power subsystem andthinkpad_acpidriver.As
thinkpad_acpihas to use the 'structpower_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of properpower_supplyproperties to implement this property.Patch 1: Adds the
power_supplydocumentation and basic public API Patch 2: Adds helpers topower_supplycore to help drivers implement the
charge_behaviourattribute
Patch 3: Adds support for force-discharge tothinkpad_acpi. Patch 4: Adds support for inhibit-discharge tothinkpad_acpi.Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)
Ognjen Galic:
Your S-o-b is on the original
inhibit_chargeandforce_dischargepatches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?Quoted 5 lines
Sebastian Reichel:
Currently the series does not actually support the property as a proper powersupply property handled fully by
power_supply_sysfs.cbecause there would be no user for this property.
Quoted 5 lines
I'm not too happy how the acpi-battery hooks work, but that's not your fault and this patchset does not really make the situation worse. So:
Sebastian, what is the plan for taking this upstream ? Does your ack mean that you are ok with me taking the entire series upstream through the pdx86 tree?
Or do you plan to apply patches 1-2 through linux-power-supply.git; and in that case can you provide an inmmutable branch with those patches for me to merge into pdx86/for-next so that I can then apply patches 3 + 4 there ?
Note merging everything through the linux-power-supply.git tree is non ideal in this case because the thinkpad_acpi.c code already has a lot of changes in pdx86/for-next.
Regards,
Hans
Quoted 38 lines
Quoted 38 lines
Previous discussions about the API:
https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers incharge_behaviour_store()Open points:
Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.Thomas Weißschuh (4):
power: supply: addcharge_behaviourattributes
power: supply: add helpers forcharge_behavioursysfs platform/x86:thinkpad_acpi: support force-discharge platform/x86:thinkpad_acpi: support inhibit-chargeDocumentation/ABI/testing/sysfs-class-power | 14 ++ drivers/platform/x86/thinkpad_acpi.c | 191 +++++++++++++++++++- drivers/power/supply/power_supply_sysfs.c | 51 ++++++ include/linux/power_supply.h | 16 ++ 4 files changed, 268 insertions(+), 4 deletions(-)base-commit:
66f4beaa6c1d28161f534471484b2daa2de1dce0Signature · 1 line
2.34.0
Hi,
Quoted 32 lines · Sebastian Reichel
Hi,
Quoted 23 lines · Thomas Weißschuh
This series adds support for the
charge_behaviourproperty to the power subsystem andthinkpad_acpidriver.As
thinkpad_acpihas to use the 'structpower_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of properpower_supplyproperties to implement this property.Patch 1: Adds the
power_supplydocumentation and basic public API Patch 2: Adds helpers topower_supplycore to help drivers implement the
charge_behaviourattribute
Patch 3: Adds support for force-discharge tothinkpad_acpi. Patch 4: Adds support for inhibit-discharge tothinkpad_acpi.Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)
Ognjen Galic:
Your S-o-b is on the original
inhibit_chargeandforce_dischargepatches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?Quoted 5 lines
Sebastian Reichel:
Currently the series does not actually support the property as a proper powersupply property handled fully by
power_supply_sysfs.cbecause there would be no user for this property.
Quoted 5 lines
I'm not too happy how the acpi-battery hooks work, but that's not your fault and this patchset does not really make the situation worse. So:
Sebastian, I have taken the liberty to assume that this means that you are ok with merging the entire series through the pdx86 tree (I've done a test-merge with linux-power-supply/for-next and there are no conflicts).
Thomas, Thank you for your patch-series, I've applied the series to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window.
Regards,
Hans
Quoted 41 lines
-- Sebastian
Quoted 38 lines
Previous discussions about the API:
https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers incharge_behaviour_store()Open points:
Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.Thomas Weißschuh (4):
power: supply: addcharge_behaviourattributes
power: supply: add helpers forcharge_behavioursysfs platform/x86:thinkpad_acpi: support force-discharge platform/x86:thinkpad_acpi: support inhibit-chargeDocumentation/ABI/testing/sysfs-class-power | 14 ++ drivers/platform/x86/thinkpad_acpi.c | 191 +++++++++++++++++++- drivers/power/supply/power_supply_sysfs.c | 51 ++++++ include/linux/power_supply.h | 16 ++ 4 files changed, 268 insertions(+), 4 deletions(-)base-commit:
66f4beaa6c1d28161f534471484b2daa2de1dce0Signature · 1 line
2.34.0
Hi Thomas,
Quoted 58 lines · Thomas Weißschuh
Hi,
this series adds support for the
charge_behaviourproperty to the power subsystem andthinkpad_acpidriver.As
thinkpad_acpihas to use the 'structpower_supply' created by the generic ACPI driver it has to rely on custom sysfs attributes instead of properpower_supplyproperties to implement this property.Patch 1: Adds the
power_supplydocumentation and basic public API Patch 2: Adds helpers topower_supplycore to help drivers implement the
charge_behaviourattribute
Patch 3: Adds support for force-discharge tothinkpad_acpi. Patch 4: Adds support for inhibit-discharge tothinkpad_acpi.Patch 3 and 4 are largely taken from other patches and adapted to the new API. (Links are in the patch trailer)
Ognjen Galic:
Your S-o-b is on the original
inhibit_chargeandforce_dischargepatches. I would like to add you as Co-developed-by but to do that it will also require your S-o-b. Could you give your sign-offs for the new patches, so you can be properly attributed?Sebastian Reichel:
Currently the series does not actually support the property as a proper powersupply property handled fully by
power_supply_sysfs.cbecause there would be no user for this property.Previous discussions about the API:
https://lore.kernel.org/platform-driver-x86/[email protected]/
https://lore.kernel.org/platform-driver-x86/[email protected]/v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2:* Use sysfs_emit-APIs instead of plain sprintf
* More cecks for actual feature availability
* Validation of the written values
* Read inhibit-charge via BICG instead of PSSG (peak shift state)
* Don't mangle error numbers incharge_behaviour_store()Open points:
Thomas Koch has observed that on a T450s with two batteries inhibit-charge on BAT0 will affect both batteries and for BAT1 it is ignored entirely, this seems to be a bug in the EC.
On my T460s with two batteries it works correctly.Thomas Weißschuh (4):
power: supply: addcharge_behaviourattributes
power: supply: add helpers forcharge_behavioursysfs platform/x86:thinkpad_acpi: support force-discharge platform/x86:thinkpad_acpi: support inhibit-charge
Quoted 9 lines
Documentation/ABI/testing/sysfs-class-power | 14 ++ drivers/platform/x86/thinkpad_acpi.c | 191 +++++++++++++++++++- drivers/power/supply/power_supply_sysfs.c | 51 ++++++ include/linux/power_supply.h | 16 ++ 4 files changed, 268 insertions(+), 4 deletions(-)base-commit:
66f4beaa6c1d28161f534471484b2daa2de1dce0
Reviewed-by
: Thomas Koch <[email protected]>
Works well on ThinkPad X220, T450s, X1C6 with the exception mentioned above.
The new API is included in TLP already [1].
[1]
https://github.com/linrunner/TLP/commit/f0bf18f847470ae495a68f9f0e30130b96348936
--
Freundliche Grüße / Kind regards,
Thomas Koch
Mail : [email protected]
Web : https://linrunner.de/tlp