Re: [RFC] add standardized attributes for force_discharge and inhibit_charge

[Thomas Koch <linrunner@gmx.net>]

Hi,

On 06.10.21 23:39, Thomas Weißschuh wrote:
> On 2021-10-06T21:24+0200, Thomas Koch wrote:
>> Date: Wed, 6 Oct 2021 21:24:14 +0200
>> From: Thomas Koch <linrunner@gmx.net>
>> To: Hans de Goede <hdegoede@redhat.com>, Sebastian Reichel
>>   <sebastian.reichel@collabora.com>
>> Cc: Thomas Weißschuh <thomas@weissschuh.net>, Nicolò Piazzalunga
>>   <nicolopiazzalunga@gmail.com>, linux-pm@vger.kernel.org,
>>   "platform-driver-x86@vger.kernel.org"
>>   <platform-driver-x86@vger.kernel.org>, "smclt30p@gmail.com"
>>   <smclt30p@gmail.com>
>> Subject: Re: [RFC] add standardized attributes for force_discharge and
>>   inhibit_charge
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
>>   Thunderbird/78.13.0
>>
>> Hi,
>>
>> On 06.10.21 19:47, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 10/6/21 6:28 PM, Sebastian Reichel wrote:
>>>> Hi,
>>>>
>>>> On Wed, Oct 06, 2021 at 05:27:22PM +0200, Hans de Goede wrote:
>>>>> On 10/6/21 4:49 PM, Thomas Weißschuh wrote:
>>>>>> On 2021-10-06T10:10+0200, Hans de Goede wrote:
>>>>>>> On 10/6/21 12:06 AM, Sebastian Reichel wrote:
>>>>>>>> On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
>>>>>>>>> Right, force-discharge automatically implies charging is
>>>>>>>>> being inhibited, so putting this in one file makes sense.
>>>>>>>>>
>>>>>>>>> Any suggestion for the name of the file?
>>>>>>>>
>>>>>>>> Maybe like this?
>>>>>>>>
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> What: /sys/class/power_supply/<supply_name>/charge_behaviour
>>>>>>>> Date: October 2021
>>>>>>>> Contact: linux-pm@vger.kernel.org
>>>>>>>> Description:
>>>>>>>>    Configure battery behaviour when a charger is being connected.
>>>>>>>>
>>>>>>>>    Access: Read, Write
>>>>>>>>
>>>>>>>>    Valid values:
>>>>>>>>
>>>>>>>>    0: auto / no override
>>>>>>>>       When charger is connected battery should be charged
>>>>>>>>    1: force idle
>>>>>>>>       When charger is connected the battery should neither be charged
>>>>>>>>       nor discharged.
>>>>>>>>    2: force discharge
>>>>>>>>       When charger is connected the battery should be discharged
>>>>>>>>       anyways.
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>
>>>>>>> That looks good to me. Although I just realized that some hw may
>>>>>>> only support 1. or 2. maybe explicitly document this and that
>>>>>>> EOPNOTSUPP will be reported when the value is not supported
>>>>>>> (vs EINVAL for plain invalid values) ?
>>>>>>
>>>>>> Would that not force a userspace applications to offer all possibilities to
>>>>>> the user only to tell them that it's not supported?
>>>>>> If the driver knows what is supported and what not it should make this
>>>>>> discoverable without actually performing the operation.
>>>>>>
>>>>>> Maybe something along the lines of /sys/power/mem_sleep.
>>>>>
>>>>> Good point, but something like /sys/power/mem_sleep works
>>>>> very differently then how all the other power_supply properties work.
>>>>
>>>> Actually we already use this format in power-supply for USB
>>>> types, implemented in power_supply_show_usb_type().
>>>>
>>>>> In general if something is supported or not on a psy class
>>>>> device is communicated by the presence / absence of attributes.
>>>>>
>>>>> So I think we should move back to having 2 separate attributes
>>>>> for this after all; and group the 2 together in the doc and
>>>>> document that enabling (setting to 1) one of force_charge /
>>>>> inhibit_charge automatically clears the setting of the other.
>>>>>
>>>>> Then the availability of the features can simply be probed
>>>>> by checking for the presence of the property files.
>>>>
>>>> If it's two files, then somebody needs to come up with proper
>>>> names. Things like 'force_discharge' look sensible in this context,
>>>> but on a system with two batteries (like some Thinkpads have) it
>>>> is easy to confuse with "I want to discharge this battery before
>>>> the other one (while no AC is connected)". > Ah I did not realize there was already some (read-only) precedence
>>> for this in the psy subsystem.
>>>
>>> Since there is precedence for this using
>>> /sys/class/power_supply/<supply_name>/charge_behaviour
>>>
>>> with an example contents of say:
>>>
>>> [auto] inhibit-charge force-discharge
>>>
>>> Works for me and having 1 file instead of 2 is better then
>>> because this clearly encapsulates that inhibit-charge and
>>> force-discharge are mutually exclusive.
>> In fact they do not reset each other on ThinkPads. It's possible to
>>
>> 1. set force_discharge=1 -- discharging commences
>> 2. set inhibit_charge=1 -- discharging continues, force_discharge remains 1
>> 3. set force_discharge=0 -- battery does not charge, inhibit_charge
>> remains 1
>
> But in the end there are only three states the user cares about, or?
> (inhibit, force_discharge and normal)
>
> So when selecting inhibit or force_discharge the driver itself can reset the
> other option so the users do not have to care about the internal state of the
> EC.
Correct. It works with the three states Sebastian suggested because
force_discharge overrules inhibit_charge.

Whereby for user-friendliness I would prefer Hans' strings

 >>> [auto] inhibit-charge force-discharge


to Sebastian's numbered states.

>
> Thomas
>

Freundliche Grüße / Kind regards,
Thomas Koch

Mail : linrunner@gmx.net
Web  : https://linrunner.de/tlp