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. Thomas
