On 31.07.2014 13:28, Tero Kristo wrote:

[...]

quoted

diff --git a/drivers/clk/ti/clk-6030.c b/drivers/clk/ti/clk-6030.c
new file mode 100644
index 0000000..61d1834
--- /dev/null
+++ b/drivers/clk/ti/clk-6030.c

Please change the filename to something like clk-twl6030.c. clk-\d+ 
filenames basically denote a SoC under TI clock driver structure.

Ok.

quoted

@@ -0,0 +1,133 @@
+/*
+ * drivers/clk/ti/clk-6030.c

Replace this with some sort of short description instead, see other 
files under drivers/clk/ti. Having an absolute filename here doesn't 
really provide anything.

Ok.

quoted

+ *
+ *  Copyright (C) 2014 Stefan Assmann [off-list ref]
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Clock driver for TI twl6030.
+ */

A basic comment about this driver, how about trying to make it more 
generic, as in making it an i2c-clock driver? I guess there are other 
external chips/boards outside twl6030 family that would be interested in 
using it.

I'll look into that.

[...]

quoted

+static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct clk *clk;
+
+	if (!node)
+		return -ENODEV;
+
+	clk = devm_clk_get(&pdev->dev, "clk32kg");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	return clk_prepare(clk);

This is plain wrong as pointed out earlier. The driver that uses the 
clock must enable it.

Understood. I'll change it to clk_prepare_enable().

[...]

quoted

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index db11b4f..440fe4e 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c

@@ -34,6 +34,7 @@
  #include <linux/platform_device.h>
  #include <linux/regmap.h>
  #include <linux/clk.h>
+#include <linux/clk-provider.h>
  #include <linux/err.h>
  #include <linux/device.h>
  #include <linux/of.h>

@@ -1012,6 +1013,8 @@ static void clocks_init(struct device *dev,
  	u32 rate;
  	u8 ctrl = HFCLK_FREQ_26_MHZ;

+	of_clk_init(NULL);
+

Don't do this please, this is horrible. of_clk_init or alternatives 
should be called from board/SoC specific context, otherwise you end up 
calling the init functions multiple times. If you are facing an issue 
that TI boards do not initialize external clocks properly currently, 
this is because TI clock driver does its init hierarchically and does 
not parse the whole DT for clock nodes. I have an experimental patch 
available I can post for you to try out which provides external clock 
support on TI boards if you like.

If you could provide that patch that'll be great.

Thanks for the comments Tero.

  Stefan


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html