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

[PATCH 0/5] 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.

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 (5):
  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: add a master clock for sensor
  media: ov2640: dt: add the device tree binding document

 .../devicetree/bindings/media/i2c/ov2640.txt       |  44 +++++++
 drivers/media/i2c/soc_camera/ov2640.c              | 140 ++++++++++++++++++---
 drivers/media/platform/soc_camera/soc_camera.c     |  10 +-
 3 files changed, 173 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt

-- 
1.9.1

[PATCH 1/5] 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>
---
 drivers/media/platform/soc_camera/soc_camera.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index f4be2a1..1a3fcb5 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -688,7 +688,7 @@ 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);
+			sdesc->subdev_desc.reset(icd->control);
 
 		ret = soc_camera_add_device(icd);
 		if (ret < 0) {
@@ -1174,8 +1174,10 @@ static void scan_add_host(struct soc_camera_host *ici)
 			struct soc_camera_subdev_desc *ssdd = &sdesc->subdev_desc;
 
 			/* The camera could have been already on, try to reset */
-			if (ssdd->reset)
-				ssdd->reset(icd->pdev);
+			if (ssdd->reset) {
+				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 2/5] media: ov2640: add async probe function

[Josh Wu]

To support async probe for ov2640, we need remove the code to get 'mclk'
in ov2640_probe() function. oterwise, if soc_camera host is not probed
in the moment, then we will fail to get 'mclk' and quit the ov2640_probe()
function.

So in this patch, we move such 'mclk' getting code to ov2640_s_power()
function. That make ov2640 survive, as we can pass a NULL (priv-clk) to
soc_camera_set_power() function.

And if soc_camera host is probed, the when ov2640_s_power() is called,
then we can get the 'mclk' and that make us enable/disable soc_camera
host's clock as well.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
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);
 	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

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

[Josh Wu]

Add device tree support for ov2640.

Cc: devicetree at vger.kernel.org
Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
v1 -> v2:
  1. use gpiod APIs.
  2. change the gpio pin's name according to datasheet.
  3. reduce the delay for .reset() function.

 drivers/media/i2c/soc_camera/ov2640.c | 86 ++++++++++++++++++++++++++++++++---
 1 file changed, 80 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 9ee910d..2a57979 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;
 };
 
 /*
@@ -1047,6 +1053,61 @@ 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 && !IS_ERR(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 enabled, give a reset impulse */
+	if (priv->resetb_gpio && !IS_ERR(priv->resetb_gpio)) {
+		gpiod_direction_output(priv->resetb_gpio, 0);
+		usleep_range(3000, 5000);
+		gpiod_direction_output(priv->resetb_gpio, 1);
+	}
+
+	return 0;
+}
+
+static int ov2640_probe_dt(struct i2c_client *client,
+		struct ov2640_priv *priv)
+{
+	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
+			GPIOD_OUT_HIGH);
+	if (!priv->resetb_gpio)
+		dev_warn(&client->dev, "resetb gpio not found!\n");
+	else if (IS_ERR(priv->resetb_gpio))
+		return -EINVAL;
+
+	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
+			GPIOD_OUT_HIGH);
+	if (!priv->pwdn_gpio)
+		dev_warn(&client->dev, "pwdn gpio not found!\n");
+	else if (IS_ERR(priv->pwdn_gpio))
+		return -EINVAL;
+
+	/* 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
  */
@@ -1058,12 +1119,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");
@@ -1077,6 +1132,18 @@ static int ov2640_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
+	if (!ssdd) {
+		if (client->dev.of_node) {
+			ret = ov2640_probe_dt(client, priv);
+			if (ret)
+				return ret;
+		} else {
+			dev_err(&client->dev,
+				"Missing platform_data for driver\n");
+			return  -EINVAL;
+		}
+	}
+
 	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,
@@ -1123,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 3/5] media: ov2640: add primary dt support

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Monday 08 December 2014 19:29:05 Josh Wu wrote:
> Add device tree support for ov2640.
> 
> Cc: devicetree at vger.kernel.org
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> v1 -> v2:
>   1. use gpiod APIs.
>   2. change the gpio pin's name according to datasheet.
>   3. reduce the delay for .reset() function.
> 
>  drivers/media/i2c/soc_camera/ov2640.c | 86 +++++++++++++++++++++++++++++---
>  1 file changed, 80 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..2a57979 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;
>  };
> 
>  /*
> @@ -1047,6 +1053,61 @@ 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 && !IS_ERR(priv->pwdn_gpio))

No need to test for IS_ERR, as the probe function would have failed in that 
case.

> +		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 enabled, give a reset impulse */
> +	if (priv->resetb_gpio && !IS_ERR(priv->resetb_gpio)) {

Same here.

> +		gpiod_direction_output(priv->resetb_gpio, 0);

Given that your DT should specify the active low GPIO flag, and that the gpiod 
API inverts the value in that case, you should set the value to 1 here.

> +		usleep_range(3000, 5000);
> +		gpiod_direction_output(priv->resetb_gpio, 1);

And to 0 here.

> +	}
> +
> +	return 0;
> +}
> +
> +static int ov2640_probe_dt(struct i2c_client *client,
> +		struct ov2640_priv *priv)
> +{
> +	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> +			GPIOD_OUT_HIGH);
> +	if (!priv->resetb_gpio)
> +		dev_warn(&client->dev, "resetb gpio not found!\n");

No need to warn here, it's perfectly fine if the reset signal isn't connected 
to a GPIO.

> +	else if (IS_ERR(priv->resetb_gpio))
> +		return -EINVAL;
> +
> +	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
> +			GPIOD_OUT_HIGH);
> +	if (!priv->pwdn_gpio)
> +		dev_warn(&client->dev, "pwdn gpio not found!\n");

Same here.

> +	else if (IS_ERR(priv->pwdn_gpio))
> +		return -EINVAL;
> +
> +	/* 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
>   */
> @@ -1058,12 +1119,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");
> @@ -1077,6 +1132,18 @@ static int ov2640_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  	}
> 
> +	if (!ssdd) {
> +		if (client->dev.of_node) {
> +			ret = ov2640_probe_dt(client, priv);
> +			if (ret)
> +				return ret;
> +		} else {
> +			dev_err(&client->dev,
> +				"Missing platform_data for driver\n");
> +			return  -EINVAL;
> +		}

I would test for !client->dev.of_node and return the error, you could then get 
rid of the else and lower the indentation level for the call to 
ov2640_probe_dt().

> +	}
> +
>  	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,
> @@ -1123,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,

-- 
Regards,

Laurent Pinchart

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

[Josh Wu]

Hi, Laurent

On 12/9/2014 2:39 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Monday 08 December 2014 19:29:05 Josh Wu wrote:
>> Add device tree support for ov2640.
>>
>> Cc: devicetree at vger.kernel.org
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> v1 -> v2:
>>    1. use gpiod APIs.
>>    2. change the gpio pin's name according to datasheet.
>>    3. reduce the delay for .reset() function.
>>
>>   drivers/media/i2c/soc_camera/ov2640.c | 86 +++++++++++++++++++++++++++++---
>>   1 file changed, 80 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
>> b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..2a57979 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;
>>   };
>>
>>   /*
>> @@ -1047,6 +1053,61 @@ 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 && !IS_ERR(priv->pwdn_gpio))
> No need to test for IS_ERR, as the probe function would have failed in that
> case.
right. I'll change it.

>
>> +		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 enabled, give a reset impulse */
>> +	if (priv->resetb_gpio && !IS_ERR(priv->resetb_gpio)) {
> Same here.
ditto.

>
>> +		gpiod_direction_output(priv->resetb_gpio, 0);
> Given that your DT should specify the active low GPIO flag, and that the gpiod
> API inverts the value in that case, you should set the value to 1 here.

Thanks for the information. I'll fix it.
>
>> +		usleep_range(3000, 5000);
>> +		gpiod_direction_output(priv->resetb_gpio, 1);
> And to 0 here.
yes.

>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ov2640_probe_dt(struct i2c_client *client,
>> +		struct ov2640_priv *priv)
>> +{
>> +	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
>> +			GPIOD_OUT_HIGH);
>> +	if (!priv->resetb_gpio)
>> +		dev_warn(&client->dev, "resetb gpio not found!\n");
> No need to warn here, it's perfectly fine if the reset signal isn't connected
> to a GPIO.
I want to print some information if no GPIO is assigned. So I'd like use 
dev_dbg() here.
What do you feel?

>
>> +	else if (IS_ERR(priv->resetb_gpio))
>> +		return -EINVAL;
>> +
>> +	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
>> +			GPIOD_OUT_HIGH);
>> +	if (!priv->pwdn_gpio)
>> +		dev_warn(&client->dev, "pwdn gpio not found!\n");
> Same here.
ditto.
>
>> +	else if (IS_ERR(priv->pwdn_gpio))
>> +		return -EINVAL;
>> +
>> +	/* 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
>>    */
>> @@ -1058,12 +1119,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");
>> @@ -1077,6 +1132,18 @@ static int ov2640_probe(struct i2c_client *client,
>>   		return -ENOMEM;
>>   	}
>>
>> +	if (!ssdd) {
>> +		if (client->dev.of_node) {
>> +			ret = ov2640_probe_dt(client, priv);
>> +			if (ret)
>> +				return ret;
>> +		} else {
>> +			dev_err(&client->dev,
>> +				"Missing platform_data for driver\n");
>> +			return  -EINVAL;
>> +		}
> I would test for !client->dev.of_node and return the error, you could then get
> rid of the else and lower the indentation level for the call to
> ov2640_probe_dt().
Okay. I'll change it in next version.

Best Regards,
Josh Wu

>
>> +	}
>> +
>>   	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,
>> @@ -1123,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,

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

[Laurent Pinchart]

Hi Josh,

On Tuesday 09 December 2014 11:10:01 Josh Wu wrote:
> On 12/9/2014 2:39 AM, Laurent Pinchart wrote:
> > Hi Josh,
> > 
> > Thank you for the patch.
> > 
> > On Monday 08 December 2014 19:29:05 Josh Wu wrote:
> >> Add device tree support for ov2640.
> >> 
> >> Cc: devicetree at vger.kernel.org
> >> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> >> ---
> >> 
> >> v1 -> v2:
> >>    1. use gpiod APIs.
> >>    2. change the gpio pin's name according to datasheet.
> >>    3. reduce the delay for .reset() function.
> >>   
> >>   drivers/media/i2c/soc_camera/ov2640.c | 86 +++++++++++++++++++++++++---
> >>   1 file changed, 80 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> >> b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..2a57979 100644
> >> --- a/drivers/media/i2c/soc_camera/ov2640.c
> >> +++ b/drivers/media/i2c/soc_camera/ov2640.c

[snip]

> >> +static int ov2640_probe_dt(struct i2c_client *client,
> >> +		struct ov2640_priv *priv)
> >> +{
> >> +	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> >> +			GPIOD_OUT_HIGH);
> >> +	if (!priv->resetb_gpio)
> >> +		dev_warn(&client->dev, "resetb gpio not found!\n");
> > 
> > No need to warn here, it's perfectly fine if the reset signal isn't
> > connected to a GPIO.
> 
> I want to print some information if no GPIO is assigned. So I'd like use
> dev_dbg() here.
> What do you feel?

If you want to print a message in that case dev_dbg is the most appropriate 
log level. I would skip it completely, but that's up to you.

> >> +	else if (IS_ERR(priv->resetb_gpio))
> >> +		return -EINVAL;
> >> +
> >> +	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
> >> +			GPIOD_OUT_HIGH);
> >> +	if (!priv->pwdn_gpio)
> >> +		dev_warn(&client->dev, "pwdn gpio not found!\n");
> > 
> > Same here.
> 
> ditto.
> 
> >> +	else if (IS_ERR(priv->pwdn_gpio))
> >> +		return -EINVAL;
> >> +
> >> +	/* 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
> >>    */

-- 
Regards,

Laurent Pinchart

[PATCH 4/5] media: ov2640: add a master clock for sensor

[Josh Wu]

The master clock (xvclk) is mandatory. It's a common clock framework clock.
It can make sensor output a pixel clock to the camera interface.

Cc: devicetree at vger.kernel.org
Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
v1 -> v2:
  1. change the clock's name.
  2. Make the clock is mandatory.

 drivers/media/i2c/soc_camera/ov2640.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 2a57979..7cb61e2 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -13,6 +13,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/i2c.h>
@@ -31,6 +32,7 @@
 
 #define VAL_SET(x, mask, rshift, lshift)  \
 		((((x) >> rshift) & mask) << lshift)
+
 /*
  * DSP registers
  * register offset for BANK_SEL == BANK_SEL_DSP
@@ -284,6 +286,7 @@ struct ov2640_priv {
 	struct v4l2_ctrl_handler	hdl;
 	u32	cfmt_code;
 	struct v4l2_clk			*clk;
+	struct clk			*master_clk;
 	const struct ov2640_win_size	*win;
 
 	struct soc_camera_subdev_desc	ssdd_dt;
@@ -746,6 +749,7 @@ static int ov2640_s_power(struct v4l2_subdev *sd, int on)
 	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
 	struct ov2640_priv *priv = to_ov2640(client);
 	struct v4l2_clk *clk;
+	int ret;
 
 	if (!priv->clk) {
 		clk = v4l2_clk_get(&client->dev, "mclk");
@@ -755,7 +759,20 @@ static int ov2640_s_power(struct v4l2_subdev *sd, int on)
 			priv->clk = clk;
 	}
 
-	return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
+	if (on) {
+		ret = clk_prepare_enable(priv->master_clk);
+		if (ret)
+			return ret;
+	} else {
+		clk_disable_unprepare(priv->master_clk);
+	}
+
+	ret = soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
+
+	if (ret && on)
+		clk_disable_unprepare(priv->master_clk);
+
+	return ret;
 }
 
 /* Select the nearest higher resolution for capture */
@@ -1144,6 +1161,10 @@ static int ov2640_probe(struct i2c_client *client,
 		}
 	}
 
+	priv->master_clk = devm_clk_get(&client->dev, "xvclk");
+	if (IS_ERR(priv->master_clk))
+		return -EINVAL;
+
 	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,
-- 
1.9.1

[PATCH 4/5] media: ov2640: add a master clock for sensor

[Fabio Estevam]

On Mon, Dec 8, 2014 at 9:29 AM, Josh Wu <josh.wu@atmel.com> wrote:

> +       priv->master_clk = devm_clk_get(&client->dev, "xvclk");
> +       if (IS_ERR(priv->master_clk))
> +               return -EINVAL;

You should return PTR_ERR(priv->master_clk) instead.

[PATCH 4/5] media: ov2640: add a master clock for sensor

[Josh Wu]

Hi, Fabio

On 12/8/2014 11:10 PM, Fabio Estevam wrote:
> On Mon, Dec 8, 2014 at 9:29 AM, Josh Wu <josh.wu@atmel.com> wrote:
>
>> +       priv->master_clk = devm_clk_get(&client->dev, "xvclk");
>> +       if (IS_ERR(priv->master_clk))
>> +               return -EINVAL;
> You should return PTR_ERR(priv->master_clk) instead.
sure. I'll fix it in next version. Thanks.

Best Regards,
Josh Wu

[PATCH 5/5] 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>
---
v1 -> v2:
  1. change the compatible string to be consistent with verdor file.
  2. change the clock and pins' name.
  3. add missed pinctrl in example.

 .../devicetree/bindings/media/i2c/ov2640.txt       | 44 ++++++++++++++++++++++
 1 file changed, 44 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..15be3cb
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
@@ -0,0 +1,44 @@
+* 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: Must be "ovti,ov2640"
+- clocks: reference master clock, if using external fixed clock, you
+          no need to have such property.
+- clock-names: Must be "xvclk", it means the master clock for ov2640.
+
+Optional Properties:
+- resetb-gpios: reset pin
+- pwdn-gpios: power down pin
+
+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_reset>;
+
+			resetb-gpios = <&pioE 24 GPIO_ACTIVE_HIGH>;
+			pwdn-gpios = <&pioE 29 GPIO_ACTIVE_HIGH>;
+
+			clocks = <&pck1>;
+			clock-names = "xvclk";
+
+			port {
+				ov2640_0: endpoint {
+					remote-endpoint = <&isi_0>;
+					bus-width = <8>;
+				};
+			};
+		};
+	};
-- 
1.9.1

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

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Monday 08 December 2014 19:29:07 Josh Wu wrote:
> Add the document for ov2640 dt.
> 
> Cc: devicetree at vger.kernel.org
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> v1 -> v2:
>   1. change the compatible string to be consistent with verdor file.

That's nice, but you still need to send a patch to add the ovti vendor prefix 
to Documentation/devicetree/bindings/vendor-prefixes.txt. It's not there yet.

>   2. change the clock and pins' name.
>   3. add missed pinctrl in example.
> 
>  .../devicetree/bindings/media/i2c/ov2640.txt       | 44 +++++++++++++++++++
>  1 file changed, 44 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..15be3cb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
> @@ -0,0 +1,44 @@
> +* 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: Must be "ovti,ov2640"
> +- clocks: reference master clock, if using external fixed clock, you
> +          no need to have such property.

That's not true anymore, the clocks property is mandatory in all cases. Just 
describe it as

- clocks: reference to the xvclk input clock.

> +- clock-names: Must be "xvclk", it means the master clock for ov2640.

I would drop "it means the master clock for ov2640".

> +Optional Properties:
> +- resetb-gpios: reset pin

- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.

> +- pwdn-gpios: power down pin

- 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_reset>;
> +
> +			resetb-gpios = <&pioE 24 GPIO_ACTIVE_HIGH>;
> +			pwdn-gpios = <&pioE 29 GPIO_ACTIVE_HIGH>;
> +
> +			clocks = <&pck1>;
> +			clock-names = "xvclk";
> +
> +			port {
> +				ov2640_0: endpoint {
> +					remote-endpoint = <&isi_0>;
> +					bus-width = <8>;
> +				};
> +			};
> +		};
> +	};

-- 
Regards,

Laurent Pinchart

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

[Josh Wu]

Hi, Laurent

On 12/9/2014 2:59 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Monday 08 December 2014 19:29:07 Josh Wu wrote:
>> Add the document for ov2640 dt.
>>
>> Cc: devicetree at vger.kernel.org
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> v1 -> v2:
>>    1. change the compatible string to be consistent with verdor file.
> That's nice, but you still need to send a patch to add the ovti vendor prefix
> to Documentation/devicetree/bindings/vendor-prefixes.txt. It's not there yet.
As Fabio already send a patch to fix the vendor file.
See URL: http://patchwork.ozlabs.org/patch/416685/
I think it will go to mainline soon.

>
>>    2. change the clock and pins' name.
>>    3. add missed pinctrl in example.
>>
>>   .../devicetree/bindings/media/i2c/ov2640.txt       | 44 +++++++++++++++++++
>>   1 file changed, 44 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..15be3cb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
>> @@ -0,0 +1,44 @@
>> +* 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: Must be "ovti,ov2640"
>> +- clocks: reference master clock, if using external fixed clock, you
>> +          no need to have such property.
> That's not true anymore, the clocks property is mandatory in all cases. Just
> describe it as
>
> - clocks: reference to the xvclk input clock.
>
>> +- clock-names: Must be "xvclk", it means the master clock for ov2640.
> I would drop "it means the master clock for ov2640".
>
>> +Optional Properties:
>> +- resetb-gpios: reset pin
> - resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
>
>> +- pwdn-gpios: power down pin
> - pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.

I'll fix all above that you mentioned in next version.
Thank a lot for the review.

Best Regards,
Josh Wu
>
>> +
>> +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_reset>;
>> +
>> +			resetb-gpios = <&pioE 24 GPIO_ACTIVE_HIGH>;
>> +			pwdn-gpios = <&pioE 29 GPIO_ACTIVE_HIGH>;
>> +
>> +			clocks = <&pck1>;
>> +			clock-names = "xvclk";
>> +
>> +			port {
>> +				ov2640_0: endpoint {
>> +					remote-endpoint = <&isi_0>;
>> +					bus-width = <8>;
>> +				};
>> +			};
>> +		};
>> +	};