Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Thomas Koch <linrunner@gmx.net>]

Hi,

 >> "inhibit_**discharge**"

 >> "stops **charging** of the battery"

 >> I'm wondering if it should be "inhibit_charge" or something like that?
 > Text and file name also seem to have reverse meaning for me. I

 > assume the text is the correct one, since it does not seem to

 > make sense inhibiting discharge. That would result in instant

 > poweroff on AC loss?


Fortunately that's only a typo in the docs file. The actual sysfs node
implemented by patch 2/3 is

	/sys/class/power_supply/BATx/inhibit_charge


--

Freundliche Grüße / Kind regards,

Thomas Koch



Mail : linrunner@gmx.net

Web  : https://linrunner.de/tlp


On 08.04.21 15:51, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Apr 07, 2021 at 10:33:41AM +0000, Barnabás Pőcze wrote:
>> 2021. április 7., szerda 12:24 keltezéssel, Hans de Goede írta:
>>> 2. If we add support for this to the kernel we should probably
>>> first agree on standardized power-supply class property names for
>>> these, rather then coming up with our own names. ATM we register
>>> 2 names for the charge start threshold, the one which the thinkpad_acpi
>>> code invented and the standardized name which was later added.
>>>
>>> I've added Sebastian, the power-supply class / driver maintainer to
>>> the Cc. for this. Sebastian Nicolo wants to add support for 2 new
>>> features as power-supply properties:
>>>
>>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>>> ...
>>> +Battery forced discharging
>>> +--------------------------
>>> +
>>> +sysfs attribute:
>>> +/sys/class/power_supply/BATx/force_discharge
>>> +
>>> +Setting this attribute to 1 forces the battery to discharge while AC is attached.
>>> +Setting it to 0 terminates forced discharging.
>>> +
>>> +Battery charge inhibiting
>>> +--------------------------
>>> +
>>> +sysfs attribute:
>>> +/sys/class/power_supply/BATx/inhibit_discharge
>>> +
>>> +Setting this attribute to 1 stops charging of the battery as a manual override
>>> +over the threshold attributes. Setting it to 0 terminates the override.
>>>
>>
>> "inhibit_**discharge**"
>> "stops **charging** of the battery"
>>
>> I'm wondering if it should be "inhibit_charge" or something like that?
>
> Text and file name also seem to have reverse meaning for me. I
> assume the text is the correct one, since it does not seem to
> make sense inhibiting discharge. That would result in instant
> poweroff on AC loss?
>
>>> Sebastian, I believe that this should be changes to instead be documented
>>> in: Documentation/ABI/testing/sysfs-class-power
>>> and besides the rename I was wondering if you have any remarks on the proposed
>>> API before Nicolo sends out a v2 ?
>
> IIUIC you have 'force_discharge', which basically means the system
> is running from battery power despite an AC adapter being connected
> and 'inhibit_discharge', which inhibits charging, so system does not
> charge battery when AC is connected, but uses AC to supply itself
> (so battery is idle)?
>
> We already have this kind of features on embedded systems (which
> often provide all kind of charger details). Those drivers solve
> this by having a writable 'status' property in the charger device:
>
> What:           /sys/class/power_supply/<supply_name>/status
> Date:           May 2007
> Contact:        linux-pm@vger.kernel.org
> Description:
>                  Represents the charging status of the battery. Normally this
>                  is read-only reporting although for some supplies this can be
>                  used to enable/disable charging to the battery.
>
>                  Access: Read, Write
>
>                  Valid values:
>                                "Unknown", "Charging", "Discharging",
>                                "Not charging", "Full"
>
> If I do not miss anything writing "Discharging" is the same as forced
> discharge and "Not Charging" (AKA Idle) is the same as your inhibit feature.
>
> -- Sebastian
>