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

From: Nicolo' Piazzalunga <nicolopiazzalunga@gmail.com>
To: Hans de Goede <hdegoede@redhat.com>,
	Thomas Koch <linrunner@gmx.net>,
	Sebastian Reichel <sre@kernel.org>
Cc: "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>,
	"Barnabás Pőcze" <pobrn@protonmail.com>
Subject: Re: [PATCH 1/3] thinkpad_acpi: add support for force_discharge
Date: Wed, 19 May 2021 16:45:36 +0200	[thread overview]
Message-ID: <c024f687-a246-8dd2-2e9a-32c8f37aa89d@gmail.com> (raw)
In-Reply-To: <8255aa89-fe3b-1de2-0195-a818795b7d32@redhat.com>

Hi Sebastian,

given the discussion below, would you agree on introducing attributes

force_discharge
inhibit_charge

rather than using 'status' property?
or how should we proceed?

Regards,
Nicolo'

On 4/17/21 7:03 PM, Hans de Goede wrote:
> Hi Thomas,
> 
> On 4/17/21 1:49 PM, Thomas Koch wrote:
>> Hi Hans,
>>
>> from a userspace perspective, I don't think it's optimal to combine the
>> two features and the status. For example, how do I find out which one is
>> available?
>>
>> I have to test the writability of status and then still don't know which
>> one is available. Seeing if force_discharge or inhibit_charge are there
>> is much simpler.
>>
>> And then enabling that: triggering force_discharge by writing
>> "Discharging" is ok. But for inhibit_charge we would need a new status,
>> something like "Charging inhibited". This then causes problems for the
>> existing userspace, says: upowerd could not handle it. You remember the
>> "Not charging" patch from Ognen?
> 
> I think you have valid points both wrt the discoverability of the
> feature availability, as well as about a "Charging inhibited" status
> value possibly (likely) causing problems for userspace.
> 
> With that said, you really need to discuss this with Sebastian. I'm fine
> with the thinkpad_acpi patches, but as I already said I want to avoid
> in essence defining new power_supply class userspace API inside
> drivers/platform/x86 .  Last time we did that it ended poorly and more
> in general it is a bad idea.
> 
> So you first need to agree on a set of power_supply class sysfs attributes
> for this and document those in the power_supply class docs, before I can
> merge the thinkpad_acpi patches.
> 
> And the agreeing on and documenting is something which you need to discuss
> with Sebastian (the power_supply maintainer).
> 
> Regards,
> 
> Hans
> 
> 
> 
>> -- 
>>
>> Freundliche Grüße / Kind regards,
>>
>> Thomas Koch
>>
>>
>>
>> Mail : linrunner@gmx.net
>>
>> Web  : https://linrunner.de/tlp
>>
>>
>> On 13.04.21 10:05, Hans de Goede wrote:
>>> 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
>>>
>>
> 