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

Hi Sato-san,

On Wed, Sep 13, 2023 at 11:26 AM Yoshinori Sato
<[email protected]> wrote:
> Signed-off-by: Yoshinori Sato <[email protected]>

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 <[email protected]>
> +
> +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 -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds