Re: [PATCH 3/3] dt-bindings: clock: add TI CDCE6214 binding

[Krzysztof Kozlowski <krzk@kernel.org>]

On 08/04/2025 14:00, Sascha Hauer wrote:
> +

A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


> +properties:
> +  compatible:
> +    enum:
> +      - ti,cdce6214
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: priref
> +      - const: secref

So one input is optional?

> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  "#clock-cells":

Use consistent quotes, either ' or "

> +    const: 1
> +
> +patternProperties:
> +  "^clk@[0-1]$":
> +    type: object
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      optional child node that can be used to specify input pin parameters. The reg
> +      properties match the CDCE6214_CLK_* defines.
> +
> +    additionalProperties: false
> +
> +    properties:
> +      reg:
> +        description:
> +          clock input identifier.
> +        minimum: 0
> +        maximum: 1
> +
> +      ti,ref-xtal:
> +        type: boolean
> +        description: |
> +          If true use input as XTAL input
> +
> +      ti,ref-lvcmos:
> +        type: boolean
> +        description: |
> +          If true use input as LVCMOS input
> +
> +      ti,ref-diff:
> +        type: boolean
> +        description: |
> +          If true use input as differential input

So these three are an enum string.

> +
> +  "^clk@[2-9]$":
> +    type: object
> +    description: |
> +      optional child node that can be used to specify output pin parameters.  The reg
> +      properties match the CDCE6214_CLK_* defines.
> +
> +    additionalProperties: false
> +
> +    properties:
> +      reg:
> +        description:
> +          clock output identifier.
> +        minimum: 2
> +        maximum: 9
> +
> +      ti,lphcsl:
> +        type: boolean
> +        description: |
> +          If true enable LP-HCSL output mode for this clock
> +
> +      ti,lvds:
> +        type: boolean
> +        description: |
> +          If true enable LVDS output mode for this clock
> +
> +      ti,cmosp:
> +        type: boolean
> +        description: |
> +          If true enable CMOSP output for this clock
> +
> +      ti,cmosn:
> +        type: boolean
> +        description: |
> +          If true enable CMOSN output for this clock

Looks the same here. Anyway having these as subnodes is too much. You
have fixed number of clocks, so you need one or two array properties in
top-level.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - "#clock-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/ti,cdce6214.h>

This file does not exist. Something is odd in this example.

> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        clock-generator@67 {
> +            compatible = "ti,cdce6214";
> +            reg = <0x67>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            #clock-cells = <1>;
> +            clocks = <&clock_ref25m>;
> +            clock-names = "priref";
> +
> +            clk@CDCE6214_CLK_SECREF {

That's not a valid unit address. Use simple numbers, see DT spec and DTS
coding style.


Best regards,
Krzysztof