Re: [PATCH v1 1/3] dt-bindings: fu540: prci: convert PRCI bindings to json-schema

[Stephen Boyd <sboyd@xxxxxxxxxx>]

Quoting Sagar Kadam (2020-09-10 03:44:02)
> diff --git a/Documentation/devicetree/bindings/clock/sifive/fu540-prci.yaml b/Documentation/devicetree/bindings/clock/sifive/fu540-prci.yaml
> new file mode 100644
> index 0000000..49386cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/sifive/fu540-prci.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2020 SiFive, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/sifive/fu540-prci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SiFive FU540 Power Reset Clock Interrupt Controller (PRCI)
> +
> +maintainers:
> +  - Sagar Kadam <sagar.kadam@xxxxxxxxxx>
> +  - Paul Walmsley  <paul.walmsley@xxxxxxxxxx>
> +
> +description:
> +  On the FU540 family of SoCs, most system-wide clock and reset integration
> +  is via the PRCI IP block.
> +  The clock consumer should specify the desired clock via the clock ID
> +  macros defined in include/dt-bindings/clock/sifive-fu540-prci.h.
> +  These macros begin with PRCI_CLK_.
> +
> +  The hfclk and rtcclk nodes are required, and represent physical
> +  crystals or resonators located on the PCB.  These nodes should be present
> +  underneath /, rather than /soc.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sifive,fu540-c000-prci
> +    description:
> +      Should have "sifive,<soc>-prci", only one value is supported

Drop description and have 

    compatible:
      const: sifive,fu540-c000-prci

> +
> +  reg:
> +    maxItems: 1
> +    description: Describe the PRCI's register target physical address region

Drop description.

> +
> +  clocks:
> +    description:
> +      Should point to the hfclk device tree node and the rtcclk device tree node.

s/device tree node//g

> +      The RTC clock here is not a time-of-day clock, but is instead a high-stability
> +      clock source for system timers and cycle counters.

Better to have:

    clocks:
      items:
        - const: high frequency clock
	- const: RTC clock

Can you add clock-names too? Making it optional is OK.

> +  "#clock-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - "#clock-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    //hfclk and rtcclk present under /, in PCB-specific DT data
> +    hfclk: hfclk {
> +      #clock-cells = <0>;
> +      compatible = "fixed-clock";
> +      clock-frequency = <33333333>;
> +      clock-output-names = "hfclk";
> +    };

Add a newline here?

> +    rtcclk: rtcclk {
> +      #clock-cells = <0>;
> +      compatible = "fixed-clock";
> +      clock-frequency = <1000000>;
> +      clock-output-names = "rtcclk";
> +    };

These may not be necessary either, just have the clock-controller node
reference phandles?

> +
> +    //under /soc, in SoC-specific DT data

Don't think this comment is necessary.

> +    prci: clock-controller@10000000 {
> +      compatible = "sifive,fu540-c000-prci";
> +      reg = <0x10000000 0x1000>;
> +      clocks = <&hfclk>, <&rtcclk>;
> +      #clock-cells = <1>;
> +    };