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

[Hans de Goede <hdegoede@redhat.com>]

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.

Regards,

Hans