Re: [PATCH v2] media: V4L2: add temporary clock helpers

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

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