Hi All,
On 11/14/2012 02:06 PM, Laurent Pinchart wrote:
...
+
+static DEFINE_MUTEX(clk_lock);
+static LIST_HEAD(v4l2_clk);
As Sylwester mentioned, what about s/v4l2_clk/v4l2_clks/ ?
Don't you think naming of a static variable isn't important enough?
;-) I think code authors should have enough freedom to at least pick
up single vs. plural form:-) "clks" is too many consonants for my
taste, if it were anything important I'd rather agree to "clk_head" or
"clk_list" or something similar.
clk_list makes sense IMO since the clk_ prefis is the same.
FWIW, clk_list looks fine for me as well.
+void v4l2_clk_put(struct v4l2_clk *clk)
+{
+ if (!IS_ERR(clk))
+ module_put(clk->ops->owner);
+}
+EXPORT_SYMBOL(v4l2_clk_put);
+
+int v4l2_clk_enable(struct v4l2_clk *clk)
+{
+ if (atomic_inc_return(&clk->enable) == 1&& clk->ops->enable) {
+ int ret = clk->ops->enable(clk);
+ if (ret< 0)
+ atomic_dec(&clk->enable);
+ return ret;
+ }
I think you need a spinlock here instead of atomic operations. You
could get preempted after atomic_inc_return() and before
clk->ops->enable() by another process that would call
v4l2_clk_enable(). The function would return with enabling the
clock.
Sorry, what's the problem then? "Our" instance will succeed and call
->enable() and the preempting instance will see the enable count> 1
and just return.
The clock is guaranteed to be enabled only after the call has returned.
The second caller of v4lw_clk_enable() thus may proceed without the
clock being enabled.
In principle enable() might also want to sleep, so how about using a
mutex for the purpose instead of a spinlock?
If enable() needs to sleep we should split the enable call into prepare
and enable, like the common clock framework did.
I'm pretty sure we won't need to toggle this from interrupt context which is
what the clock framework does, AFAIU. Accessing i2c subdevs mandates
sleeping already.
We might not need to have a mutex either if no driver needs to sleep for
this, still I guess this is more likely. I'm ok with both; just thought to
mention this.
Right, I'm fine with a mutex for now, we'll split enable into enable and
prepare later if needed.
How about just dropping reference counting from this code entirely ?
What would be use cases for multiple users of a single clock ? E.g. multiple
sensors case where each one uses same clock provided by a host interface ?
If we allow the sensor subdev drivers to be setting the clock frequency and
each sensor uses different frequency, then I can't see how this can work
reliably. I mean it's the clock's provider that should coordinate and
reference count the clock users. If a clock is enabled for one sensor and
some other sensor is attempting to set different frequency then the
set_rate
callback should return an error. The clock provider will need use
internally
a lock for the clock anyway, and to track the clock reference count too.
So I'm inclined to leave all this refcounting bits out to individual clock
providers.
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
