Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge

[Sebastian Reichel <sre@kernel.org>]

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

Hi,

On Wed, Apr 07, 2021 at 10:33:41AM +0000, Barnabás Pőcze wrote:
> 2021. április 7., szerda 12:24 keltezéssel, Hans de Goede írta:
> > 2. If we add support for this to the kernel we should probably
> > first agree on standardized power-supply class property names for
> > these, rather then coming up with our own names. ATM we register
> > 2 names for the charge start threshold, the one which the thinkpad_acpi
> > code invented and the standardized name which was later added.
> >
> > I've added Sebastian, the power-supply class / driver maintainer to
> > the Cc. for this. Sebastian Nicolo wants to add support for 2 new
> > features as power-supply properties:
> >
> > --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> > ...
> > +Battery forced discharging
> > +--------------------------
> > +
> > +sysfs attribute:
> > +/sys/class/power_supply/BATx/force_discharge
> > +
> > +Setting this attribute to 1 forces the battery to discharge while AC is attached.
> > +Setting it to 0 terminates forced discharging.
> > +
> > +Battery charge inhibiting
> > +--------------------------
> > +
> > +sysfs attribute:
> > +/sys/class/power_supply/BATx/inhibit_discharge
> > +
> > +Setting this attribute to 1 stops charging of the battery as a manual override
> > +over the threshold attributes. Setting it to 0 terminates the override.
> >
> 
> "inhibit_**discharge**"
> "stops **charging** of the battery"
> 
> I'm wondering if it should be "inhibit_charge" or something like that?

Text and file name also seem to have reverse meaning for me. I
assume the text is the correct one, since it does not seem to
make sense inhibiting discharge. That would result in instant
poweroff on AC loss?

> > Sebastian, I believe that this should be changes to instead be documented
> > in: Documentation/ABI/testing/sysfs-class-power
> > and besides the rename I was wondering if you have any remarks on the proposed
> > API before Nicolo sends out a v2 ?

IIUIC you have 'force_discharge', which basically means the system
is running from battery power despite an AC adapter being connected
and 'inhibit_discharge', which inhibits charging, so system does not
charge battery when AC is connected, but uses AC to supply itself
(so battery is idle)?

We already have this kind of features on embedded systems (which
often provide all kind of charger details). Those drivers solve
this by having a writable 'status' property in the charger device:

What:           /sys/class/power_supply/<supply_name>/status
Date:           May 2007
Contact:        linux-pm@vger.kernel.org
Description:
                Represents the charging status of the battery. Normally this
                is read-only reporting although for some supplies this can be
                used to enable/disable charging to the battery.
 
                Access: Read, Write
 
                Valid values:
                              "Unknown", "Charging", "Discharging",
                              "Not charging", "Full"

If I do not miss anything writing "Discharging" is the same as forced
discharge and "Not Charging" (AKA Idle) is the same as your inhibit feature.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]