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

From: Hans de Goede <hdegoede@redhat.com>
To: "Thomas Koch" <linrunner@gmx.net>,
	"Sebastian Reichel" <sre@kernel.org>,
	"Barnabás Pőcze" <pobrn@protonmail.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>,
	"jwrdegoede@fedoraproject.org" <jwrdegoede@fedoraproject.org>,
	"smclt30p@gmail.com" <smclt30p@gmail.com>
Subject: Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge
Date: Tue, 13 Apr 2021 10:05:12 +0200	[thread overview]
Message-ID: <6f58903f-5219-9aff-78ca-33687e2e4147@redhat.com> (raw)
In-Reply-To: <39c3436a-a08d-9530-3215-854148215312@gmx.net>

Hi,

On 4/9/21 8:33 PM, Thomas Koch wrote:
> Hi,
> 
> On 08.04.21 15:51, Sebastian Reichel wrote:
> 
>> 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.
> 
> There are ThinkPads with two batteries (BAT0, BAT1) and the hardware
> allows to select which one to discharge. An approach through
> /sys/class/power_supply/AC/status won't cover this.

The mentioned status strings come from /sys/class/power_supply/VAT#/status,
rather then from /sys/class/power_supply/AC/status.

There is one problem though, which is that the status attribute is being
managed by drivers/acpi/battery.c. There is infra for a driver like
the thinkpad_apci driver to add new attributes to a power_supply but
AFAIK there is no infra to say intercept writes to an attribute where
the reading is handled by another driver.

I guess we could add some special hook to allow another driver to
intercept status writes.

Sebastian, what is your take on this ?

Regards,

Hans