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. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
