Re: [PATCHv1 01/11] dt-bindings: power: supply: adc-battery: add binding

[Matti Vaittinen <mazziesaccount@gmail.com>]

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>;
> +    };

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~