| Message ID | 20240907-bq256xx-omit-battery-class-v1-1-45f6d8dbd1e5@mainlining.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add omit-battery-class property for bq256xxx | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 12 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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?". > > Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.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. Best regards, Krzysztof
On Sat, Sep 07, 2024 at 01:07:45PM +0200, Barnabás Czémán wrote: > Add omit-battery-class property for avoid system create a battery device. > > Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org> Please run scripts/checkpatch.pl and fix reported warnings. Then please run and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Your SoB does not match. Best regards, Krzysztof
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@mainlining.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
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. + interrupts: maxItems: 1 description: |
Add omit-battery-class property for avoid system create a battery device. Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org> --- Documentation/devicetree/bindings/power/supply/bq256xx.yaml | 6 ++++++ 1 file changed, 6 insertions(+)