[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aBsUObKHmJkBFN04@pengutronix.de>
Date: Wed, 7 May 2025 10:05:13 +0200
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Conor Dooley <conor+dt@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Rob Herring <robh@...nel.org>, 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 v4 2/3] dt-bindings: clock: add TI CDCE6214 binding
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@...gutronix.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 |
Powered by blists - more mailing lists