[PATCH v4 2/5] media: ov2640: add async probe function

[Guennadi Liakhovetski]

On Fri, 26 Dec 2014, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote:
> > On Fri, 26 Dec 2014, Laurent Pinchart wrote:
> > > On Friday 26 December 2014 14:37:14 Josh Wu wrote:
> > >> On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
> > >>> On Mon, 22 Dec 2014, Josh Wu wrote:
> > >>>> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
> > >>>>> On Fri, 19 Dec 2014, Josh Wu wrote:
> > >>>>>> On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> > >>>>>>> On Thu, 18 Dec 2014, Josh Wu wrote:
> > >>>>>>>> To support async probe for ov2640, we need remove the code to get
> > >>>>>>>> 'mclk' in ov2640_probe() function. oterwise, if soc_camera host
> > >>>>>>>> is not probed in the moment, then we will fail to get 'mclk' and
> > >>>>>>>> quit the ov2640_probe() function.
> > >>>>>>>> 
> > >>>>>>>> So in this patch, we move such 'mclk' getting code to
> > >>>>>>>> ov2640_s_power() function. That make ov2640 survive, as we can
> > >>>>>>>> pass a NULL (priv-clk) to soc_camera_set_power() function.
> > >>>>>>>> 
> > >>>>>>>> And if soc_camera host is probed, the when ov2640_s_power() is
> > >>>>>>>> called, then we can get the 'mclk' and that make us
> > >>>>>>>> enable/disable soc_camera host's clock as well.
> > >>>>>>>> 
> > >>>>>>>> Signed-off-by: Josh Wu <josh.wu at atmel.com>
> > >>>>>>>> ---
> > >>>>>>>> v3 -> v4:
> > >>>>>>>> v2 -> v3:
> > >>>>>>>> v1 -> v2:
> > >>>>>>>>      no changes.
> > >>>>>>>>  
> > >>>>>>>>  drivers/media/i2c/soc_camera/ov2640.c | 31 ++++++++++++++-------
> > >>>>>>>>  1 file changed, 21 insertions(+), 10 deletions(-)
> > >>>>>>>> 
> > >>>>>>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>>>> b/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>>>> index 1fdce2f..9ee910d 100644
> > >>>>>>>> --- a/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> > >>>>>>>> @@ -739,6 +739,15 @@ static int ov2640_s_power(struct v4l2_subdev
> > >>>>>>>> *sd, int on)
> > >>>>>>>>     	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > >>>>>>>>     	struct soc_camera_subdev_desc *ssdd =
> > >>>>>>>> soc_camera_i2c_to_desc(client);
> > >>>>>>>>     	struct ov2640_priv *priv = to_ov2640(client);
> > >>>>>>>> +	struct v4l2_clk *clk;
> > >>>>>>>> +
> > >>>>>>>> +	if (!priv->clk) {
> > >>>>>>>> +		clk = v4l2_clk_get(&client->dev, "mclk");
> > >>>>>>>> +		if (IS_ERR(clk))
> > >>>>>>>> +			dev_warn(&client->dev, "Cannot get the mclk.
> > >>>>>>>> maybe soc-camera host is not probed yet.\n");
> > >>>>>>>> +		else
> > >>>>>>>> +			priv->clk = clk;
> > >>>>>>>> +	}
> > >>>>>>>>       	return soc_camera_set_power(&client->dev, ssdd, priv
> > >>>>>>>> ->clk, on);
> > >>>>>>>>     }
> > >>>> 
> > >>>> Just let me explained a little more details at first:
> > >>>> 
> > >>>> As my understanding, current the priv->clk is a v4l2_clk: mclk, which
> > >>>> is a wrapper clock in soc_camera.c. it can make soc_camera to call
> > >>>> camera host's clock_start() clock_stop(). As in ov2640, the real mck
> > >>>> (pck1) is in ov2640 dt node (xvclk). So the camera host's
> > >>>> clock_start()/stop() only need to enable/disable his peripheral
> > >>>> clock.
> > >>> 
> > >>> I'm looking at the ov2640 datasheet. In the block diagram I only see
> > >>> one input clock - the xvclk. Yes, it can be supplied by the camera
> > >>> host controller, in which case it is natural for the camera host
> > >>> driver to own and control it, or it can be a separate clock device -
> > >>> either static or configurable. This is just a note to myself to
> > >>> clarify, that it's one and the same clock pin we're talking about.
> > >>> 
> > >>> Now, from the hardware / DT PoV, I think, the DT should look like:
> > >>> 
> > >>> a) in the ov2640 I2C DT node we should have a clock consumer entry,
> > >>> linking to a board-specific source.
> > >> 
> > >> That's what this patch series do right now.
> > >> In my patch 5/5 DT document said, ov2640 need a clock consumer which
> > >> refer to the xvclk input clock.
> > >> And it is a required property.
> > >> 
> > >>> b) if the ov2640 clock is supplied by a camera host, its DT entry
> > >>> should have a clock source subnode, to which ov2640 clock consumer
> > >>> entry should link. The respective camera host driver should then parse
> > >>> that clock subnode and register the respective clock with the V4L2
> > >>> framework, by calling v4l2_clk_register().
> > >> 
> > >> Ok, So in this case, I need to wait for the "mclk" in probe of ov2640
> > >> driver. So that I can be compatible for the camera host which provide
> > >> the clock source.
> > > 
> > > Talking about mclk and xvclk is quite confusing. There's no mclk from an
> > > ov2640 point of view. The ov2640 driver should call v4l2_clk_get("xvclk").
> > 
> > Yes, I also was thinking about this, and yes, requesting a "xvclk" clock
> > would be more logical. But then, as you write below, if we let the
> > v4l2_clk wrapper first check for a CCF "xvclk" clock, say, none is found.
> > How do we then find the exported "mclk" V4L2 clock? Maybe v4l2_clk_get()
> > should use two names?..
> 
> Given that v4l2_clk_get() is only used by soc-camera drivers and that they all 
> call it with the clock name set to "mclk", I wonder whether we couldn't just 
> get rid of struct v4l2_clk.id and ignore the id argument to v4l2_clk_get() 
> when CCF isn't available. Maybe we've overdesigned v4l2_clk :-)

Sure, that'd be fine with me, if everyone else agrees.

> > >>> c) if the ov2640 clock is supplied by a different clock source, the
> > >>> respective driver should parse it and also eventually call
> > >>> v4l2_clk_register().
> > >>> 
> > >>> Implementing case (b) above is so far up to each individual
> > >>> (soc-camera) camera host driver. In soc-camera host drivers don't
> > >>> register V4L2 clocks themselves, as you correctly noticed, they just
> > >>> provide a .clock_start() and a .clock_stop() callbacks. The
> > >>> registration is done by the soc-camera core.
> > >>> 
> > >>> If I understand correctly you have case (c). Unfortunately, this case
> > >>> isn't supported atm. I think, a suitable way to do this would be:
> > >>> 
> > >>> (1) modify soc-camera to not register a V4L2 clock if the host doesn't
> > >>> provide the required callbacks.
> > >>> 
> > >>> (2) hosts should recognise configurations, in which they don't supply
> > >>> the master clock to clients and not provide the callbacks then.
> > >>> 
> > >>> (3) a separate driver should register a suitable V4L2 clock.
> > >>> 
> > >>> Whereas I don't think we need to modify camera drivers. Their
> > >>> requesting of a V4L2 clock is correct as is.
> > >>> 
> > >>> Some more fine-print: if the clock is supplied by a generic device, it
> > >>> would be wrong for it to register a V4L2 clock. It should register a
> > >>> normal CCF clock, and a separate V4L2 driver should create a V4L2
> > >>> clock from it. This isn't implemented either and we've been talking
> > >>> about it for a while now...
> > > 
> > > v4l2_clk_get() should try to get the clock from CCF with a call to
> > > clk_get() first, and then look at the list of v4l2-specific clocks.
> > 
> > Yes, how will it find the "mclk" when "xvclk" (or any other name) is
> > requested? We did discuss this in the beginning and agreed to use a fixed
> > clock name for the time being...
> 
> Please see above.
> 
> > > That's at least how I had envisioned it when v4l2_clk_get() was
> > > introduced. Let's remember that v4l2_clk was designed as a temporary
> > > workaround for platforms not implementing CCF yet. Is that still needed,
> > > or could be instead just get rid of it now ?
> >
> > I didn't check, but I don't think all platforms, handled by soc-camera,
> > support CCF yet.
> 
> After a quick check it looks like only OMAP1 and SH Mobile are missing. Atmel, 
> MX2, MX3 and R-Car all support CCF. PXA27x has CCF support but doesn't enable 
> it yet for an unknown (to me) reason.
> 
> The CEU driver is used on both arch/sh and arch/arm/mach-shmobile. The former 
> will most likely never receive CCF support, and the latter is getting fixed. 
> As arch/sh isn't maintained anymore I would be fine with dropping CEU support 
> for it.
> 
> OMAP1 is thus the only long-term show-stopper. What should we do with it ?

Indeed, what should we? :)

Thanks
Guennadi

> -- 
> Regards,
> 
> Laurent Pinchart