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

[Stephen Boyd <sboyd@kernel.org>]

Quoting Sascha Hauer (2025-04-30 02:01:35)
> diff --git a/Documentation/devicetree/bindings/clock/ti,cdce6214.yaml b/Documentation/devicetree/bindings/clock/ti,cdce6214.yaml
> new file mode 100644
> index 0000000000000..d4a3a3df9ceb9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ti,cdce6214.yaml
> @@ -0,0 +1,155 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ti,cdce6214.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI CDCE6214 programmable clock generator with PLL
> +
> +maintainers:
> +  - Sascha Hauer <s.hauer@pengutronix.de>
> +
> +description: >
> +  Ultra-Low Power Clock Generator With One PLL, Four Differential Outputs,
> +  Two Inputs, and Internal EEPROM
> +
> +  https://www.ti.com/product/CDCE6214
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,cdce6214
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 1
> +    items:
> +      enum: [ priref, secref ]
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  '#clock-cells':
> +    const: 1
> +
> +patternProperties:
> +  '^clk@[0-1]$':
> +    type: object
> +    description:
> +      optional child node that can be used to specify input pin parameters. The reg
> +      properties match the CDCE6214_CLK_* defines.

Presumably the EEPROM is typically used to configure all this stuff? Do
you actually need to program this from the kernel, or are you
implementing all this for development purposes?

> +
> +    additionalProperties: false
> +
> +    properties:
> +      reg:
> +        description:
> +          clock input identifier.
> +        minimum: 0
> +        maximum: 1
> +
> +      ti,clkin-fmt:
> +        enum: [ lvcmos, xtal, differential ]
> +        description:
> +          Clock input format.
> +
> +      ti,xo-cload-femtofarads:
> +        description:
> +          Selects load cap for XO in femto Farad (fF). Up to 9000fF
> +        minimum: 3000
> +        maximum: 9000
> +
> +      ti,xo-bias-current-microamp:
> +        description:
> +          Bias current setting of the XO.
> +        minimum: 0
> +        maximum: 1758
> +
> +  '^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,clkout-fmt:
> +        enum: [ cmos, lvds, lp-hcsl ]
> +        description:
> +          Clock input format.

Is it "Clock output format"?

> +
> +      ti,cmosn-mode:
> +        enum: [ disabled, high, low ]
> +        description:
> +          CMOSN output mode.
> +
> +      ti,cmosp-mode:
> +        enum: [ disabled, high, low ]
> +        description:
> +          CMOSP output mode.

Would 'disabled' be the absence of the property? I think we could just
have ti,cmosn-mode = <0> or <1> for low or high.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    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 = "secref";
> +
> +            clk@1 {
> +                reg = <1>; // CDCE6214_CLK_SECREF
> +                ti,clkin-fmt = "xtal";
> +                ti,xo-cload-femtofarads = <4400>;
> +                ti,xo-bias-current-microamp = <295>;
> +            };
> +
> +            clk@3 {
> +                reg = <3>; // CDCE6214_CLK_OUT1
> +                ti,clkout-fmt = "cmos";
> +                ti,cmosp-mode = "high";
> +                ti,cmosn-mode = "low";
> +            };
> +
> +            clk@4 {
> +                reg = <4>; // CDCE6214_CLK_OUT2
> +                ti,clkout-fmt = "lvds";
> +            };
> +
> +            clk@6 {
> +                reg = <6>; // CDCE6214_CLK_OUT4

Can you use the defines instead of numbers so we know they're the same?

> +                ti,clkout-fmt = "lp-hcsl";
> +            };
> +        };
> +    };