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

From: "Barnabás Pőcze" <pobrn@protonmail.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Nicolo' Piazzalunga <nicolopiazzalunga@gmail.com>,
	"platform-driver-x86@vger.kernel.org" 
	<platform-driver-x86@vger.kernel.org>,
	Mark Pearson <markpearson@lenovo.com>,
	Nitin Joshi1 <njoshi1@lenovo.com>,
	Sebastian Reichel <sre@kernel.org>,
	"jwrdegoede@fedoraproject.org" <jwrdegoede@fedoraproject.org>,
	"smclt30p@gmail.com" <smclt30p@gmail.com>,
	"linrunner@gmx.net" <linrunner@gmx.net>
Subject: Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge
Date: Wed, 07 Apr 2021 10:33:41 +0000	[thread overview]
Message-ID: <3anWBvkrPqTNQyfx2ZwDaLZKXtw5PMwTTdcgGNt0FaACUSsrkb5PaoqVKxLpxXU-4NcVZ9AqDQLs2VMOmvS-KfxHRmOSQiZlMjyvH282mdQ=@protonmail.com> (raw)
In-Reply-To: <06f65bb5-eca4-c1ba-a8c2-b44f8a94c699@redhat.com>

Hi

2021. április 7., szerda 12:24 keltezéssel, Hans de Goede írta:

> Hi Nicola,
>
> Thank you for your patch series.
>
> I'm not sure what to do with these. I have a couple of concerns here:
>
> 1. These features are useful, but not super useful and as such I wonder
> how often they are used and this how well tested the firmware is wrt these.
> I have added Mark and Nitin from Lenovo to the Cc. Mark, Nitin, can you
> comment on if it is ok from a firmware pov to try and use the following
> battery related ACPI methods on all thinkpads? :
>
> #define GET_DISCHARGE	"BDSG"
> #define SET_DISCHARGE	"BDSS"
> #define GET_INHIBIT	"PSSG"
> #define SET_INHIBIT	"BICS"
>
>
> 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?

> 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 ?
>
> Regards,
>
> Hans
>
>
> On 3/17/21 3:01 PM, Nicolo' Piazzalunga wrote:
> > Lenovo ThinkPad systems have a feature that lets you
> > force the battery to discharge when AC is attached.
> >
> > This patch implements that feature and exposes it via the generic
> > ACPI battery driver in the generic location:
> >
> > /sys/class/power_supply/BATx/force_discharge
> >
> > Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
> > Signed-off-by: Thomas Koch <linrunner@gmx.net>
> > Signed-off-by: Nicolo' Piazzalunga <nicolopiazzalunga@gmail.com>
> [...]

Regards,
Barnabás Pőcze