Hi,

OK for all your remarks, we have changed our implementation to follow
the new one using clkdev.
The new patch will be sent today.

Regards

Philippe

On 06/24/10 16:14, Russell King - ARM Linux wrote:

On Thu, May 27, 2010 at 10:27:28AM +0200, Philippe Langlais wrote:
   

quoted

+#if defined(DEBUG_CGU)
+#define debug(fmt, args...)                                          \
+	printk(PKMOD fmt, ## args)
+#else
+#define debug(fmt, args...)
+#endif
     

pr_debug?
   

quoted

+
+/**
+ * U6 hw low level access clock functions
+ **/
+static void u6_clk_disable(struct clk *clk);
+static int u6_clk_enable(struct clk *clk);
+
+/**
+ * HW enable clock function.
+ **/
+
+static int u67xx_cgu_enable_fake_clock(struct clk *clk)
+{
+	debug("u67xx_cgu_enable_fake_clock for %s\n", clk->name);
+	return 0;
+}
+
+static int u67xx_cgu_enable_hw_clock(struct clk *clk)
+{
+	unsigned long value;
+	debug("u67xx_cgu_enable_hw_clock for %s\n", clk->name);
+
+	if (unlikely(clk->enable_reg == 0)) {
     

These are pointers.  0 is an integer.  Use NULL.  Check your code with
sparse and you'll get warnings for these things.

   

quoted

+/*** Basic clocks ***/
+
+/* Base external input clocks */
+static struct clk func_32k_ck = {
+	.name = "func_32k_ck",
+	.rate = 32000,
+	.flags = RATE_FIXED | ALWAYS_ENABLED,
+	.usecount = 1,
+};
     

Don't put data definitions in header files.

   

quoted

+static struct clk *onchip_clks[] = {
+	/* external root sources */
+	&func_32k_ck,
+	&osc_ck,
+	&sys_ck,
+	&sc_ck,
+	&fix_ck,
+	&tv_ck,
+#ifndef U6_OPTIMIZED_TREE
+	&dsp2_ck,
+#endif
+	&sdm_ck,
+	&arm_ck,
+	&hclk_ck,
+	&hclk2_ck,
+	&pclk1_ck,
+	&pclk2_ck,
+	&tvclk_ck,
     

This all looks very much like it was copied from the OMAP code as of about
a year ago.  The OMAP code has moved forwards with lots of improvements,
including using clkdev to eliminate the disgusting SoC specific code from
drivers (which clkdev is there to prevent.)