Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Josh Wu <josh.wu@atmel.com>]

On 1/12/2015 6:38 PM, Laurent Pinchart wrote:
> Hi Josh,
>
> On Monday 12 January 2015 17:14:33 Josh Wu wrote:
>> On 1/9/2015 6:47 AM, Laurent Pinchart wrote:
>>> On Thursday 08 January 2015 23:37:58 Guennadi Liakhovetski wrote:
>>>> On Wed, 7 Jan 2015, Josh Wu wrote:
>>>>> On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote:
>>>>>> On Tue, 6 Jan 2015, Josh Wu wrote:
>>>>>>> Hi, Guennadi
>>>>>>>
>>>>>>> After look deep into this patch, I found you miss one line that should
>>>>>>> be changed as well.
>>>>>>> It's In function v4l2_clk_get(), there still has one line code called
>>>>>>> v4l2_clk_find(dev_id, id).
>>>>>>> You need to change it to v4l2_clk_find(dev_id, NULL) as well.
>>>>>>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev,
>>>>>>> "mclk") cannot acquired the "mclk" clock.
>>>>>>>
>>>>>>> After above changes, this patch works for me.
>>>>>> I think you're right, in fact, since we now don't store CCF-based
>>>>>> v4l2_clk wrappers on the list, this can be simplified even further,
>>>>>> I'll update the patch. Did you only test this patch or both?
>>>>> I tested both patches with Atmel-isi driver. For the 2/2 patch I applied
>>>>> the modification Laurent suggested.
>>>>> Those patches works for me.
>>>>>
>>>>> The only concern is in ov2640 I still need to acquired two v4l2 clocks:
>>>>>      "xvclk"  that will get the xvclk CCF clock directly.
>>>>>      "mclk"  that make ISI driver call his clock_start()/stop() to
>>>>>      enable/disable ISI's peripheral clock.
>>>>>
>>>>> If I only get xvclk clock, then the camera capture will be failed with a
>>>>> ISI timeout error.
>>>> No, this doesn't look right to me. The camera sensor has only one clock
>>>> input, so, it should only request one clock. Where does the clock signal
>>>> to the camera come from on your system?
>>> That's correct, the sensor driver only has one clock input, so it should
>>> just request the xvclk clock.
>>>
>>>> If it comes from the ISI itself, you don't need to specify the clock in
>>>> the DT, since the ISI doesn't produce a clock from DT. If you do want to
>>>> have your clock consumer (ov2640) and the supplier (ISI) properly
>>>> described in DT, you'll have to teach the ISI to register a CCF clock
>>>> source, which then will be connected to from the ov2640. If you choose
>>>> not to show your clock in the DT, you can just use v4l2_clk_get(dev,
>>>> "xvclk") and it will be handled by v4l2_clk / soc-camera / isi-atmel.
>>>>
>>>> If the closk to ov2640 is supplied by a separate clock source, then you
>>>> v4l2_clk_get() will connect ov2640 to it directly and soc-camera will
>>>> enable and disable it on power-on / -off as required.
>>> The ISI has no way to supply a sensor clock, the clock is supplied by a
>>> separate clock source.
>>>
>>>>  From your above description it looks like the clock to ov2640 is
>>>> supplied by a separate source, but atmel-isi's .clock_start() /
>>>> .clock_stop() functions still need to be called? By looking at those
>>>> functions it looks like they turn on and off clocks, supplying the ISI
>>>> itself... Instead of only turning on and off clocks, provided by the ISI
>>>> to a camera sensor. If my understanding is right, then this is a bug in
>>>> atmel-isi and it has to be fixed.
>>> That's correct as well, the ISI driver needs to be fixed.
>> Thanks both of you for the details. Now I got it.
>> Indeed, I need fix this in atmel-isi driver not in ov2640 driver.
>> So I will send a new patch for this, which should move the ISI
>> peripheral clock enable/disable() from clock_start/stop() to
>> isi_camera_add_device/remove_device().
> Shouldn't you move it to the start_streaming() and stop_streaming() functions
> instead ? An even better solution would be to use runtime PM to enable/disable
> the ISI clock in the runtime PM resume and suspend handlers, and call
> pm_runtime_get_sync() and pm_runtime_put() when you need the ISI to be
> operational.

Okay, I'll try to add the PM functions for atmel-isi in the meantime.

Best Regards,
Josh Wu
>