Hi, On Thu, Oct 07, 2021 at 07:56:13AM +0200, Thomas Koch wrote: > > > > > 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:Ack > > > > > > > > > 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. LGTM. -- Sebastian
Attachment:
signature.asc
Description: PGP signature
