Am 07.06.24 um 12:26 schrieb Thomas Weißschuh:
On 2024-06-07 01:10:02+0000, Armin Wolf wrote:
Am 06.06.24 um 16:50 schrieb Thomas Weißschuh:
Introduce a mechanism for drivers to extend the properties implemented
by a power supply.
Motivation
----------
Various drivers, mostly in platform/x86 extend the ACPI battery driver
with additional sysfs attributes to implement more UAPIs than are
exposed through ACPI by using various side-channels, like WMI,
nonstandard ACPI or EC communication.
While the created sysfs attributes look similar to the attributes
provided by the powersupply core, there are various deficiencies:
* They don't show up in uevent payload.
* They can't be queried with the standard in-kernel APIs.
* They don't work with triggers.
* The extending driver has to reimplement all of the parsing,
formatting and sysfs display logic.
* Writing a extension driver is completely different from writing a
normal power supply driver.
* Properties can not be properly overriden.
The proposed extension API avoids all of these issues.
An extension is just a "struct power_supply_ext" with the same kind of
callbacks as in a normal "struct power_supply_desc".
The API is meant to be used via battery_hook_register(), the same way as
the current extensions.
For example my upcoming cros_ec charge control driver[0] saves 80 lines
of code with this patchset.
Contents
--------
* Patch 1 and 2 are generic preparation patches, that probably make
sense without this series.
* Patch 3 implements the extension API itself.
* Patch 4 implements a PoC locking scheme for the extension API.
* Patch 5 adds extension support to test_power.c
* Patch 6 converts the in-tree platform/x86/system76 driver to the
extension API.
Open issues
-----------
* Newly registered properties will not show up in hwmon.
To do that properly would require some changes in the hwmon core.
As far as I know, no current driver would extend the hwmon properties anyways.
* As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
it also be gated behind this or another config?
* Only one extension can be used at a time.
So far this should be enough, having more would complicate the
implementation.
* Is an rw_semaphore acceptable?
[0] https://lore.kernel.org/lkml/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@xxxxxxxxxxxxxx/
Nice, i love this proposal!
Good to hear!
I agree that the hwmon update functionality will need some changes in the hwmon core to work,
but there would be at least one driver benefiting from this (dell-wmi-ddv). Maybe we can add
support for this at a later point in time.
Surely. Alternatively we could re-register the hwmon device after an
extension was added.
The possibility of registering multiple power supply extensions on a single power supply will
be necessary to support battery charge control on Dell notebooks in the future. This is because
there will be two drivers on Dell notebooks which register battery extensions: dell-wmi-ddv and
dell-laptop (when support for battery charge control is supported someday).
How difficult would it be to support such scenarios? If its very difficult, then maybe we can implement
this later when the need arises.
It's not really difficult. The problem is in the callback functions
going from a 'struct power_supply' back to the correct extension struct
for use with container_of() to access the drivers private data.
But we can add a marker member to 'struct power_supply_ext' with which
the callback can figure out which of the registered extensions is its
own. Something like "led_hw_trigger_type" in the LED subsystem.
Maybe we can do the same thing as the battery hook API and just pass a pointer to
the power_supply_ext instance to the callbacks. They then can use container_of()
to access the drivers private data if the struct power_supply_ext is embedded
inside the private data struct.
And some documentation about how conflicts are to be resolved.
Thomas
Sound like a plan, i suggest that extensions be prevented from registering with
a power supply containing conflicting properties or containing extensions with
conflicting properties.
As a side note, maybe there is a way to make power_supply_update_groups() available
for other power supply drivers? Afaik the ACPI battery driver would benefit from this too.
Thanks,
Armin Wolf
Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
---
Thomas Weißschuh (6):
power: supply: sysfs: use power_supply_property_is_writeable()
power: supply: core: avoid iterating properties directly
power: supply: core: implement extension API
power: supply: core: add locking around extension access
power: supply: test-power: implement a power supply extension
platform/x86: system76: Use power_supply extension API
drivers/platform/x86/system76_acpi.c | 83 +++++++++---------
drivers/power/supply/power_supply.h | 9 ++
drivers/power/supply/power_supply_core.c | 136 ++++++++++++++++++++++++++++--
drivers/power/supply/power_supply_hwmon.c | 48 +++++------
drivers/power/supply/power_supply_sysfs.c | 39 ++++++---
drivers/power/supply/test_power.c | 102 ++++++++++++++++++++++
include/linux/power_supply.h | 25 ++++++
7 files changed, 357 insertions(+), 85 deletions(-)
---
base-commit: 2df0193e62cf887f373995fb8a91068562784adc
change-id: 20240602-power-supply-extensions-07d949f509d9
