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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5766d152-51e7-42f5-864f-5cb1798606a3@kernel.org>
Date: Tue, 8 Apr 2025 16:27:23 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Sascha Hauer <s.hauer@...gutronix.de>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, kernel@...gutronix.de,
 Alvin Šipraga <alsi@...g-olufsen.dk>
Subject: Re: [PATCH 3/3] dt-bindings: clock: add TI CDCE6214 binding

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@...E6214_CLK_SECREF {

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


Best regards,
Krzysztof