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

[Sebastian Reichel <sebastian.reichel@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 5264 bytes --]

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@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.
> > > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]