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. Regards, Hans
