Hi, Guennadi
Thanks for the reply.
And Merry Christmas and happy new year.
On 12/25/2014 6:39 AM, Guennadi Liakhovetski wrote:
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.wu@xxxxxxxxx>
---
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.
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...
I think I understand your point 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.
According to my understanding, if I implement this way I can be
compatible with the camera host which provide the xvclk for ov2640.
So I will prepare the next version with this way.
BTW: do you have any comment for the 1/5 patches?
Best Regards,
Josh Wu
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 majordomo@xxxxxxxxxxxxxxx
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
