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

Krzysztof Kozlowski
2024-09-18T10:46:09Z

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@kemnade.info>
> ---
> .../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@kemnade.info>
> +
> +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