Re: [PATCH 1/3] dt-bindings: power: supply: Add TI TWL603X charger

[Krzysztof Kozlowski]

On 18/09/2024 10:41, Andreas Kemnade wrote:
> Use a fallback compatible since for especially for generic
> defensive setup of parameters, both 6030 and 6032 are the same and
> U-Boot actually uses a generic 6030/32 function to enable the
> charger.
> 
> Signed-off-by: Andreas Kemnade <andreas@xxxxxxxxxxxx>
> ---
>  .../power/supply/ti,twl6030-charger.yaml      | 62 +++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml b/Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml
> new file mode 100644
> index 0000000000000..fe0fe9a78761c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/ti,twl6030-charger.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license. See checkpatch.

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` 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.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/ti,twl6030-charger.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TWL6030/32 BCI (Battery Charger Interface)
> +
> +description: |

Do not need '|' unless you need to preserve formatting.


> +  The battery charger needs to be configured to do any charging besides of
> +  precharging. The GPADC in the PMIC has to be used to get the related
> +  voltages.
> +
> +maintainers:
> +  - Andreas Kemnade <andreas@xxxxxxxxxxxx>
> +
> +allOf:
> +  - $ref: power-supply.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:

No need for items here. Just const directly.

> +          - const: "ti,twl6030-charger"

Not tested.

Drop ""

> +      - items:
> +          - const: "ti,twl6032-charger"
> +          - const: "ti,twl6030-charger"
> +
> +  interrupts:
> +    minItems: 2

Drop minItems

> +    maxItems: 2

... and actually drop both and instead list items with description
(items: -descriptio: ... - description: ....)

> +
> +  io-channels:
> +    items:
> +      - description: VBUS Voltage Channel
> +
> +  io-channel-names:
> +    items:
> +      - const: vusb
> +
> +  monitored-battery:

Just : true

> +    description:
> +      phandle of battery characteristics devicetree node

That's redundant, you do no say anything useful here.

> +
> +required:
> +  - compatible
> +  - interrupts
> +  - monitored-battery
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pmic {

Drop... or this is supposed to be part of parent schema?

> +      bci {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

e.g. charger

> +        compatible = "ti,twl6032-charger", "ti,twl6030-charger";
> +        interrupts = <2>, <5>;
> +        io-channels = <&gpadc 10>;
> +        io-channel-names = "vusb";
> +        monitored-battery = <&bat>;
> +      };
> +    };

Best regards,
Krzysztof