Hi Josh,
On Mon, 22 Dec 2014, Josh Wu wrote:
> Hi, Guennadi
>
> On 12/20/2014 6:16 AM, Guennadi Liakhovetski wrote:
> > On Fri, 19 Dec 2014, Josh Wu wrote:
> >
> > > Hi, Guennadi
> > >
> > > Thanks for the review.
> > >
> > > On 12/19/2014 5:59 AM, Guennadi Liakhovetski wrote:
> > > > Hi Josh,
> > > >
> > > > Thanks for your patches!
> > > >
> > > > 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...@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.
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().
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...
> That is the motivation I want ov2640 be probed even without "mclk".
ov2640 can be used with other boards and camera hosts, not only your
specific board. In other configurations your change will break the driver.
> > Ok, think about this: you check whether priv->clk is set on each
> > .s_power() call, which is already a bit awkward.
> yes, it is.
>
> > Such approach can be used
> > when there's no other way to perform a one-time action, but here we have
> > one. But never mind, that's not the main problem. If priv->clk isn't set,
> > you try to acquire it. But during probing, when this function is called
> > for the first time clock isn't available yet, but you still want to
> > succeed probing. So, you just issue a warning and continue. But then later
> > an application opens the camera, .s_power() is called again, but for some
> > reason the clock might still be not available, and this time you should
> > fail.
>
> > But you don't, you succeed and then you'll fail somewhere later,
> > presumably, with a timeout waiting for frames. Am I right?
> if the clock (v4l2 clock: mclk) is not available, then, there is no camera
> host available.
This isn't true - from the hardware perspective. The clock can be supplied
by a different source.
> So the system should have no v4l2 device found.
> I think in this case the application cannot call the camera sensor .s_power()
> via v4l2 ioctl.
> So the timeout case should not happened.
No, sorry, I meant a physical clock, not aclock object. You can register
the complete framework and try to use it, but if the physical clock isn't
enabled, the camera sensor won't function correctly.
> > > > > @@ -1078,21 +1087,21 @@ static int ov2640_probe(struct i2c_client
> > > > > *client,
> > > > > if (priv->hdl.error)
> > > > > return priv->hdl.error;
> > > > > - priv->clk = v4l2_clk_get(&client->dev, "mclk");
> > > > > - if (IS_ERR(priv->clk)) {
> > > > > - ret = PTR_ERR(priv->clk);
> > > > > - goto eclkget;
> > > > > - }
> > > > > -
> > > > > ret = ov2640_video_probe(client);
> > > > The first thing the above ov2640_video_probe() function will do is call
> > > > ov2640_s_power(), which will request the clock. So, by moving requesting
> > > > the clock from ov2640_probe() to ov2640_s_power() doesn't change how
> > > > probing will be performed, am I right?
> > > yes, you are right. In this patch, the "mclk" will requested by
> > > ov2640_s_power().
> > >
> > > The reason why I put the getting "mclk" code from ov2640_probe() to
> > > ov2640_s_power() is : as the "mclk" here is camera host's peripheral
> > > clock.
> > > That means ov2640 still can be probed properly (read ov2640 id) even no
> > > "mclk". So when I move this code to ov2640_s_power(), otherwise the
> > > ov2640_probe() will be failed or DEFER_PROBE.
> > >
> > > Is this true for all camera host? If it's not true, then I think use
> > > -EPROBE_DEFER would be a proper way.
> > Sorry, not sure what your question is.
> Sorry, I don't make me clear here.
> My question should be: Are all the camera host's clock_start()/stop() only
> operate their peripheral clock?
Yes, that's all camera hosts have. They cannot operate other unrelated
clock sources.
> > And I'm not sure ov2640's registers
> > can be accessed with no running clock.
> No, it seems there is a misunderstanding here.
>
> I didn't mean ov2640 can be probed without xvclk.
> What I try to say is the ov2640 can be probed without camera host's peripheral
> clock.
Ok, then your patch will break the driver even earlier - when trying to
access ov2640 registers without enabling the clock.
> > I think some camera sensors can do
> > this, but I have no idea about this one. How did you verify? Is it
> > mentioned in a datasheet? Or did you really disconnected (grounded) the
> > sensor clock input and tried to access its reqisters?
>
> > If you just
> > verified, that it's working without requesting the clock, are you sure
> > your clock output isn't running permanently all the time anyway?
> I didn't verify the those method as I only probed the ov2640 without ISI
> enabled. ISI peripheral clock is disabled and etc.
Right, this means a separate (probably always-on) clock source is used on
your specific board, but this doesn't have to be the case on all other
boards, where ov2640 is used.
> > > > Or are there any other patched,
> > > > that change that, that I'm overseeing?
> > > >
> > > > If I'm right, then I would propose an approach, already used in other
> > > > drivers instead of this one: return -EPROBE_DEFER if the clock isn't
> > > > available during probing. See ef6672ea35b5bb64ab42e18c1a1ffc717c31588a
> > > > for
> > > > an example. Or did I misunderstand anything?
> I can implement with your method. like in probe() function, request the
> v4l2_clk "mclk", if failed then return -EPROBE_DEFER.
Yes, please, I think this would be a correct solution.
> But I remember you mentioned that you will remove the v4l2 clock in future.
> See ff5430de commit message.
> So I just want to not so depends on the v4l2_clk "mclk".
This might or might not happen. We so far depend on it. And we might also
keep it, just adding a CCF V4L2 clock driver to handle generic clock
sources like in case (c) above.
Thanks
Guennadi
> Best Regards,
> Josh Wu
>
> > > Actually months ago I already done a version of ov2640 patch which use
> > > -EPROBE_DEFER way.
> > >
> > > But now I think the ov2640 can be probed correctly without "mclk", so it
> > > is no
> > > need to return -EPROBE_DEFER.
> > > And the v4l2 asyn API can handle the synchronization of host. So I prefer
> > > to
> > > use this way.
> > > What do you think about this?
> > >
> > > Best Regards,
> > > Josh Wu
> > >
> > > > Thanks
> > > > Guennadi
> > > >
> > > > > if (ret) {
> > > > > - v4l2_clk_put(priv->clk);
> > > > > -eclkget:
> > > > > - v4l2_ctrl_handler_free(&priv->hdl);
> > > > > + goto evideoprobe;
> > > > > } else {
> > > > > dev_info(&adapter->dev, "OV2640 Probed\n");
> > > > > }
> > > > > + ret = v4l2_async_register_subdev(&priv->subdev);
> > > > > + if (ret < 0)
> > > > > + goto evideoprobe;
> > > > > +
> > > > > + return 0;
> > > > > +
> > > > > +evideoprobe:
> > > > > + v4l2_ctrl_handler_free(&priv->hdl);
> > > > > return ret;
> > > > > }
> > > > > @@ -1100,7 +1109,9 @@ static int ov2640_remove(struct i2c_client
> > > > > *client)
> > > > > {
> > > > > struct ov2640_priv *priv = to_ov2640(client);
> > > > > - v4l2_clk_put(priv->clk);
> > > > > + v4l2_async_unregister_subdev(&priv->subdev);
> > > > > + if (priv->clk)
> > > > > + v4l2_clk_put(priv->clk);
> > > > > v4l2_device_unregister_subdev(&priv->subdev);
> > > > > v4l2_ctrl_handler_free(&priv->hdl);
> > > > > return 0;
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > --
> > > > 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
>
--
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
