On 09/02/2016 07:04 AM, Rob Herring wrote:

On Fri, Aug 26, 2016 at 02:45:49PM -0700, York Sun wrote:

quoted

From: York Sun <redacted>

SI5338 is a programmable clock generator. It has 4 sets of inputs,
PLL, multisynth and dividers to make 4 outputs. This driver splits
them into multiple clocks to comply with common clock framework.

See Documentation/devicetree/bindings/clock/silabs,si5338.txt for
details.

Signed-off-by: York Sun <redacted>
CC: Mike Turquette <mturquette@baylibre.com>
CC: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
CC: Guenter Roeck <linux@roeck-us.net>
CC: Andrey Filippov <redacted>
CC: Paul Bolle <redacted>

7 versions before you decide to start cc'ing DT list?

7? I only started to CC DT list since version 8. Didn't know I needed 
to. I thought device tree binding is part of driver.

[...]

quoted

diff --git a/Documentation/devicetree/bindings/clock/silabs,si5338.txt b/Documentation/devicetree/bindings/clock/silabs,si5338.txt
new file mode 100644
index 0000000..cc7ae8e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/silabs,si5338.txt

@@ -0,0 +1,192 @@
+Binding for Silicon Labs Si5338 programmable i2c clock generator.
+
+Reference
+[1] Si5338 Data Sheet
+    http://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf
+
+The Si5338 is a programmable i2c clock generators with up to 4 output
+clocks. It has 4 sets of possible input clocks
+
+IN1/IN2: differential
+IN3: single-ended
+IN4: single-ended
+IN5/IN6: differential
+
+Additionally, IN1/IN2 can be used as XTAL with different setting.
+The clock tree looks like below (without support of zero-delay)
+
+
+      IN1/IN2 IN3         IN4 IN5/IN6
+         |     |           |     |
+   ------|     |           |     |
+   |     |     |           |     |
+   |     \     /           \     /
+   |      \   /             \   /
+   |       \ /               \ /
+ XTAL     REFCLK            FBCLK
+   |       |  \             /   |
+   |       |   \           /    |
+   |       | DIVREFCLK DIVFBCLK |
+   |       |     \       /      |
+   |       |      \     /       |
+   |       |       \   /        |
+   |       |        PLL         |
+   |       |      / | | \       |
+   |       |     /  / \  \      |
+   |       |    /  /   \  \     |
+   |       |   /   |   |   \    |
+   |       |   |   |   |   |    |
+   |       |  MS0 MS1 MS2 MS3   |
+   |       |   |   |   |   |    |
+
+       OUT0  OUT1  OUT2  OUT3
+
+The output clock can choose from any of the above clock as its source, with
+exceptions: MS1 can only be used for OUT1, MS2 can only be used for OUT2, MS3
+can only be used for OUT3.
+
+==I2C device node==
+
+Required properties:
+- compatible: shall be "silabs,si5338".
+- reg: i2c device address, shall be 0x60, 0x61, 0x70, or 0x71
+- #clock-cells: shall be set to 1 for multiple outputs
+- clocks: list of parent clocks in the order of <xtal>, <in1/2>, <in3>, <in4>,
+	 <in5/6>.
+	Note, xtal and in1/2 are mutually exclusive. Only one can be set.
+- clock-names: The name of the clocks in the same order.
+- #address-cells: shall be set to 1.
+- #size-cells: shall be set to 0.
+- silabs,pll-master: If PLL is used, pick one MS (0, 1, 2, or 3) to allow
+	chaning PLL rate. This is arbitrary since MS0/1/2/3 share one PLL.
+	PLL can be calculated backward to satisfy MS. This node is not
+	required if PLL is not used, or if silabs,pll-vco is set.
+
+Optional properties if not set by platform driver:
+- silab,name-prefix: name prefix for si5338
+        If multiple si5338 chips exist, use name-prefix to form unique names.
+	If omitted, i2c bus name will be used as the prefix.

Drop this please. There should not be any need for it.

Which part? The default name, or slab,name-prefix completely?

When multiple si5338 chips exist, we need to name the clocks 
differently, don't we?

quoted

+- silab,ref-source: source of refclk, valid value is defined as
+	#define SI5338_REF_SRC_CLKIN12          0
+	#define SI5338_REF_SRC_CLKIN3           1
+	#define SI5338_REF_SRC_XTAL             4
+- silab,fb-source:  source of fbclk, valid value is defined as
+	#define SI5338_FB_SRC_CLKIN4            2
+	#define SI5338_FB_SRC_CLKIN56           3
+	#define SI5338_FB_SRC_NOCLK             5
+- silabs,pll-source: source of pll, valid value is defined as
+	#define SI5338_PFD_IN_REF_REFCLK           0
+	#define SI5338_PFD_IN_REF_FBCLK            1
+	#define SI5338_PFD_IN_REF_DIVREFCLK        2
+	#define SI5338_PFD_IN_REF_DIVFBCLK         3
+	#define SI5338_PFD_IN_REF_XOCLK            4
+	#define SI5338_PFD_IN_REF_NOCLK            5
+- silabs,pll-vco: Specify VCO frequency for optimal ratios for all outputs.
+	If specified, silabs,pll-master is ignored.
+
+==Child nodes==
+
+Each of the clock outputs can be configured individually by
+using a child node to the I2C device node. If a child node for a clock
+output is not set, platform driver has to set up.
+
+Required child node properties:
+- name: name for the child node
+        It has to be unique. The name prefix is ignored.
+	If using platform data and the name is not specificed,
+        clkout0/1/2/3 will be used with name prefix.
+- reg: number of clock output.
+
+Optional child node properties:
+- silabs,drive-config: the configuration of output driver
+  The valid value list is long. Please refer to soruce code.

May be long, but needs to be documented here. I don't follow why you are
using strings for the values here.

Using string so it is readable. The acceptable values are

3V3_CMOS_A+
3V3_CMOS_A-
3V3_CMOS_B+
3V3_CMOS_B-
3V3_CMOS_A+B+
3V3_CMOS_A-B+
3V3_CMOS_A+B-
3V3_CMOS_A-B-

2V5_CMOS_A+
2V5_CMOS_A-
2V5_CMOS_B+
2V5_CMOS_B-
2V5_CMOS_A+B+
2V5_CMOS_A-B+
2V5_CMOS_A+B-
2V5_CMOS_A-B-

1V8_CMOS_A+
1V8_CMOS_A-
1V8_CMOS_B+
1V8_CMOS_B-
1V8_CMOS_A+B+
1V8_CMOS_A-B+
1V8_CMOS_A+B-
1V8_CMOS_A-B-

1V5_HSTL_A+
1V5_HSTL_A-
1V5_HSTL_B+
1V5_HSTL_B-
1V5_HSTL_A+B+
1V5_HSTL_A-B+
1V5_HSTL_A+B-
1V5_HSTL_A-B-

3V3_SSTL_A+
3V3_SSTL_A-
3V3_SSTL_B+
3V3_SSTL_B-
3V3_SSTL_A+B+
3V3_SSTL_A-B+
3V3_SSTL_A+B-
3V3_SSTL_A-B-

2V5_SSTL_A+
2V5_SSTL_A-
2V5_SSTL_B+
2V5_SSTL_B-
2V5_SSTL_A+B+
2V5_SSTL_A-B+
2V5_SSTL_A+B-
2V5_SSTL_A-B-

1V8_SSTL_A+
1V8_SSTL_A-
1V8_SSTL_B+
1V8_SSTL_B-
1V8_SSTL_A+B+
1V8_SSTL_A-B+
1V8_SSTL_A+B-
1V8_SSTL_A-B-

3V3_LVPECL
2V5_LVPECL
3V3_LVDS
2V5_LVDS
1V8_LVDS

Will add here in next version.

quoted

+- silabs,clock-source: source clock of the output divider
+	#define SI5338_OUT_MUX_FBCLK            0
+	#define SI5338_OUT_MUX_REFCLK           1
+	#deinfe SI5338_OUT_MUX_DIVFBCLK         2
+	#deinfe SI5338_OUT_MUX_DIVREFCLK        3
+	#deinfe SI5338_OUT_MUX_XOCLK            4
+	#deinfe SI5338_OUT_MUX_MS0              5
+	#deinfe SI5338_OUT_MUX_MSN              6 /* MS0/1/2/3 */
+	#deinfe SI5338_OUT_MUX_NOCLK            7
+- silabs,disable-state : clock output disable state, shall be
+	#define SI5338_OUT_DIS_HIZ              0
+	#define SI5338_OUT_DIS_LOW              1
+	#define SI5338_OUT_DIS_HI               2
+	#define SI5338_OUT_DIS_ALWAYS_ON        3
+- enabled: the output is enabled by default
+	The existence of this node enables the output when the driver is loaded
+	otherwise the clock is only enabled when used

Drop this. Either platform code should claim this and enable the clock
or use the critical clocks binding.

The idea is to specify if the clock(s) should be enabled upon loading 
the driver.  I am not familiar with critical clocks binding. Can you 
point me to its document/code? Not seeing it in kernel tree.

quoted

+
+==Example==
+
+/* 25MHz reference crystal */
+ref25: ref25M {
+	compatible = "fixed-clock";
+	#clock-cells = <0>;
+	clock-frequency = <25000000>;
+};
+clkin56: ref100M {
+	compatible = "fixed-clock";
+	#clock-cells = <0>;
+	clock-frequency = <100000000>;
+};
+i2c-master-node {
+	si5338: clock-generator@70 {
+		compatible = "silabs,si5338";
+		reg = <0x70>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#clock-cells = <1>;
+
+		/* connect xtal to 25MHz, in5/in6 to 100MHz */
+		clocks = <&ref25>, <0>, <0>, <0>, <&clkin56>;
+		clock-names = "xtal", "in12", "in3", "in4", "in56";
+
+		/* connect xtal as source of refclk */
+		silab,ref-source = <SI5338_REF_SRC_XTAL>;
+
+		/* connect in5/in6 as source of fbclk */
+		silab,fb-source = <SI5338_FB_SRC_CLKIN56>;
+
+		/* connect divrefclk as source of pll */
+		silab,pll-source = <SI5338_PFD_IN_REF_DIVREFCLK>;
+
+		/* Choose one MS for pll master */
+		silabs,pll-master = <0>;
+
+		/* Specify pll-vco frequency. pll-master is ignored. */
+		silabs,pll-vco = <2450000000>;
+
+		/* output */
+		clkout0 {

reg property means you need a unit address.

The reg has the address of each output, i.e. 0, 1, 2, 3. What's wrong here?

quoted

+			reg = <0>;
+			silabs,drive-config = "1V8_LVDS";
+			silabs,clock-source = <SI5338_OUT_MUX_MS0>;
+			silabs,disable-state = <SI5338_OUT_DIS_HIZ>;
+			clock-frequency = <125000000>;
+		};
+		clkout1 {
+			reg = <1>;
+			silabs,drive-config = "1V8_LVDS";
+			silabs,clock-source = <SI5338_OUT_MUX_MSN>;
+			silabs,disable-state = <SI5338_OUT_DIS_HIZ>;
+			clock-frequency = <125000000>;
+		};
+		clkout2 {
+			reg = <2>;
+			silabs,drive-config = "1V8_LVDS";
+			silabs,clock-source = <SI5338_OUT_MUX_MSN>;
+			silabs,disable-state = <SI5338_OUT_DIS_HIZ>;
+			clock-frequency = <125000000>;
+		};
+		clkout3 {
+			reg = <3>;
+			silabs,drive-config = "1V8_LVDS";
+			silabs,clock-source = <SI5338_OUT_MUX_MSN>;
+			silabs,disable-state = <SI5338_OUT_DIS_HIZ>;
+			clock-frequency = <125000000>;
+		};
+
+	};
+};

I see no more comment below. Are you OK with the rest of the patch?

York