Add binding for a battery that is only monitored via ADC
channels and simple status GPIOs.
Signed-off-by: Sebastian Reichel <[email protected]>
---
.../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 <[email protected]>
+
+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>;
+ };
--
2.39.2
Hi Sebastian,
thanks for your patches!
On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <[email protected]> wrote:
> Add binding for a battery that is only monitored via ADC
> channels and simple status GPIOs.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
This does look very useful.
> +title: ADC battery
> +
> +maintainers:
> + - Sebastian Reichel <[email protected]>
> +
> +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 <[email protected]> wrote:
>
> > Add binding for a battery that is only monitored via ADC
> > channels and simple status GPIOs.
> >
> > Signed-off-by: Sebastian Reichel <[email protected]>
>
> This does look very useful.
:)
> > +title: ADC battery
> > +
> > +maintainers:
> > + - Sebastian Reichel <[email protected]>
> > +
> > +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 <[email protected]>
Thank you for your patch. There is something to discuss/improve.
> +
> +maintainers:
> + - Sebastian Reichel <[email protected]>
> +
> +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/[email protected]/
Best regards,
Krzysztof
Sebastian Reichel <[email protected]> 于2023年3月13日周一 06:31写道：
>
> Hi,
>
> On Sun, Mar 12, 2023 at 01:46:50AM +0800, Zheng Wang wrote:
> > In da9150_charger_probe, &charger->otg_work is bound with
> > da9150_charger_otg_work. da9150_charger_otg_ncb may be
> > called to start the work.
> >
> > If we remove the module which will call da9150_charger_remove
> > to make cleanup, there may be a unfinished work. The possible
> > sequence is as follows:
> >
> > Fix it by canceling the work before cleanup in the da9150_charger_remove
> >
> > CPU0 CPUc1
> >
> > |da9150_charger_otg_work
> > da9150_charger_remove |
> > power_supply_unregister |
> > device_unregister |
> > power_supply_dev_release|
> > kfree(psy) |
> > |
> > | power_supply_changed(charger->usb);
> > | //use
> >
> > Fixes: c1a281e34dae ("power: Add support for DA9150 Charger")
> > Signed-off-by: Zheng Wang <[email protected]>
> > ---
> > v2:
> > - fix wrong description in commit message and mov cancel_work_sync
> > after usb_unregister_notifier suggested by Sebastian Reichel
> > ---
>
> Thanks, queued to power-supply's fixes branch. Please make sure you
> send your patches to the correct destination next time (linux-scsi
> should be linux-pm).
Thanks for your effort. I'll keep that in mind :)
Best regards,
Zheng
>
> > drivers/power/supply/da9150-charger.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/power/supply/da9150-charger.c b/drivers/power/supply/da9150-charger.c
> > index 14da5c595dd9..a87aeaea38e1 100644
> > --- a/drivers/power/supply/da9150-charger.c
> > +++ b/drivers/power/supply/da9150-charger.c
> > @@ -657,6 +657,7 @@ static int da9150_charger_remove(struct platform_device *pdev)
> >
> > if (!IS_ERR_OR_NULL(charger->usb_phy))
> > usb_unregister_notifier(charger->usb_phy, &charger->otg_nb);
> > + cancel_work_sync(&charger->otg_work);
> >
> > power_supply_unregister(charger->battery);
> > power_supply_unregister(charger->usb);
> > --
> > 2.25.1
> >
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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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>;
> + };
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Tue, Mar 14, 2023 at 12:17 AM Sebastian Reichel <[email protected]> wrote:
> There is no mainline board using this driver and I think there
> never was one. I did add a Fixes tag now, but its probably not worth
> any backporting trouble considering it has no users.
Good point. If a tree falls in the wood an no-one is there to hear it,
it doesn't make a sound.
Yours,
Linus Walleij