Received-SPF: pass (google.com: domain of linux-kernel+bounces-207149-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 8 Jun 2024 19:02:53 +0200
From: Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
To: Armin Wolf <W_Armin@gmx.de>
Cc: Sebastian Reichel <sre@kernel.org>, 
	Jeremy Soller <jeremy@system76.com>, System76 Product Development <productdev@system76.com>, 
	Hans de Goede <hdegoede@redhat.com>, Ilpo =?utf-8?B?SsOkcnZpbmVu?= <ilpo.jarvinen@linux.intel.com>, 
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH RFC 0/6] power: supply: extension API
Message-ID: <b3d265a3-5bf0-4ed7-b959-9c92aac8fa43@t-8ch.de>
References: <20240606-power-supply-extensions-v1-0-b45669290bdc@weissschuh.net>
 <41964782-222c-45fa-846e-3656eff5b3a9@gmx.de>
 <86cafef5-8a41-46c8-8ee6-6b25dd165b58@t-8ch.de>
 <992978fb-d74c-4da7-a103-9b623b78d889@gmx.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <992978fb-d74c-4da7-a103-9b623b78d889@gmx.de>

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@weissschuh.net/
> > > 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@weissschuh.net>
> > > > ---
> > > > 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