Re: [PATCH RFC 0/6] power: supply: extension API

[Armin Wolf]

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!

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.

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.

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
>
> Best regards,