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. And some documentation about how conflicts are to be resolved. Thomas > > 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
