[PATCH v5 0/4] media: ov2640: add device tree support

[PATCH v5 0/4] media: ov2640: add device tree support

[Josh Wu]

This patch series add device tree support for ov2640. And also add
the document for the devicetree properties.
v4->v5:
  1. based on soc-camera v4l2-clk changes.
  2. remove the master_clk related (one commit), we only have one clk.

v3->v4:
  1. refined the dt document.
  2. Add Laurent's acked-by.

v2->v3:
  1. fix the gpiod_xxx api usage as we use reset pin as ACTIVE_LOW.
  2. update the devicetree binding document.

v1 -> v2:
  1.  modified the dt bindings according to Laurent's suggestion.
  2. add a fix patch for soc_camera. Otherwise the .reset() function
won't work.

Josh Wu (4):
  media: soc-camera: use icd->control instead of icd->pdev for reset()
  media: ov2640: add async probe function
  media: ov2640: add primary dt support
  media: ov2640: dt: add the device tree binding document

 .../devicetree/bindings/media/i2c/ov2640.txt       |  46 ++++++++
 drivers/media/i2c/soc_camera/ov2640.c              | 117 ++++++++++++++++++---
 drivers/media/platform/soc_camera/soc_camera.c     |   8 +-
 3 files changed, 152 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt

-- 
1.9.1

[PATCH v5 1/4] media: soc-camera: use icd->control instead of icd->pdev for reset()

[Josh Wu]

icd->control is the sub device dev, i.e. i2c device.
icd->pdev is the soc camera device's device.

To be consitent with power() function, we will call reset() with
icd->control as well.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/media/platform/soc_camera/soc_camera.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index f4be2a1..7e6b914 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -688,7 +688,8 @@ static int soc_camera_open(struct file *file)
 
 		/* The camera could have been already on, try to reset */
 		if (sdesc->subdev_desc.reset)
-			sdesc->subdev_desc.reset(icd->pdev);
+			if (icd->control)
+				sdesc->subdev_desc.reset(icd->control);
 
 		ret = soc_camera_add_device(icd);
 		if (ret < 0) {
@@ -1175,7 +1176,8 @@ static void scan_add_host(struct soc_camera_host *ici)
 
 			/* The camera could have been already on, try to reset */
 			if (ssdd->reset)
-				ssdd->reset(icd->pdev);
+				if (icd->control)
+					ssdd->reset(icd->control);
 
 			icd->parent = ici->v4l2_dev.dev;
 
@@ -1461,7 +1463,7 @@ static int soc_camera_async_bound(struct v4l2_async_notifier *notifier,
 				memcpy(&sdesc->subdev_desc, ssdd,
 				       sizeof(sdesc->subdev_desc));
 				if (ssdd->reset)
-					ssdd->reset(icd->pdev);
+					ssdd->reset(&client->dev);
 			}
 
 			icd->control = &client->dev;
-- 
1.9.1

[PATCH v5 2/4] media: ov2640: add async probe function

[Josh Wu]

In async probe, there is a case that ov2640 is probed before the
host device which provided 'mclk'.
To support this async probe, we will get 'mclk' at first in the probe(),
if failed it will return -EPROBE_DEFER. That will let ov2640 wait for
the host device probed.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

Changes in v5:
- don't change the ov2640_s_power() code.
- will get 'mclk' at the beginning of ov2640_probe().

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/media/i2c/soc_camera/ov2640.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 1fdce2f..057dd49 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -1068,6 +1068,10 @@ static int ov2640_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
+	priv->clk = v4l2_clk_get(&client->dev, "mclk");
+	if (IS_ERR(priv->clk))
+		return -EPROBE_DEFER;
+
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
 	v4l2_ctrl_handler_init(&priv->hdl, 2);
 	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
@@ -1075,24 +1079,28 @@ static int ov2640_probe(struct i2c_client *client,
 	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
 			V4L2_CID_HFLIP, 0, 1, 1, 0);
 	priv->subdev.ctrl_handler = &priv->hdl;
-	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;
+	if (priv->hdl.error) {
+		ret = priv->hdl.error;
+		goto err_clk;
 	}
 
 	ret = ov2640_video_probe(client);
 	if (ret) {
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
+		goto err_videoprobe;
 	} else {
 		dev_info(&adapter->dev, "OV2640 Probed\n");
 	}
 
+	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (ret < 0)
+		goto err_videoprobe;
+
+	return 0;
+
+err_videoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
+err_clk:
+	v4l2_clk_put(priv->clk);
 	return ret;
 }
 
@@ -1100,6 +1108,7 @@ static int ov2640_remove(struct i2c_client *client)
 {
 	struct ov2640_priv       *priv = to_ov2640(client);
 
+	v4l2_async_unregister_subdev(&priv->subdev);
 	v4l2_clk_put(priv->clk);
 	v4l2_device_unregister_subdev(&priv->subdev);
 	v4l2_ctrl_handler_free(&priv->hdl);
-- 
1.9.1

[PATCH v5 3/4] media: ov2640: add primary dt support

[Josh Wu]

Add device tree support for ov2640.
In device tree, user needs to provide the master clock (xvclk).
User can add the reset/pwdn pins if they have.

Cc: devicetree at vger.kernel.org
Signed-off-by: Josh Wu <josh.wu@atmel.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Changes in v5:
- change the 'mclk' to 'xvclk'. As v4l2-clk will handle the CCF xvclk
  and v4l2 master clock internally.

Changes in v4:
- modify the code comment.
- Add Laurent's acked by.

Changes in v3:
- fix gpiod usage.
- refine the ov2640_probe() function.

Changes in v2:
- use gpiod APIs.
- change the gpio pin's name according to datasheet.
- reduce the delay for .reset() function.

 drivers/media/i2c/soc_camera/ov2640.c | 90 ++++++++++++++++++++++++++++++++---
 1 file changed, 83 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 057dd49..c70e9e7 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -18,6 +18,8 @@
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/v4l2-mediabus.h>
 #include <linux/videodev2.h>
 
@@ -283,6 +285,10 @@ struct ov2640_priv {
 	u32	cfmt_code;
 	struct v4l2_clk			*clk;
 	const struct ov2640_win_size	*win;
+
+	struct soc_camera_subdev_desc	ssdd_dt;
+	struct gpio_desc *resetb_gpio;
+	struct gpio_desc *pwdn_gpio;
 };
 
 /*
@@ -1038,6 +1044,63 @@ static struct v4l2_subdev_ops ov2640_subdev_ops = {
 	.video	= &ov2640_subdev_video_ops,
 };
 
+/* OF probe functions */
+static int ov2640_hw_power(struct device *dev, int on)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ov2640_priv *priv = to_ov2640(client);
+
+	dev_dbg(&client->dev, "%s: %s the camera\n",
+			__func__, on ? "ENABLE" : "DISABLE");
+
+	if (priv->pwdn_gpio)
+		gpiod_direction_output(priv->pwdn_gpio, !on);
+
+	return 0;
+}
+
+static int ov2640_hw_reset(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ov2640_priv *priv = to_ov2640(client);
+
+	if (priv->resetb_gpio) {
+		/* Active the resetb pin to perform a reset pulse */
+		gpiod_direction_output(priv->resetb_gpio, 1);
+		usleep_range(3000, 5000);
+		gpiod_direction_output(priv->resetb_gpio, 0);
+	}
+
+	return 0;
+}
+
+static int ov2640_probe_dt(struct i2c_client *client,
+		struct ov2640_priv *priv)
+{
+	/* Request the reset GPIO deasserted */
+	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
+			GPIOD_OUT_LOW);
+	if (!priv->resetb_gpio)
+		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
+	else if (IS_ERR(priv->resetb_gpio))
+		return PTR_ERR(priv->resetb_gpio);
+
+	/* Request the power down GPIO asserted */
+	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
+			GPIOD_OUT_HIGH);
+	if (!priv->pwdn_gpio)
+		dev_dbg(&client->dev, "pwdn gpio is not assigned!\n");
+	else if (IS_ERR(priv->pwdn_gpio))
+		return PTR_ERR(priv->pwdn_gpio);
+
+	/* Initialize the soc_camera_subdev_desc */
+	priv->ssdd_dt.power = ov2640_hw_power;
+	priv->ssdd_dt.reset = ov2640_hw_reset;
+	client->dev.platform_data = &priv->ssdd_dt;
+
+	return 0;
+}
+
 /*
  * i2c_driver functions
  */
@@ -1049,12 +1112,6 @@ static int ov2640_probe(struct i2c_client *client,
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 	int			ret;
 
-	if (!ssdd) {
-		dev_err(&adapter->dev,
-			"OV2640: Missing platform_data for driver\n");
-		return -EINVAL;
-	}
-
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
 		dev_err(&adapter->dev,
 			"OV2640: I2C-Adapter doesn't support SMBUS\n");
@@ -1068,10 +1125,22 @@ static int ov2640_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
-	priv->clk = v4l2_clk_get(&client->dev, "mclk");
+	priv->clk = v4l2_clk_get(&client->dev, "xvclk");
 	if (IS_ERR(priv->clk))
 		return -EPROBE_DEFER;
 
+	if (!ssdd && !client->dev.of_node) {
+		dev_err(&client->dev, "Missing platform_data for driver\n");
+		ret = -EINVAL;
+		goto err_clk;
+	}
+
+	if (!ssdd) {
+		ret = ov2640_probe_dt(client, priv);
+		if (ret)
+			goto err_clk;
+	}
+
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
 	v4l2_ctrl_handler_init(&priv->hdl, 2);
 	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
@@ -1121,9 +1190,16 @@ static const struct i2c_device_id ov2640_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ov2640_id);
 
+static const struct of_device_id ov2640_of_match[] = {
+	{.compatible = "ovti,ov2640", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ov2640_of_match);
+
 static struct i2c_driver ov2640_i2c_driver = {
 	.driver = {
 		.name = "ov2640",
+		.of_match_table = of_match_ptr(ov2640_of_match),
 	},
 	.probe    = ov2640_probe,
 	.remove   = ov2640_remove,
-- 
1.9.1

[PATCH v5 4/4] media: ov2640: dt: add the device tree binding document

[Josh Wu]

Add the document for ov2640 dt.

Cc: devicetree at vger.kernel.org
Signed-off-by: Josh Wu <josh.wu@atmel.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---

Changes in v5: None
Changes in v4:
- remove aggsigned-clocks as it's general.
- refine the explation.

Changes in v3:
- fix incorrect description.
- Add assigned-clocks & assigned-clock-rates.
- resetb pin should be ACTIVE_LOW.

Changes in v2:
- change the compatible string to be consistent with verdor file.
- change the clock and pins' name.
- add missed pinctrl in example.

 .../devicetree/bindings/media/i2c/ov2640.txt       | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov2640.txt b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
new file mode 100644
index 0000000..c429b5b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
@@ -0,0 +1,46 @@
+* Omnivision OV2640 CMOS sensor
+
+The Omnivision OV2640 sensor support multiple resolutions output, such as
+CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
+output format.
+
+Required Properties:
+- compatible: should be "ovti,ov2640"
+- clocks: reference to the xvclk input clock.
+- clock-names: should be "xvclk".
+
+Optional Properties:
+- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
+- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
+
+The device node must contain one 'port' child node for its digital output
+video port, in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+	i2c1: i2c at f0018000 {
+		ov2640: camera at 0x30 {
+			compatible = "ovti,ov2640";
+			reg = <0x30>;
+
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_pck1 &pinctrl_ov2640_pwdn &pinctrl_ov2640_resetb>;
+
+			resetb-gpios = <&pioE 24 GPIO_ACTIVE_LOW>;
+			pwdn-gpios = <&pioE 29 GPIO_ACTIVE_HIGH>;
+
+			clocks = <&pck1>;
+			clock-names = "xvclk";
+
+			assigned-clocks = <&pck1>;
+			assigned-clock-rates = <25000000>;
+
+			port {
+				ov2640_0: endpoint {
+					remote-endpoint = <&isi_0>;
+					bus-width = <8>;
+				};
+			};
+		};
+	};
-- 
1.9.1

[PATCH v5 3/4] media: ov2640: add primary dt support

[Lad, Prabhakar]

Hi Josh,

Thanks for the patch.

On Tue, Feb 10, 2015 at 9:31 AM, Josh Wu <josh.wu@atmel.com> wrote:
[Snip]
>
> -       priv->clk = v4l2_clk_get(&client->dev, "mclk");
> +       priv->clk = v4l2_clk_get(&client->dev, "xvclk");

with this change don?t you need to update the board file using this driver/
the bridge driver ?

Regards,
--Prabhakar Lad

[PATCH v5 3/4] media: ov2640: add primary dt support

[Josh Wu]

Hi, Prabhakar Lad

On 2/17/2015 12:48 AM, Lad, Prabhakar wrote:
> Hi Josh,
>
> Thanks for the patch.
>
> On Tue, Feb 10, 2015 at 9:31 AM, Josh Wu <josh.wu@atmel.com> wrote:
> [Snip]
>> -       priv->clk = v4l2_clk_get(&client->dev, "mclk");
>> +       priv->clk = v4l2_clk_get(&client->dev, "xvclk");
> with this change don?t you need to update the board file using this driver/
> the bridge driver ?
I think no.

First, my patch should be on top of the following two patches, which 
changed the *v4l2_clk_get()* behavior:
[v3,1/2] V4L: remove clock name from v4l2_clk API
https://patchwork.linuxtv.org/patch/28108/
[v4,2/2] V4L: add CCF support to the v4l2_clk API
https://patchwork.linuxtv.org/patch/28111/

After applied above two patches, v4l2_clk_get() function is changed. The 
name "mclk" is refer to a CCF clock of the ov2640 device.
If not found such a "mclk" CCF clock, v4l2_clk_get() will try to get the 
internal register clock in soc_camera.c.
As the CCF dt clock is not support by ov2640 until I add DT support, 
that means current ov2640 driver will always not found the "mclk" CCF 
clock, and they will use internal clock.
So after I changed the name "mclk" to "xvclk", the default behavior will 
not change (still using internal clock registered by soc-camera.c).

Best Regards,
Josh Wu

>
> Regards,
> --Prabhakar Lad

[PATCH v5 3/4] media: ov2640: add primary dt support

[Lad, Prabhakar]

Hi Josh,

On Wed, Feb 25, 2015 at 3:54 AM, Josh Wu <josh.wu@atmel.com> wrote:
> Hi, Prabhakar Lad
>
>
> On 2/17/2015 12:48 AM, Lad, Prabhakar wrote:
>>
>> Hi Josh,
>>
>> Thanks for the patch.
>>
>> On Tue, Feb 10, 2015 at 9:31 AM, Josh Wu <josh.wu@atmel.com> wrote:
>> [Snip]
>>>
>>> -       priv->clk = v4l2_clk_get(&client->dev, "mclk");
>>> +       priv->clk = v4l2_clk_get(&client->dev, "xvclk");
>>
>> with this change don?t you need to update the board file using this
>> driver/
>> the bridge driver ?
>
> I think no.
>
> First, my patch should be on top of the following two patches, which changed
> the *v4l2_clk_get()* behavior:
> [v3,1/2] V4L: remove clock name from v4l2_clk API
> https://patchwork.linuxtv.org/patch/28108/
> [v4,2/2] V4L: add CCF support to the v4l2_clk API
> https://patchwork.linuxtv.org/patch/28111/
>
Thanks I missed the dependent patches.

Cheers,
--Prabhakar Lad

[PATCH v5 2/4] media: ov2640: add async probe function

[Guennadi Liakhovetski]

Hi Josh,

Thanks for a patch update. I think it looks good as a first step in your 
patch series, just a minor comment below:

On Tue, 10 Feb 2015, Josh Wu wrote:

> In async probe, there is a case that ov2640 is probed before the
> host device which provided 'mclk'.
> To support this async probe, we will get 'mclk' at first in the probe(),
> if failed it will return -EPROBE_DEFER. That will let ov2640 wait for
> the host device probed.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
> Changes in v5:
> - don't change the ov2640_s_power() code.
> - will get 'mclk' at the beginning of ov2640_probe().
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/media/i2c/soc_camera/ov2640.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
> index 1fdce2f..057dd49 100644
> --- a/drivers/media/i2c/soc_camera/ov2640.c
> +++ b/drivers/media/i2c/soc_camera/ov2640.c
> @@ -1068,6 +1068,10 @@ static int ov2640_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  	}
>  
> +	priv->clk = v4l2_clk_get(&client->dev, "mclk");
> +	if (IS_ERR(priv->clk))
> +		return -EPROBE_DEFER;
> +
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
>  	v4l2_ctrl_handler_init(&priv->hdl, 2);
>  	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
> @@ -1075,24 +1079,28 @@ static int ov2640_probe(struct i2c_client *client,
>  	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
>  			V4L2_CID_HFLIP, 0, 1, 1, 0);
>  	priv->subdev.ctrl_handler = &priv->hdl;
> -	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;
> +	if (priv->hdl.error) {
> +		ret = priv->hdl.error;
> +		goto err_clk;
>  	}
>  
>  	ret = ov2640_video_probe(client);
>  	if (ret) {
> -		v4l2_clk_put(priv->clk);
> -eclkget:
> -		v4l2_ctrl_handler_free(&priv->hdl);
> +		goto err_videoprobe;

Since you add a "goto" here, you don't need an "else" after it, and the 
"probed" success message should go down, so, just make it

	ret = ov2640_video_probe(client);
	if (ret < 0)
		goto err_videoprobe;

	ret = v4l2_async_register_subdev(&priv->subdev);
	if (ret < 0)
		goto err_videoprobe;

	dev_info(&adapter->dev, "OV2640 Probed\n");

	return 0;

err_...

Thanks
Guennadi

>  	} else {
>  		dev_info(&adapter->dev, "OV2640 Probed\n");
>  	}
>  
> +	ret = v4l2_async_register_subdev(&priv->subdev);
> +	if (ret < 0)
> +		goto err_videoprobe;
> +
> +	return 0;
> +
> +err_videoprobe:
> +	v4l2_ctrl_handler_free(&priv->hdl);
> +err_clk:
> +	v4l2_clk_put(priv->clk);
>  	return ret;
>  }
>  
> @@ -1100,6 +1108,7 @@ static int ov2640_remove(struct i2c_client *client)
>  {
>  	struct ov2640_priv       *priv = to_ov2640(client);
>  
> +	v4l2_async_unregister_subdev(&priv->subdev);
>  	v4l2_clk_put(priv->clk);
>  	v4l2_device_unregister_subdev(&priv->subdev);
>  	v4l2_ctrl_handler_free(&priv->hdl);
> -- 
> 1.9.1
> 

[PATCH v5 2/4] media: ov2640: add async probe function

[Josh Wu]

On 3/2/2015 5:06 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> Thanks for a patch update. I think it looks good as a first step in your
> patch series, just a minor comment below:
>
> On Tue, 10 Feb 2015, Josh Wu wrote:
>
>> In async probe, there is a case that ov2640 is probed before the
>> host device which provided 'mclk'.
>> To support this async probe, we will get 'mclk' at first in the probe(),
>> if failed it will return -EPROBE_DEFER. That will let ov2640 wait for
>> the host device probed.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>> Changes in v5:
>> - don't change the ov2640_s_power() code.
>> - will get 'mclk' at the beginning of ov2640_probe().
>>
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/media/i2c/soc_camera/ov2640.c | 29 +++++++++++++++++++----------
>>   1 file changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
>> index 1fdce2f..057dd49 100644
>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
>> @@ -1068,6 +1068,10 @@ static int ov2640_probe(struct i2c_client *client,
>>   		return -ENOMEM;
>>   	}
>>   
>> +	priv->clk = v4l2_clk_get(&client->dev, "mclk");
>> +	if (IS_ERR(priv->clk))
>> +		return -EPROBE_DEFER;
>> +
>>   	v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
>>   	v4l2_ctrl_handler_init(&priv->hdl, 2);
>>   	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
>> @@ -1075,24 +1079,28 @@ static int ov2640_probe(struct i2c_client *client,
>>   	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
>>   			V4L2_CID_HFLIP, 0, 1, 1, 0);
>>   	priv->subdev.ctrl_handler = &priv->hdl;
>> -	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;
>> +	if (priv->hdl.error) {
>> +		ret = priv->hdl.error;
>> +		goto err_clk;
>>   	}
>>   
>>   	ret = ov2640_video_probe(client);
>>   	if (ret) {
>> -		v4l2_clk_put(priv->clk);
>> -eclkget:
>> -		v4l2_ctrl_handler_free(&priv->hdl);
>> +		goto err_videoprobe;
> Since you add a "goto" here, you don't need an "else" after it, and the
> "probed" success message should go down, so, just make it
>
> 	ret = ov2640_video_probe(client);
> 	if (ret < 0)
> 		goto err_videoprobe;
>
> 	ret = v4l2_async_register_subdev(&priv->subdev);
> 	if (ret < 0)
> 		goto err_videoprobe;
>
> 	dev_info(&adapter->dev, "OV2640 Probed\n");
>
> 	return 0;
>
> err_...

Yes. This looks better.
I'll update and resend this patch. This change is independent and no 
need to resend the whole patch series.
Thanks.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>>   	} else {
>>   		dev_info(&adapter->dev, "OV2640 Probed\n");
>>   	}
>>   
>> +	ret = v4l2_async_register_subdev(&priv->subdev);
>> +	if (ret < 0)
>> +		goto err_videoprobe;
>> +
>> +	return 0;
>> +
>> +err_videoprobe:
>> +	v4l2_ctrl_handler_free(&priv->hdl);
>> +err_clk:
>> +	v4l2_clk_put(priv->clk);
>>   	return ret;
>>   }
>>   
>> @@ -1100,6 +1108,7 @@ static int ov2640_remove(struct i2c_client *client)
>>   {
>>   	struct ov2640_priv       *priv = to_ov2640(client);
>>   
>> +	v4l2_async_unregister_subdev(&priv->subdev);
>>   	v4l2_clk_put(priv->clk);
>>   	v4l2_device_unregister_subdev(&priv->subdev);
>>   	v4l2_ctrl_handler_free(&priv->hdl);
>> -- 
>> 1.9.1
>>

[PATCH v5 2/4][resend] media: ov2640: add async probe function

[Josh Wu]

In async probe, there is a case that ov2640 is probed before the
host device which provided 'mclk'.
To support this async probe, we will get 'mclk' at first in the probe(),
if failed it will return -EPROBE_DEFER. That will let ov2640 wait for
the host device probed.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

Changes in v5 resend:
- refine the error handling flow according to Guennadi's comment.

Changes in v5:
- don't change the ov2640_s_power() code.
- will get 'mclk' at the beginning of ov2640_probe().

 drivers/media/i2c/soc_camera/ov2640.c | 36 +++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 1fdce2f..16adbcc 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -1068,6 +1068,10 @@ static int ov2640_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
+	priv->clk = v4l2_clk_get(&client->dev, "mclk");
+	if (IS_ERR(priv->clk))
+		return -EPROBE_DEFER;
+
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
 	v4l2_ctrl_handler_init(&priv->hdl, 2);
 	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
@@ -1075,24 +1079,27 @@ static int ov2640_probe(struct i2c_client *client,
 	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
 			V4L2_CID_HFLIP, 0, 1, 1, 0);
 	priv->subdev.ctrl_handler = &priv->hdl;
-	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;
+	if (priv->hdl.error) {
+		ret = priv->hdl.error;
+		goto err_clk;
 	}
 
 	ret = ov2640_video_probe(client);
-	if (ret) {
-		v4l2_clk_put(priv->clk);
-eclkget:
-		v4l2_ctrl_handler_free(&priv->hdl);
-	} else {
-		dev_info(&adapter->dev, "OV2640 Probed\n");
-	}
+	if (ret < 0)
+		goto err_videoprobe;
 
+	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (ret < 0)
+		goto err_videoprobe;
+
+	dev_info(&adapter->dev, "OV2640 Probed\n");
+
+	return 0;
+
+err_videoprobe:
+	v4l2_ctrl_handler_free(&priv->hdl);
+err_clk:
+	v4l2_clk_put(priv->clk);
 	return ret;
 }
 
@@ -1100,6 +1107,7 @@ static int ov2640_remove(struct i2c_client *client)
 {
 	struct ov2640_priv       *priv = to_ov2640(client);
 
+	v4l2_async_unregister_subdev(&priv->subdev);
 	v4l2_clk_put(priv->clk);
 	v4l2_device_unregister_subdev(&priv->subdev);
 	v4l2_ctrl_handler_free(&priv->hdl);
-- 
1.9.1