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 >> >
