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

[Thomas Koch <linrunner@gmx.net>]

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

>
> Regards,
>
> Hans
>
>

Freundliche Grüße / Kind regards,
Thomas Koch

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