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@xxxxxxx>
To: Hans de Goede <hdegoede@xxxxxxxxxx>, Sebastian Reichel
<sebastian.reichel@xxxxxxxxxxxxx>
Cc: Thomas Weißschuh <thomas@xxxxxxxxxxxxxx>, Nicolò Piazzalunga
<nicolopiazzalunga@xxxxxxxxx>, linux-pm@xxxxxxxxxxxxxxx,
"platform-driver-x86@xxxxxxxxxxxxxxx"
<platform-driver-x86@xxxxxxxxxxxxxxx>, "smclt30p@xxxxxxxxx"
<smclt30p@xxxxxxxxx>
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@xxxxxxxxxxxxxxx
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@xxxxxxx
Web : https://linrunner.de/tlp
