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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, May 05, 2025 at 10:50:49AM -0700, Stephen Boyd wrote:
> 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?

The EEPROM could be used to configure this. I don't know if the final
product will have the EEPROM programmed, but even if it is, should we
make this mandatory?

Speaking of the EEPROM I think we should make sure that the pin
configuration in the device tree is optional so that we do not overwrite
settings from the EEPROM if it contains valid values.

> > +        enum: [ cmos, lvds, lp-hcsl ]
> > +        description:
> > +          Clock input format.
> 
> Is it "Clock output format"?

Yes.

> 
> > +
> > +      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.

Yes. I think we can do that.

> 
> > +
> > +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?

Yes, I could and have done that, but Krzysztof objected to it here:
https://lore.kernel.org/all/5766d152-51e7-42f5-864f-5cb1798606a3@kernel.org/

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |