Re: [PATCH 8/9] dt-bindings: Add documentation for Photonicat PMU

[Krzysztof Kozlowski <krzk@kernel.org>]

On 06/09/2024 11:36, Junhao Xie wrote:
> Add device tree binding documentation for Photonicat PMU MFD, LEDs,
> hardware monitor, power off, power supply, real-time clock watchdog.
> 
> Signed-off-by: Junhao Xie <bigfoot@classfun.cn>
> ---
>  .../hwmon/ariaboard,photonicat-pmu-hwmon.yaml |  40 +++++++
>  .../leds/ariaboard,photonicat-pmu-leds.yaml   |  41 +++++++
>  .../mfd/ariaboard,photonicat-pmu.yaml         | 107 ++++++++++++++++++
>  .../ariaboard,photonicat-pmu-poweroff.yaml    |  34 ++++++
>  .../ariaboard,photonicat-pmu-supply.yaml      |  55 +++++++++
>  .../rtc/ariaboard,photonicat-pmu-rtc.yaml     |  37 ++++++
>  .../ariaboard,photonicat-pmu-watchdog.yaml    |  37 ++++++
>  7 files changed, 351 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
>  create mode 100644 Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
>  create mode 100644 Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
>  create mode 100644 Documentation/devicetree/bindings/power/reset/ariaboard,photonicat-pmu-poweroff.yaml
>  create mode 100644 Documentation/devicetree/bindings/power/supply/ariaboard,photonicat-pmu-supply.yaml
>  create mode 100644 Documentation/devicetree/bindings/rtc/ariaboard,photonicat-pmu-rtc.yaml
>  create mode 100644 Documentation/devicetree/bindings/watchdog/ariaboard,photonicat-pmu-watchdog.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
> new file mode 100644
> index 000000000000..c9b1bab20c31
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ariaboard,photonicat-pmu-hwmon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Photonicat PMU Hardware Monitor
> +
> +maintainers:
> +  - Junhao Xie <bigfoot@classfun.cn>
> +
> +description:
> +  Board temperature sensor on the Photonicat PMU MCU
> +
> +properties:
> +  compatible:
> +    const: ariaboard,photonicat-pmu-hwmon
> +
> +  label:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Label for hwmon device

No resources here. Fold it into parent binding.

> +
> +required:
> +  - compatible
> +  - label
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      serial {
> +          mcu {
> +              compatible = "ariaboard,photonicat-pmu";

Drop, no need.

> +

Messed indentation.

Entire example is redundant. Merge it to parent binding.


> +              hwmon {
> +                  compatible = "ariaboard,photonicat-pmu-hwmon";
> +                  label = "pcat_board";
> +              };
> +          };
> +      };
> diff --git a/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml b/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
> new file mode 100644
> index 000000000000..6ccb0e691b09
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/ariaboard,photonicat-pmu-leds.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Photonicat PMU LEDs
> +
> +maintainers:
> +  - Junhao Xie <bigfoot@classfun.cn>
> +
> +description:
> +  LEDs on the Photonicat PMU MCU
> +
> +allOf:
> +  - $ref: common.yaml#
> +
> +properties:
> +  compatible:
> +    const: ariaboard,photonicat-pmu-leds

Your compatibles per device do not make much sense. You organized
bindings per drivers, but that's not what we want.

> +
> +  label: true

Drop

> +
> +required:
> +  - compatible
> +  - label
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      serial {
> +          mcu {
> +              compatible = "ariaboard,photonicat-pmu";
> +
> +              leds-status {
> +                  compatible = "ariaboard,photonicat-pmu-leds";
> +                  label = "net-status";
> +              };
> +          };
> +      };
> diff --git a/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml b/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
> new file mode 100644
> index 000000000000..df16d9507821
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
> @@ -0,0 +1,107 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/ariaboard,photonicat-pmu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ariaboard Photonicat PMU
> +
> +maintainers:
> +  - Junhao Xie <bigfoot@classfun.cn>
> +
> +description:
> +  Driver for the Power Management MCU in the Ariaboard Photonicat,

Bindings are for hardware, not drivers. Drop it everywhere and explain
hardware.

> +  which provides battery and charger power supply, real-time clock,
> +  watchdog, hardware shutdown.
> +
> +properties:
> +  compatible:
> +    const: ariaboard,photonicat-pmu

That's the only compatible you should have. Drop all others.

> +
> +  current-speed:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 115200
> +    description: PMU Serial baudrate
> +
> +  local-address:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 127
> +    default: 1
> +    description: CPU board address

Address of what? In which notation? It's part of this hardware.


> +
> +  remote-address:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 127
> +    default: 1
> +    description: PMU board address

Eee, no. Your board knows its address. You do not have to tell it.

> +
> +  hwmon:
> +    $ref: /schemas/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
> +
> +  poweroff:
> +    $ref: /schemas/power/reset/ariaboard,photonicat-pmu-poweroff.yaml
> +
> +  rtc:
> +    $ref: /schemas/rtc/ariaboard,photonicat-pmu-rtc.yaml
> +
> +  watchdog:
> +    $ref: /schemas/watchdog/ariaboard,photonicat-pmu-watchdog.yaml
> +
> +patternProperties:
> +  '^leds-(status)':

That's not a pattern.

> +    $ref: /schemas/leds/ariaboard,photonicat-pmu-leds.yaml
> +
> +  '^supply-(battery|charger)$':
> +    $ref: /schemas/power/supply/ariaboard,photonicat-pmu-supply.yaml

Why two nodes?

> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      serial {
> +          photonicat-pmu {
> +              compatible = "ariaboard,photonicat-pmu";
> +              current-speed = <115200>;
> +              local-address = <1>;
> +              remote-address = <1>;
> +
> +              supply-battery {
> +                  compatible = "ariaboard,photonicat-pmu-supply";
> +                  label = "battery";

Nope, drop label.

> +                  type = "battery";

No, there is no type property.

Missing monitored battery.

> +              };
> +
> +              supply-charger {
> +                  compatible = "ariaboard,photonicat-pmu-supply";
> +                  label = "charger";
> +                  type = "charger";
> +              };
> +
> +              hwmon {
> +                  compatible = "ariaboard,photonicat-pmu-hwmon";
> +                  label = "pcat_board";
> +              };
> +
> +              leds-status {
> +                  compatible = "ariaboard,photonicat-pmu-leds";
> +                  label = "net-status";
> +              };
> +
> +              poweroff {
> +                  compatible = "ariaboard,photonicat-pmu-poweroff";
> +              };
> +
> +              rtc {
> +                  compatible = "ariaboard,photonicat-pmu-rtc";
> +              };
> +
> +              watchdog {
> +                  compatible = "ariaboard,photonicat-pmu-watchdog";
> +              };

These are seriously redundant and useless nodes.  There is nothing
beneficial from the nodes above - they are all empty, without resources.
Drop all of them.

I finish the review here.

Best regards,
Krzysztof