Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi Josh, On Tuesday 30 December 2014 18:02:23 Josh Wu wrote: On 12/30/2014 8:15 AM, Laurent Pinchart wrote: On Monday 29 December 2014 16:28:02 Josh Wu wrote: On 12/26/2014 6:06 PM, Laurent Pinchart wrote: On Friday 26 December 2014 10:14:26 Guennadi Liakhovetski wrote: On Fri, 26 Dec 2014, Laurent Pinchart wrote: [snip] 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 :-) Sorry, I'm not clear about how to implement what you discussed here. Do you mean, In the ov2640 driver: 1. need to remove the patch 4/5, add a master clock for sensor No, the sensor has a clock input named xvclk, the ov2640 driver should thus manage that clock. Patch 4/5 does the right thing. However, I've just realized that it will cause regressions on the i.MX27, i.MX31 and i.MX37 3DS development boards that use the sensor without registering a clock named xvclk. You should fix that as part of the patch series. Thanks for the information. So I think to be compatible with i.MX series board, I have two ways: 1. Make the xvclk clock be optional in ov2640 driver. After the i.MX series board switch to CCF, and we can change it to mandatory. 2. switch the i.MX host driver to DT, and add the xvclk to their dts. As I am not similar with i.MX board and cannot test for them. I prefer to the #1, which is simple and work well. We can change the property when CCF DT is introduced to i.MX boards. I'd be fine with this if DT bindings were not required to be stable. The driver can always be fixed later, but biding should be correct from the start. As the ov2640 chip requires a clock, the bindings must require the clock, and the driver must enforce the requirement, at least when the device is instantiated from DT. -- 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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi Guennadi, On Tuesday 30 December 2014 13:12:27 Guennadi Liakhovetski wrote: On Tue, 30 Dec 2014, Josh Wu wrote: [snip] And until omap1 is in the mainline we cannot drop v4l2_clk. s/until/as lonh as/ So I think the better way right now for ov2640 driver is still request both the v4l2_clock: mclk, and the clock: xvclk in probe(). In that way, we can take our time to introduce other patches to merged these two clocks. Does it make sense? How about this sequence, that we cat still try to get in on time for the next merge window: 1. stop using the clock name in v4l2_clk 2. add support for CCF to v4l2_clk, falling back to current behaviour if no clock is found 3. switch ov2640 to using xvclk It looks good at first sight. Otherwise I would propose to add asynchronous probing support to ov2640 _without_ changing the clock name. Whether or not we changee clock's name isn't related directly to async support, since the driver already is using v4l2_clk with the standarf wrong name. So, I would consider these two changes separately - one is a new feature, another is a fix. The only question is in which order to apply them. In general fix-first is preferred, but since in this case the fix requires framework changes, we could go with feature-first. This also makes sense since features need to catch a merge window, whereas a fix can go in later too. So, if we don't manage the above 3-step plan, I would priposw the most primitive patch ro ov2640 just adding async support in line wuth other drivers and without changing the clock name at first. Async support can go in without the clock rename, but DT support can't. -- 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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi Laurent, First of all, sorry, I am currently on a holiday, so, replies are delayed, real work (reviewing or anything else) is impossible. On Tue, 30 Dec 2014, Laurent Pinchart wrote: Hi Guennadi, On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote: On Fri, 26 Dec 2014, Laurent Pinchart wrote: 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: [snip] 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. Can you submit a patch ? That's the best way to find out if anyone objects. Can do, sure, once I am back home and find time for this. [snip] 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? :) You're listed as the soc-camera maintainer, so you should provide an answer to that question :-) Thanks for ar reminder;) I'll propose one, let's drop the omap1-camera driver (I've CC'ed the original author of the driver to this e-mail). Let's see what they reply, but I don't tgink Josh will want to wait that long. And until omap1 is in the mainline we cannot drop v4l2_clk. Thanks Guennadi -- 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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi Guennadi, On Tuesday 30 December 2014 09:36:31 Guennadi Liakhovetski wrote: Hi Laurent, First of all, sorry, I am currently on a holiday, so, replies are delayed, real work (reviewing or anything else) is impossible. Sure, no worries. Enjoy your holidays without thinking too much about soc- camera :-) On Tue, 30 Dec 2014, Laurent Pinchart wrote: On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote: On Fri, 26 Dec 2014, Laurent Pinchart wrote: 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: [snip] 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. Can you submit a patch ? That's the best way to find out if anyone objects. Can do, sure, once I am back home and find time for this. [snip] 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. On a side note, patches to enable CCF for PXA27x have been posted, they should be merged in v3.20. 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? :) You're listed as the soc-camera maintainer, so you should provide an answer to that question :-) Thanks for ar reminder;) I'll propose one, let's drop the omap1-camera driver (I've CC'ed the original author of the driver to this e-mail). Let's see what they reply, but I don't tgink Josh will want to wait that long. And until omap1 is in the mainline we cannot drop v4l2_clk. Agreed, this was more of a question for the next step. -- 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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi, Laurent
On 12/30/2014 8:15 AM, Laurent Pinchart wrote:
Hi Josh,
On Monday 29 December 2014 16:28:02 Josh Wu wrote:
On 12/26/2014 6:06 PM, Laurent Pinchart wrote:
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...@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 :-)
Sorry, I'm not clear about how to implement what you discussed here.
Do you mean, In the ov2640 driver:
1. need to remove the patch 4/5, add a master clock for sensor
No, the sensor has a clock input named xvclk, the ov2640 driver should thus
manage that clock. Patch 4/5 does the right thing.
However, I've just realized that it will cause
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi, Guennadi On 12/30/2014 4:36 PM, Guennadi Liakhovetski wrote: Hi Laurent, First of all, sorry, I am currently on a holiday, so, replies are delayed, real work (reviewing or anything else) is impossible. Thanks for your review in holiday. That's very helpful. On Tue, 30 Dec 2014, Laurent Pinchart wrote: Hi Guennadi, On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote: On Fri, 26 Dec 2014, Laurent Pinchart wrote: 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: [snip] 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. Can you submit a patch ? That's the best way to find out if anyone objects. Can do, sure, once I am back home and find time for this. [snip] 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? :) You're listed as the soc-camera maintainer, so you should provide an answer to that question :-) Thanks for ar reminder;) I'll propose one, let's drop the omap1-camera driver (I've CC'ed the original author of the driver to this e-mail). Let's see what they reply, but I don't tgink Josh will want to wait that long. Thanks. it's indeed true for me. And until omap1 is in the mainline we cannot drop v4l2_clk. So I think the better way right now for ov2640 driver is still request both the v4l2_clock: mclk, and the clock: xvclk in probe(). In that way, we can take our time to introduce other patches to merged these two clocks. Does it make sense? Best Regards, Josh Wu Thanks Guennadi -- 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 -- 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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
On Tue, 30 Dec 2014, Josh Wu wrote: [snip] And until omap1 is in the mainline we cannot drop v4l2_clk. s/until/as lonh as/ So I think the better way right now for ov2640 driver is still request both the v4l2_clock: mclk, and the clock: xvclk in probe(). In that way, we can take our time to introduce other patches to merged these two clocks. Does it make sense? How about this sequence, that we cat still try to get in on time for the next merge window: 1. stop using the clock name in v4l2_clk 2. add support for CCF to v4l2_clk, falling back to current behaviour if no clock is found 3. switch ov2640 to using xvclk Otherwise I would propose to add asynchronous probing support to ov2640 _without_ changing the clock name. Whether or not we changee clock's name isn't related directly to async support, since the driver already is using v4l2_clk with the standarf wrong name. So, I would consider these two changes separately - one is a new feature, another is a fix. The only question is in which order to apply them. In general fix-first is preferred, but since in this case the fix requires framework changes, we could go with feature-first. This also makes sense since features need to catch a merge window, whereas a fix can go in later too. So, if we don't manage the above 3-step plan, I would priposw the most primitive patch ro ov2640 just adding async support in line wuth other drivers and without changing the clock name at first. Thanks Guennadi -- 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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi, Laurent and Guennadi
On 12/26/2014 6:06 PM, 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...@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 :-)
Sorry, I'm not clear about how to implement what you discussed here.
Do you mean, In the ov2640 driver:
1. need to remove the patch 4/5, add a master clock for sensor
2. need to register a xvclk v4l2 clock which is a CCF clock. Or this
part can put in soc_camera.c
3. So in ov2640_probe(), need to call v4l2_clk_get(xvclk), which will do
a. Get CCF clock xvclk by call devm_clk_get(xvclk), and if
failed then return the error code.
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi Josh,
On Monday 29 December 2014 16:28:02 Josh Wu wrote:
On 12/26/2014 6:06 PM, Laurent Pinchart wrote:
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...@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 :-)
Sorry, I'm not clear about how to implement what you discussed here.
Do you mean, In the ov2640 driver:
1. need to remove the patch 4/5, add a master clock for sensor
No, the sensor has a clock input named xvclk, the ov2640 driver should
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi Guennadi, On Friday 26 December 2014 11:38:11 Guennadi Liakhovetski wrote: On Fri, 26 Dec 2014, Laurent Pinchart wrote: 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: [snip] 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. Can you submit a patch ? That's the best way to find out if anyone objects. [snip] 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? :) You're listed as the soc-camera maintainer, so you should provide an answer to that question :-) I'll propose one, let's drop the omap1-camera driver (I've CC'ed the original author of the driver to this e-mail). -- 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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi Josh,
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...@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).
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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
Hi Laurent,
On Fri, 26 Dec 2014, Laurent Pinchart wrote:
Hi Josh,
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...@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?..
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:
Re: [PATCH v4 2/5] media: ov2640: add async probe function
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...@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 :-)
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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
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...@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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
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...@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.
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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
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.
That is the motivation I want ov2640 be probed even without mclk.
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.
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.
@@ -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?
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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
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);
}
Ok, think about this: you check whether priv-clk is set on each
.s_power() call, which is already a bit awkward. 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?
@@ -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. And I'm not sure ov2640's registers
can be accessed with no running 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?
Thanks
Guennadi
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?
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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
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);
}
@@ -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? 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?
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
Re: [PATCH v4 2/5] media: ov2640: add async probe function
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);
}
@@ -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.
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?
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
