On 2024-06-08 18:27:07+0000, Armin Wolf wrote: > 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. <snip> > > > > > > > > [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. That indeed sounds like the obvious thing to do. I tried very hard to keep the callback signatures exactly the same as in power_supply_desc and didn't even see this possibility. > > > > > 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. Ack. > 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. I'll take a look and spin that into its own series. Or as you seem to know that driver better, I'd be happy if you did. > 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
