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@xxxxxxx >> >> 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@xxxxxxxxxxxxxxx >>>>> 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 >>> >> >
