| Message ID | 20230309225041.477440-2-sre@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add DT support for generic ADC battery | expand |
Hi Sebastian, thanks for your patches! On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote: > Add binding for a battery that is only monitored via ADC > channels and simple status GPIOs. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> This does look very useful. > +title: ADC battery > + > +maintainers: > + - Sebastian Reichel <sre@kernel.org> > + > +description: | > + Basic Battery, which only reports (in circuit) voltage and optionally > + current via an ADC channel. I would over-specify: "voltage over the terminals" and "current out of the battery" so this cannot be misunderstood. + this text: It can also optionally indicate that the battery is full by pulling a GPIO line. > + charged-gpios: > + description: > + GPIO which signals that the battery is fully charged. It doesn't say how, I guess either this is an analog circuit (!) or a charger IC? If it doesn't matter, no big deal, but if something is implicit here, then spell it out please. > + fuel-gauge { This techno-lingo/slang term is a bit unfortunate, but if there are precedents then stick with it. The correct term could be something like battery-capacity-meter I suppose. Yours, Linus Walleij
Hi Linus, On Fri, Mar 10, 2023 at 09:14:39AM +0100, Linus Walleij wrote: > Hi Sebastian, > > thanks for your patches! > > On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote: > > > Add binding for a battery that is only monitored via ADC > > channels and simple status GPIOs. > > > > Signed-off-by: Sebastian Reichel <sre@kernel.org> > > This does look very useful. :) > > +title: ADC battery > > + > > +maintainers: > > + - Sebastian Reichel <sre@kernel.org> > > + > > +description: | > > + Basic Battery, which only reports (in circuit) voltage and optionally > > + current via an ADC channel. > > I would over-specify: "voltage over the terminals" and > "current out of the battery" so this cannot be misunderstood. > > + this text: > > It can also optionally indicate that the battery is full by pulling a GPIO > line. Ack. > > > + charged-gpios: > > + description: > > + GPIO which signals that the battery is fully charged. > > It doesn't say how, I guess either this is an analog circuit (!) or > a charger IC? If it doesn't matter, no big deal, but if something is > implicit here, then spell it out please. In my case the GPIO is provided by a charger chip, that is not software controllable (just reports charge-done & charger-connected via GPIOs). I've seen something similar in a customer device some years ago. I will add a sentence: The GPIO is often provided by charger ICs, that are not software controllable. > > + fuel-gauge { > > This techno-lingo/slang term is a bit unfortunate, but if there are > precedents then stick with it. > > The correct term could be something like battery-capacity-meter > I suppose. Right now in DT we have - specific node name (e.g. chip names) that should be changed :) - smart-battery - battery - fuel-gauge I think fuel-gauge is the most sensible of that list, considering hardware vendors usually call their chips battery fuel gauge. -- Sebastian
On 09/03/2023 23:50, Sebastian Reichel wrote: > Add binding for a battery that is only monitored via ADC > channels and simple status GPIOs. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> Thank you for your patch. There is something to discuss/improve. > + > +maintainers: > + - Sebastian Reichel <sre@kernel.org> > + > +description: | Don't need '|'. > + Basic Battery, which only reports (in circuit) voltage and optionally > + current via an ADC channel. > + > +allOf: > + - $ref: power-supply.yaml# > + > +properties: > + compatible: > + const: adc-battery > + > + charged-gpios: > + description: > + GPIO which signals that the battery is fully charged. > + maxItems: 1 > + > + io-channels: > + minItems: 1 > + maxItems: 3 > + > + io-channel-names: Simpler: minItems: 1 items: - const: voltage - enum: [ current, power ] - const: power > + oneOf: > + - const: voltage > + - items: > + - const: voltage > + - enum: > + - current > + - power > + - items: > + - const: voltage > + - const: current > + - const: power > + What about temperature? For max17040 this was recently proposed and I wonder whether it is desirable. https://lore.kernel.org/all/74ba115e-9838-4983-7b93-188a8260dd8a@linaro.org/ Best regards, Krzysztof
Hi Sebastian, On 3/10/23 00:50, Sebastian Reichel wrote: > Add binding for a battery that is only monitored via ADC > channels and simple status GPIOs. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> > --- > .../bindings/power/supply/adc-battery.yaml | 67 +++++++++++++++++++ > 1 file changed, 67 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/adc-battery.yaml > > diff --git a/Documentation/devicetree/bindings/power/supply/adc-battery.yaml b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml > new file mode 100644 > index 000000000000..9d478bf9d2ee > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml > @@ -0,0 +1,67 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/supply/adc-battery.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ADC battery > + > +maintainers: > + - Sebastian Reichel <sre@kernel.org> > + > +description: | > + Basic Battery, which only reports (in circuit) voltage and optionally > + current via an ADC channel. > + > +allOf: > + - $ref: power-supply.yaml# > + > +properties: > + compatible: > + const: adc-battery > + > + charged-gpios: > + description: > + GPIO which signals that the battery is fully charged. > + maxItems: 1 > + > + io-channels: > + minItems: 1 > + maxItems: 3 > + > + io-channel-names: > + oneOf: > + - const: voltage > + - items: > + - const: voltage > + - enum: > + - current > + - power > + - items: > + - const: voltage > + - const: current > + - const: power Good side of not knowing things is being able to asking for more information ;) So, just by judging these bindings, we have a battery which provides fuel-gauge information via analog line connected to ADC(?) Reading the description you have here and comments by Linus allows me to assume the line can represent current flowing out of the battery, or the battery voltage. My guess then is that the io-channel-names property is going to tell which if these properties is being informed by the specific lines, right(?). Do you think you could add some small description for io-channel-names if you respin the series? I'd like to be more certain I "guessed" things right. ;) Maybe also add the 'power' option in the main description which currently just states voltage and power. (Assuming some devices do actually "expose" power levels via these "channels"? > + > + monitored-battery: true > + > +required: > + - compatible > + - io-channels > + - io-channel-names > + - monitored-battery > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + fuel-gauge { > + compatible = "adc-battery"; > + charged-gpios = <&gpio 42 GPIO_ACTIVE_HIGH>; > + io-channels = <&adc 13>, <&adc 37>; > + io-channel-names = "voltage", "current"; > + > + power-supplies = <&charger>; > + monitored-battery = <&battery>; > + };
diff --git a/Documentation/devicetree/bindings/power/supply/adc-battery.yaml b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml new file mode 100644 index 000000000000..9d478bf9d2ee --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/adc-battery.yaml @@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/supply/adc-battery.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ADC battery + +maintainers: + - Sebastian Reichel <sre@kernel.org> + +description: | + Basic Battery, which only reports (in circuit) voltage and optionally + current via an ADC channel. + +allOf: + - $ref: power-supply.yaml# + +properties: + compatible: + const: adc-battery + + charged-gpios: + description: + GPIO which signals that the battery is fully charged. + maxItems: 1 + + io-channels: + minItems: 1 + maxItems: 3 + + io-channel-names: + oneOf: + - const: voltage + - items: + - const: voltage + - enum: + - current + - power + - items: + - const: voltage + - const: current + - const: power + + monitored-battery: true + +required: + - compatible + - io-channels + - io-channel-names + - monitored-battery + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + fuel-gauge { + compatible = "adc-battery"; + charged-gpios = <&gpio 42 GPIO_ACTIVE_HIGH>; + io-channels = <&adc 13>, <&adc 37>; + io-channel-names = "voltage", "current"; + + power-supplies = <&charger>; + monitored-battery = <&battery>; + };
Add binding for a battery that is only monitored via ADC channels and simple status GPIOs. Signed-off-by: Sebastian Reichel <sre@kernel.org> --- .../bindings/power/supply/adc-battery.yaml | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/adc-battery.yaml