diff mbox series

[RFC,v2,13/30] Documentation/devicetree: Add renesas,sh7751-cpg binding document.

Message ID 66ed5e27cb600f3317d315c4fd60bd3e9eb09c17.1694596125.git.ysato@users.sourceforge.jp (mailing list archive)
State New, archived
Headers show
Series Device Tree support for SH7751 based board | expand

Commit Message

Yoshinori Sato Sept. 13, 2023, 9:23 a.m. UTC 
Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 .../bindings/clock/renesas,sh7750-cpg.yaml    | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml

Comments

Krzysztof Kozlowski Sept. 13, 2023, 10:44 a.m. UTC | #1 
On 13/09/2023 11:23, Yoshinori Sato wrote:
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>

Same problems as before. Also:

1. Please use subject prefixes matching the subsystem. You can get them
for example with `git log --oneline -- DIRECTORY_OR_FILE` on the
directory your patch is touching.

2. Drop redundant "binding document." Drop full-stop from subject.
Subjects do not have full stops, unlike commit msg which here is missing.

3. Please use scripts/get_maintainers.pl to get a list of necessary
people and lists to CC. It might happen, that command when run on an
older kernel, gives you outdated entries. Therefore please be sure you
base your patches on recent Linux kernel.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.


Best regards,
Krzysztof
Geert Uytterhoeven Sept. 18, 2023, 7:21 p.m. UTC | #2 
Hi Sato-san,

On Wed, Sep 13, 2023 at 11:26 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>

Thanks for your patch!

Patch prefix should be "dt-bindings: clock: renesas:".

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/renesas,cpg-clocks.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas SH7750 / SH7751 Clock Pulse Generator (CPG)
> +
> +maintainers:
> +  - Yoshinori Sato <ysato@users.sourceforge.jp>
> +
> +description:
> +  The Clock Pulse Generator (CPG) generates core clocks for the SoC.  It
> +  includes PLLs, and fixed and variable ratio dividers.
> +
> +  The CPG may also provide a Clock Domain for SoC devices,

That functionality would require '#power-domain-cells'.

> +
> +properties:
> +  compatible:
> +      - const: renesas,sh7750-cpg      # SH7750 / 7750S / 7751

make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml:

    Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml:20:7:
[warning] wrong indentation: expected 4 but found 6 (indentation)
    Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml:20:34:
[error] syntax error: found character '\t' that cannot start any token
(syntax)

> +      - items:
> +          - const: renesas,sh7750r-cpg # SH7750R / 7751R

This is the wrong order for compatible values: they should be ordered
from most-specific to least-specific.
However, given the small but significant differences between the
different variants, I think you need to define all five:

  compatible:
    enum:
      - renesas,sh7750-cpg
      - renesas,sh7750r-cpg
      - renesas,sh7750s-cpg
      - renesas,sh7751-cpg
      - renesas,sh7751r-cpg

> +  reg:
> +    maxItems: 2
> +    items:
> +      - description: FRQCR register
> +      - description: WDT registers

I think the above should be combined into one larger block, containing
the CPG, standby control, and watchdog registers.
A second block would contain the clock stop registers.  Even although
the driver doesn't support these yet, it would be good to have them
in DT.

And probably you want to specify "reg-names", too.

> +
> +  clocks: true

maxItems: 1

> +
> +  '#clock-cells':
> +    const: 1
> +
> +  renesas,mode:
> +    description: Board-specific settings of the MD0 - MD2 bits
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 6
> +
> +required:
> +  - compatible
> +  - reg

+ reg-names

> +  - clocks
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r8a7740-clock.h>
> +        cpg: cpg@ffc00000 {

Please align "cpg:" with "#include" above, using spaces as indentation.

> +               #clock-cells = <1>;
> +               compatible = "renesas,sh7750r-cpg","renesas,sh7750-cpg";
> +               clocks = <&xtal>;
> +               reg = <0xffc00000 2>, <0xffc00008 4>;
> +               renesas,mode = <0x05>
> +        };

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Oct. 3, 2023, 9:16 a.m. UTC | #3 
On Wed, Sep 13, 2023 at 11:26 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  .../bindings/clock/renesas,sh7750-cpg.yaml    | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml
>
> diff --git a/Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml b/Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml
> new file mode 100644
> index 000000000000..bf10a09440ac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/renesas,cpg-clocks.yaml#

warning: ignoring duplicate '$id' value
'http://devicetree.org/schemas/clock/renesas,cpg-clocks.yaml#'

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml b/Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml
new file mode 100644
index 000000000000..bf10a09440ac
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml
@@ -0,0 +1,58 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/renesas,cpg-clocks.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas SH7750 / SH7751 Clock Pulse Generator (CPG)
+
+maintainers:
+  - Yoshinori Sato <ysato@users.sourceforge.jp>
+
+description:
+  The Clock Pulse Generator (CPG) generates core clocks for the SoC.  It
+  includes PLLs, and fixed and variable ratio dividers.
+
+  The CPG may also provide a Clock Domain for SoC devices,
+
+properties:
+  compatible:
+      - const: renesas,sh7750-cpg	# SH7750 / 7750S / 7751
+      - items:
+          - const: renesas,sh7750r-cpg	# SH7750R / 7751R
+
+  reg:
+    maxItems: 2
+    items:
+      - description: FRQCR register
+      - description: WDT registers
+
+  clocks: true
+
+  '#clock-cells':
+    const: 1
+
+  renesas,mode:
+    description: Board-specific settings of the MD0 - MD2 bits
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 6
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/r8a7740-clock.h>
+        cpg: cpg@ffc00000 {
+		#clock-cells = <1>;
+	        compatible = "renesas,sh7750r-cpg","renesas,sh7750-cpg";
+    		clocks = <&xtal>;
+		reg = <0xffc00000 2>, <0xffc00008 4>;
+		renesas,mode = <0x05>
+        };