Message-ID: <mts2assktel77gycqoynz42fwimdufyzskgcpfm4dyng7yqesd@cqxussdgn2dg>
Date: Mon, 16 Sep 2024 12:58:13 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Armin Wolf <W_Armin@....de>, Hans de Goede <hdegoede@...hat.com>, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v3 6/9] power: supply: core: implement extension API

Hi,

On Sat, Sep 14, 2024 at 04:25:25PM GMT, Thomas Weißschuh wrote:
> On 2024-09-14 12:50:57+0000, Sebastian Reichel wrote:
> > On Wed, Sep 04, 2024 at 09:25:39PM GMT, Thomas Weißschuh wrote:
> > > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > > ---
> > 
> > missing long description :)
> 
> I'll paste the cover letter here for the next one :-)
> 
> > In general I like the concept. It looks like a good candidate to
> > clean up the mess with the custom registration of properties done
> > by the x86 platform code.
> > 
> > Apart from ACPI with custom battery extensions mess, there is
> > something similar in the embedded world: Many charger chips have
> > rudimentary fuel-gauge support. Some drivers register a charger/usb
> > and a battery device. That results in two battery devices when the
> > machine also has a proper fuel gauge. Having two battery devices
> > exposed is bad, since that means the machine has two batteries
> > (like some Thinkpads). Not exposing the battery from the charger
> > device is possibly loosing information (depending on fuel gauge
> > features).
> > 
> > I think it should be possible to register the charger's rudimentary
> > battery device as a power_supply_ext, if it detects a proper fuel
> > gauge being available (e.g. via the DT "power-supplies" property).
> > But then the charger's properties are the preferred ones. I think
> > they should be fallback properties instead. A machine only gets its
> > own fuel-gauge, when that provides better values than the data
> > available from the charger :)
> > 
> > So the question is which property getter/setter should be used
> > when the same property is exposed multiple times. Your examples
> > only add new properties and does not run into this. The intuitive
> > thing would be for properties from extensions to be preferred,
> > but do we actually have a use case for that?
> 
> Currently duplicate properties are straight up rejected.
> With multiple extensions any order-based resolution of duplicates seems
> to be prone for confusion in case the registration order changes.

Ah, I missed the part about rejecting duplicates.

> The x86 platform and embedded usecases seem quite disjunct to me.
> If we can keep them separate that would be easier to implement.

I wouldn't say they are disjunct. The embedded use case is basically
a superset of the x86 variant.

> x86:
> 
> Don't allow any overriding, like the code currently does.
> 
> embedded:
> 
> Add different extension priorities. When registering a new one,
> unregister all lower priority ones; do nothing if a higher priority is
> already registered.
> This makes the result predictable even if the order changes.
> A fallback extension would need quite some bespoke logic I think.

Adding priorities seems sensible, but also requires assigning
priorities to the base device. I need to think more about this, but
it can wait until the basic version for x86 has landed

> Note:
> I noticed that the locking for power_supply_set_property() is currently
> broken. In case you are wondering about that.

ok.

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists