On 10/31/2013 09:56 AM, Tero Kristo wrote:

On 10/31/2013 04:19 PM, Nishanth Menon wrote:

quoted

On 10/25/2013 10:56 AM, Tero Kristo wrote:
[...]

quoted

diff --git a/Documentation/devicetree/bindings/clock/ti/dpll.txt b/Documentation/devicetree/bindings/clock/ti/dpll.txt
new file mode 100644
index 0000000..7b87721
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ti/dpll.txt

[...]

quoted

quoted

+  ti,am3-* dpll types list the registers in the same order, except "autoidle"
+  register is left out as this hardware does not have it, e.g.:
+	reg = <0x40>, <0x50>, <0x60>;
+  results in following register map:
+	base + 0x40 - control
+	base + 0x50 - idlest
+	base + 0x60 - mult-div1
+
+Optional properties:
+- DPLL mode setting - defining any one or more of the following overrides
+  default setting.
+	- ti,low-power-stop : DPLL supports low power stop mode, gating output
+	- ti,low-power-bypass : DPLL output matches rate of parent bypass clock
+	- ti,lock : DPLL locks in programmed rate

quoted

arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_BYPASS) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = ((1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED) |
arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
arch/arm/mach-omap2/dpll3xxx.c: _omap3_dpll_write_clken(clk, DPLL_LOCKED);

There is no if checks for DPLL_LOCKED which is set with ti,lock,
should we just drop it?

Probably better to keep it in, as there might be some code in future 
which needs this. Kind of better for readability also, as the field is 
specified to list the valid modes for the DPLL. One could argue that 
current kernel code is wrong for _omap3_dpll_write_clken(clk, 
DPLL_LOCKED) as it does not check if the mode is valid for the DPLL or not.

yep, the thought did cross my mind.

quoted

[...]

quoted

diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
new file mode 100644
index 0000000..89733c9
--- /dev/null
+++ b/drivers/clk/ti/dpll.c

[...]

quoted

+/**
+ * ti_clk_register_dpll() - Registers the DPLL clock
+ * @name:	Name of the clock node
+ * @parent_names: list of parent names
+ * @num_parents: num of parents in parent_names
+ * @flags:	init flags
+ * @dpll_data:	DPLL data
+ * @ops:	ops for DPLL
+ */
+static struct clk *ti_clk_register_dpll(const char *name,
+					const char **parent_names,
+					int num_parents, unsigned long flags,
+					struct dpll_data *dpll_data,
+					const struct clk_ops *ops,
+					struct regmap *regmap)
+{
+	struct clk *clk;
+	struct clk_init_data init = { NULL };
+	struct clk_hw_omap *clk_hw;
+
+	/* allocate the divider */
+	clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+	if (!clk_hw) {
+		pr_err("%s: could not allocate clk_hw_omap\n", __func__);

here and in every other driver - please dont add pr_err for kzalloc
not able to allocate, kzalloc will provide the warning anyways.

Oh it does? Never seen that happen as kzalloc never failed for me. :) 
However, I can remove these as I don't think they will ever happen anyway.

There have been cleanups such as commit
38bb5253a95f2eb8cb765b7ab88aac686de6cb12 etc.. we dont want to
introduce new ones now.

quoted

quoted

+	clk_hw->ops = hw_ops;
+	of_property_read_u32(node, "reg", (u32 *)&clk_hw->clksel_reg);
+	clk_hw->regmap = regmap;
+	clk_hw->hw.init = &init;
+
+	init.name = name;
+	init.ops = ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	/* register the clock */
+	clk = clk_register(NULL, &clk_hw->hw);
+
+	if (IS_ERR(clk))
+		kfree(clk_hw);
+	else
+		omap2_init_clk_hw_omap_clocks(clk);
+
+	return clk;
+}
+#endif

[...]

quoted

+
+#ifdef CONFIG_ARCH_OMAP3

just a mention - I understand we are still in transition and we need
these #ifdefs, but eventually, we should be able to make the dpll.c
independent of SoC variant #ifdefs.

Eventually yes. Currently it fails to compile if you do OMAP4 / OMAP5 
only build, as you don't have the OMAP3 support routines in.

fair enough.


-- 
Regards,
Nishanth Menon