| Message ID | 20211113104225.141333-1-linux@weissschuh.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | power: supply: add charge_behaviour property (force-discharge, inhibit-charge) | expand |
Hi Thomas, thank you very much for working on this. It is high time that we leave external kernel modules for ThinkPads behind us. On 13.11.21 11:42, 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, Nicolo' Piazzalunga, Thomas Koch: > > 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? S-o-b/Co-developed-by/Tested-by is fine with me. I tested your patches. Hardware: - ThinkPad X220, BAT0 - ThinkPad T450s, BAT0+BAT1 - ThinkPad X1C6, BAT0 Test Results: 1. force-discharge Everythings works as expected - Writing including disengaging w/ "auto" : OK - Reading: OK - Battery discharging: OK - Disengaging with "auto": OK 2. inhibit-charge Works as expected: - Writing: OK - Disengaging with "auto": OK Discrepancies: - Battery charge inhibited: BAT0 OK, BAT1 no effect e.g. continues charging - Reading: always returns "auto" Note: the reading discrepancy may be related to Hans' remarks [1]. [1] https://lore.kernel.org/all/09a66da1-1a8b-a668-3179-81670303ea37@redhat.com/ > > 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/20211108192852.357473-1-linux@weissschuh.net/ > https://lore.kernel.org/platform-driver-x86/21569a89-8303-8573-05fb-c2fec29983d1@gmail.com/ > > 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 | 154 +++++++++++++++++++- > drivers/power/supply/power_supply_sysfs.c | 51 +++++++ > include/linux/power_supply.h | 16 ++ > 4 files changed, 231 insertions(+), 4 deletions(-) > > > base-commit: 66f4beaa6c1d28161f534471484b2daa2de1dce0 > -- Freundliche Grüße / Kind regards, Thomas Koch Mail : linrunner@gmx.net Web : https://linrunner.de/tlp
On 2021-11-16 17:56+0100, Thomas Koch wrote: > thank you very much for working on this. It is high time that we leave > external kernel modules for ThinkPads behind us. > > On 13.11.21 11:42, 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, Nicolo' Piazzalunga, Thomas Koch: > > > > 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? > S-o-b/Co-developed-by/Tested-by is fine with me. > > I tested your patches. > > Hardware: > > - ThinkPad X220, BAT0 > - ThinkPad T450s, BAT0+BAT1 > - ThinkPad X1C6, BAT0 > > Test Results: > > 1. force-discharge > > Everythings works as expected > - Writing including disengaging w/ "auto" : OK > - Reading: OK > > - Battery discharging: OK > - Disengaging with "auto": OK > > 2. inhibit-charge > > Works as expected: > - Writing: OK > > - Disengaging with "auto": OK > > > Discrepancies: > - Battery charge inhibited: BAT0 OK, BAT1 no effect e.g. continues charging > - Reading: always returns "auto" I tested it on a T460s with two batteries and there inhibit-charge works fine for both batteries. What does not work is setting force-discharge for both batteries at the same time. This makes somewhat sense as on a physical level probably only one of them can be used at a time. Mark Pearson: Could you confirm that this is the intended behaviour? In my changes queued for v2 of the series[0] I added validation of the written settings and an EIO is now reported if the settings were not applied, so this should help userspace handle this situatoin. The plan is to submit v2 after the first round of review for the core PM changes. [0] https://git.sr.ht/~t-8ch/linux/tree/charge-control
Hi Thomas, On 2021-11-17 12:57, Thomas Weißschuh wrote: > On 2021-11-16 17:56+0100, Thomas Koch wrote: >> thank you very much for working on this. It is high time that we leave >> external kernel modules for ThinkPads behind us. >> >> On 13.11.21 11:42, 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, Nicolo' Piazzalunga, Thomas Koch: >>> >>> 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? >> S-o-b/Co-developed-by/Tested-by is fine with me. >> >> I tested your patches. >> >> Hardware: >> >> - ThinkPad X220, BAT0 >> - ThinkPad T450s, BAT0+BAT1 >> - ThinkPad X1C6, BAT0 >> >> Test Results: >> >> 1. force-discharge >> >> Everythings works as expected >> - Writing including disengaging w/ "auto" : OK >> - Reading: OK >> >> - Battery discharging: OK >> - Disengaging with "auto": OK >> >> 2. inhibit-charge >> >> Works as expected: >> - Writing: OK >> >> - Disengaging with "auto": OK >> >> >> Discrepancies: >> - Battery charge inhibited: BAT0 OK, BAT1 no effect e.g. continues charging >> - Reading: always returns "auto" > > I tested it on a T460s with two batteries and there inhibit-charge works > fine for both batteries. > What does not work is setting force-discharge for both batteries at the same > time. > This makes somewhat sense as on a physical level probably only one of them can > be used at a time. > > Mark Pearson: Could you confirm that this is the intended behaviour? > Confirmed - only one battery can be used with the BDSS command
Hi Thomas, On 17.11.21 18:57, Thomas Weißschuh wrote: > On 2021-11-16 17:56+0100, Thomas Koch wrote: >> thank you very much for working on this. It is high time that we leave >> external kernel modules for ThinkPads behind us. >> >> On 13.11.21 11:42, 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, Nicolo' Piazzalunga, Thomas Koch: >>> >>> 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? >> S-o-b/Co-developed-by/Tested-by is fine with me. >> >> I tested your patches. >> >> Hardware: >> >> - ThinkPad X220, BAT0 >> - ThinkPad T450s, BAT0+BAT1 >> - ThinkPad X1C6, BAT0 >> >> Test Results: >> >> 1. force-discharge >> >> Everythings works as expected >> - Writing including disengaging w/ "auto" : OK >> - Reading: OK >> >> - Battery discharging: OK >> - Disengaging with "auto": OK >> >> 2. inhibit-charge >> >> Works as expected: >> - Writing: OK >> >> - Disengaging with "auto": OK >> >> >> Discrepancies: >> - Battery charge inhibited: BAT0 OK, BAT1 no effect e.g. continues charging >> - Reading: always returns "auto" > > I tested it on a T460s with two batteries and there inhibit-charge works > fine for both batteries. > What does not work is setting force-discharge for both batteries at the same > time. > This makes somewhat sense as on a physical level probably only one of them can > be used at a time. My experience confirms your consideration. The ThinkPad battery circuit can handle exactly one battery at a time i.e. - Charging, AC connected - Forced discharging, AC connected - Discharging, AC disconnected The other battery is always idle during this time. > Mark Pearson: Could you confirm that this is the intended behaviour? > > In my changes queued for v2 of the series[0] I added validation of the written > settings and an EIO is now reported if the settings were not applied, so this > should help userspace handle this situatoin. > > The plan is to submit v2 after the first round of review for the core PM > changes. Please wait until i'm finished with testing your queued v2. I am getting errors here and would first like to rule out homemade problems with my kernel build and/or base version. > [0] https://git.sr.ht/~t-8ch/linux/tree/charge-control -- Freundliche Grüße / Kind regards, Thomas Koch Mail : linrunner@gmx.net Web : https://linrunner.de/tlp