[RFC,PATCH 1/3] Add a common struct clk

Hi Ryan,

quoted

+int clk_enable(struct clk *clk)
+{
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&clk->enable_lock, flags);

	WARN_ON(clk->prepare_count == 0); ?

Added later, but yes.

quoted

+	if (clk->enable_count == 0 && clk->ops->enable)
+		ret = clk->ops->enable(clk);

Does it make sense to have a clock with no enable function which still
returns success from clk_enable? Do we have any platforms which have
NULL clk_enable functions?

It does, yes. Driver code should be always be calling clk_enable before using 
a clock, regardless of the implementation (which it shouldn't have to care 
abut), and should abort their initialisation if the clk_enable() fails.

Some clocks are always running, so the enable op will be empty. This is not an 
error, so the driver is free to continue.

I think that for enable/disable at least we should require platforms to
provide functions and oops if they have failed to do so. In the rare
case that a platform doesn't need to do anything for enable/disable they
can just supply empty functions.

Sounds like useless boilerplate - it's not an error to not need 
enable/disable, so I don't see why we need to add extra effort to handle this 
case.

quoted

+/**
+ * __clk_get - acquire a reference to a clock
+ *
+ * @clk: The clock to refcount
+ *
+ * Before a clock is returned from clk_get, this function should be
called + * to update any clock-specific refcounting.

This is a bit misleading. It's not "should be called", it "is called". I
think you should just remove the documentation for __clk_get/__clk_put
or move it into clk.c since the functions are only used internally by
the common clock code.

It'd be nice to remove this from the header, but this means we'll need extern 
prototypes in clkdev.c. Might be a reasonable compromise though.

quoted

+/**
+ * clk_prepare - prepare clock for atomic enabling.
+ *
+ * @clk: The clock to prepare
+ *
+ * Do any blocking initialisation on @clk, allowing the clock to be
later + * enabled atomically (via clk_enable). This function may sleep.

"Possibly blocking" as below?

Yep, will unify these (and spell "possibly" correctly :) )

Cheers,


Jeremy