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
--
2.34.0
This adds support for the force-discharge charge_behaviour through the
embedded controller of ThinkPads.
Co-developed-by: Thomas Koch <[email protected]>
Signed-off-by: Thomas Koch <[email protected]>
Co-developed-by: Nicolò Piazzalunga <[email protected]>
Signed-off-by: Nicolò Piazzalunga <[email protected]>
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 9c632df734bb..e3567cc686fa 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -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,
};
--
2.34.0
On Wed, 2021-11-24 at 00:27 +0100, Thomas Wei?schuh wrote:
> this series adds support for the charge_behaviour property to the power
> subsystem and thinkpad_acpi driver.
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.
Tested-by: Kevin Locke <[email protected]>
Thanks again,
Kevin
Hi,
On Wed, Nov 24, 2021 at 12:27:00AM +0100, Thomas Wei?schuh wrote:
> 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.
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:
Acked-by: Sebastian Reichel <[email protected]>
-- Sebastian
> 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
> --
> 2.34.0
>
Hi,
On 12/3/21 22:33, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Nov 24, 2021 at 12:27:00AM +0100, Thomas Weißschuh wrote:
>> 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.
>
> 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:
>
> Acked-by: Sebastian Reichel <[email protected]>
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
>> 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
>> --
>> 2.34.0
>>
Hi Thomas,
On 24.11.21 00:27, Thomas Weißschuh wrote:
> 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
>
Reviewed-by
: Thomas Koch <[email protected]>
Tested-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
Hi,
On 12/3/21 22:33, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Nov 24, 2021 at 12:27:00AM +0100, Thomas Weißschuh wrote:
>> 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.
>
> 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:
>
> Acked-by: Sebastian Reichel <[email protected]>
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
>> 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
>> --
>> 2.34.0
>>
Hi,
On 12/3/21 22:33, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Nov 24, 2021 at 12:27:00AM +0100, Thomas Weißschuh wrote:
>> 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.
>
> 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:
>
> Acked-by: Sebastian Reichel <[email protected]>
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
>
> -- Sebastian
>
>> 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
>> --
>> 2.34.0
>>