Message-ID: <52icxdwohfdiwjvjsbvpxvgaux7hedr6d63zkyhlvbg3t6ykrj@zm47trzk5l5k>
Date: Mon, 16 Sep 2024 10:48:04 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Barnabás Czémán <trabarni@...il.com>, 
	Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Andrew Davis <afd@...com>, 
	linux-pm@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Barnabás Czémán <barnabas.czeman@...nlining.org>
Subject: Re: [PATCH 1/2] dt-bindings: power: supply: bq256xx: Add
 omit-battery-class property

Hi,

On Sat, Sep 07, 2024 at 01:11:57PM GMT, Krzysztof Kozlowski wrote:
> On 07/09/2024 13:07, Barnabás Czémán wrote:
> > Add omit-battery-class property for avoid system create a battery device.
> 
> This does not help much, basically repeats commit subject. You need to
> answer to "why?".

Exposing two battery devices for a single battery is a bug, since that
means there are two distinct batteries. Also note, that platforms having
multiple batteries is a real thing. e.g. some Thinkpads used to have an
internal battery and a hot-swappable one.

> > Signed-off-by: Barnabás Czémán <barnabas.czeman@...nlining.org>
> > ---
> >  Documentation/devicetree/bindings/power/supply/bq256xx.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/power/supply/bq256xx.yaml b/Documentation/devicetree/bindings/power/supply/bq256xx.yaml
> > index a76afe3ca299..744f5782e8e7 100644
> > --- a/Documentation/devicetree/bindings/power/supply/bq256xx.yaml
> > +++ b/Documentation/devicetree/bindings/power/supply/bq256xx.yaml
> > @@ -62,6 +62,12 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/phandle
> >      description: phandle to the battery node being monitored
> >  
> > +  omit-battery-class:
> > +    type: boolean
> > +    description: |
> > +      If this property is set, the operating system does not try to create a
> > +      battery device.
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.

Fully agreed. Also I think we already have the necessary information
in the DT bindings. If there is a fuel-gauge in addition to the
bq256xx charger, there should be a power-supplies link [0] between
those two. Without the fuel-gauge obviously no such link exists. So
the existance of this link can be used to decide wether a battery
device should be registered or not.

If the link exists, the charger driver should not create its own
battery device. In the future the extension API might be used to
let the charger extend the fuel gauge in case it is missing support
for some battery properties, which can be provided by the charger.

Note, that the link is going the other way around (from the battery
to the charger). Also the fuel-gauge device will be registered after
the charger device, so you cannot rely on power_supply_for_each_device
when the charger probes. I see two options to solve that:

1. Create a new power-supply core function, which goes through all DT
   nodes, check that the nodename is "battery" or "fuel-gauge" and have
   a property "power-supplies" with a phandle pointing to the charger
   node.

2. Register the battery device at charger probe time and when the
   fuel-gauge driver is registered, call a (to be introduced) callback
   function in the charger driver, which unregisters the charger's
   battery.

I have a slight preference for the second option, but I'm fine with
either way. This does not create new ABI and can be easily changed
in the future.

[0] Documentation/devicetree/bindings/power/supply/power-supply.yaml

Greetings,

-- Sebastian

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

Powered by blists - more mailing lists