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

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

[Josh Wu]

Add device tree support for ov2640.

Cc: devicetree@vger.kernel.org
Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
v2 -> v3:
  1. fix gpiod usage.
  2. refine the ov2640_probe() function.

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 | 87 ++++++++++++++++++++++++++++++++---
 1 file changed, 81 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 9ee910d..e0bcf5b 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,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)
+{
+	/* reset is not actived */
+	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);
+
+	/* Power down actived */
+	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
  */
@@ -1058,12 +1121,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 +1134,17 @@ static int ov2640_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
+	if (!ssdd && !client->dev.of_node) {
+		dev_err(&client->dev, "Missing platform_data for driver\n");
+		return  -EINVAL;
+	}
+
+	if (!ssdd && client->dev.of_node) {
+		ret = ov2640_probe_dt(client, priv);
+		if (ret)
+			return ret;
+	}
+
 	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 +1191,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

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

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

I only have three minor comments. After fixing them,

Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

On Thursday 11 December 2014 15:35:37 Josh Wu wrote:
> Add device tree support for ov2640.
> 
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Josh Wu <josh.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
> v2 -> v3:
>   1. fix gpiod usage.
>   2. refine the ov2640_probe() function.
> 
> 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 | 87 +++++++++++++++++++++++++++++---
>  1 file changed, 81 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..e0bcf5b 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,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)
> +{
> +	/* reset is not actived */

Do you mean that you configure the reset signal as inactive ? I would state 
that as "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);
> +
> +	/* Power down actived */

And "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
>   */
> @@ -1058,12 +1121,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 +1134,17 @@ static int ov2640_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  	}
> 
> +	if (!ssdd && !client->dev.of_node) {
> +		dev_err(&client->dev, "Missing platform_data for driver\n");
> +		return  -EINVAL;
> +	}
> +
> +	if (!ssdd && client->dev.of_node) {

You can just test for !ssdd as you've tested client->dev.of_node above 
already.

> +		ret = ov2640_probe_dt(client, priv);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	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 +1191,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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Wu]

Hi, Laurent

On 12/11/2014 4:10 PM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> I only have three minor comments. After fixing them,
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks for the review.
>
> On Thursday 11 December 2014 15:35:37 Josh Wu wrote:
>> Add device tree support for ov2640.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> v2 -> v3:
>>    1. fix gpiod usage.
>>    2. refine the ov2640_probe() function.
>>
>> 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 | 87 +++++++++++++++++++++++++++++---
>>   1 file changed, 81 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
>> b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..e0bcf5b 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,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)
>> +{
>> +	/* reset is not actived */
> Do you mean that you configure the reset signal as inactive ? I would state
> that as "Request the reset GPIO deasserted".

yes, exactly. I'll take your sentence. thanks.
>
>> +	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);
>> +
>> +	/* Power down actived */
> And "Request the power down GPIO asserted".
ditto.

>
>> +	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
>>    */
>> @@ -1058,12 +1121,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 +1134,17 @@ static int ov2640_probe(struct i2c_client *client,
>>   		return -ENOMEM;
>>   	}
>>
>> +	if (!ssdd && !client->dev.of_node) {
>> +		dev_err(&client->dev, "Missing platform_data for driver\n");
>> +		return  -EINVAL;
>> +	}
>> +
>> +	if (!ssdd && client->dev.of_node) {
> You can just test for !ssdd as you've tested client->dev.of_node above
> already.

Right, thanks.

Best Regards,
Josh Wu
>
>> +		ret = ov2640_probe_dt(client, priv);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	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 +1191,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,

[v3][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@vger.kernel.org
Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
v2 -> v3:
  1. should return PTR_ERR().

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 e0bcf5b..055702c 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 */
@@ -1145,6 +1162,10 @@ static int ov2640_probe(struct i2c_client *client,
 			return ret;
 	}
 
+	priv->master_clk = devm_clk_get(&client->dev, "xvclk");
+	if (IS_ERR(priv->master_clk))
+		return PTR_ERR(priv->master_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,
-- 
1.9.1

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

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Thursday 11 December 2014 15:35:38 Josh Wu wrote:
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Josh Wu <josh.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> ---
> v2 -> v3:
>   1. should return PTR_ERR().
> 
> 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 e0bcf5b..055702c 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 */
> @@ -1145,6 +1162,10 @@ static int ov2640_probe(struct i2c_client *client,
>  			return ret;
>  	}
> 
> +	priv->master_clk = devm_clk_get(&client->dev, "xvclk");
> +	if (IS_ERR(priv->master_clk))
> +		return PTR_ERR(priv->master_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,

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Josh Wu]

Add the document for ov2640 dt.

Cc: devicetree@vger.kernel.org
Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
v2 -> v3:
  1. fix incorrect description.
  2. Add assigned-clocks & assigned-clock-rates.
  3. resetb pin should be ACTIVE_LOW.

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       | 53 ++++++++++++++++++++++
 1 file changed, 53 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..958e120
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
@@ -0,0 +1,53 @@
+* 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 to the xvclk input clock. It can be an external fixed
+          clock or a programmable clock from SoC.
+- clock-names: Must be "xvclk".
+- assigned-clocks: reference to the above 'clocks' property.
+- assigned-clock-rates: reference to the clock frequency of xvclk. Typical
+                        value is 25Mhz (25000000).
+                        This clock should only have single user. Specifying
+                        Conflicting rate configuration in multiple consumer
+                        nodes for a shared clock is forbidden.
+
+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@f0018000 {
+		ov2640: camera@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

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

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Thursday 11 December 2014 15:35:39 Josh Wu wrote:
> Add the document for ov2640 dt.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> v2 -> v3:
>   1. fix incorrect description.
>   2. Add assigned-clocks & assigned-clock-rates.
>   3. resetb pin should be ACTIVE_LOW.
> 
> 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       | 53 +++++++++++++++++++
>  1 file changed, 53 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..958e120
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
> @@ -0,0 +1,53 @@
> +* 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 to the xvclk input clock. It can be an external fixed
> +          clock or a programmable clock from SoC.

It could also be a variable clock provided by something else. I would just 
drop the second sentence.

> +- clock-names: Must be "xvclk".
> +- assigned-clocks: reference to the above 'clocks' property.
> +- assigned-clock-rates: reference to the clock frequency of xvclk. Typical
> +                        value is 25Mhz (25000000).
> +                        This clock should only have single user. Specifying
> +                        Conflicting rate configuration in multiple
> consumer
> +                        nodes for a shared clock is forbidden.

Those two properties are optional. I'm not sure they should even be mentioned 
in the ov2640 bindings. For one thing they're not needed if the clock doesn't 
need to be forced to a specific frequency, and the clock rate could also be 
configured through other means depending on the platform.

> +
> +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@f0018000 {
> +		ov2640: camera@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>;
> +				};
> +			};
> +		};
> +	};

-- 
Regards,

Laurent Pinchart

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

[Josh Wu]

Hi, Laurent

On 12/12/2014 10:17 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Thursday 11 December 2014 15:35:39 Josh Wu wrote:
>> Add the document for ov2640 dt.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> v2 -> v3:
>>    1. fix incorrect description.
>>    2. Add assigned-clocks & assigned-clock-rates.
>>    3. resetb pin should be ACTIVE_LOW.
>>
>> 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       | 53 +++++++++++++++++++
>>   1 file changed, 53 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..958e120
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
>> @@ -0,0 +1,53 @@
>> +* 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 to the xvclk input clock. It can be an external fixed
>> +          clock or a programmable clock from SoC.
> It could also be a variable clock provided by something else. I would just
> drop the second sentence.

I mention this since these are the cases in at91 platform.
Sure. I'll drop it.
>
>> +- clock-names: Must be "xvclk".
>> +- assigned-clocks: reference to the above 'clocks' property.
>> +- assigned-clock-rates: reference to the clock frequency of xvclk. Typical
>> +                        value is 25Mhz (25000000).
>> +                        This clock should only have single user. Specifying
>> +                        Conflicting rate configuration in multiple
>> consumer
>> +                        nodes for a shared clock is forbidden.
> Those two properties are optional. I'm not sure they should even be mentioned
> in the ov2640 bindings. For one thing they're not needed if the clock doesn't
> need to be forced to a specific frequency, and the clock rate could also be
> configured through other means depending on the platform.

hmm, if the clock is fixed then this 'assigned-clock-rates' is not needed.
And I only find one binding document mentioned the assigned-clocks. It 
looks like a general property for all clocks. We don't need mention it 
repeatedly.
So I'd like to drop this two properties in next version.

Thanks and Best Regards,
Josh Wu
>
>> +
>> +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@f0018000 {
>> +		ov2640: camera@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>;
>> +				};
>> +			};
>> +		};
>> +	};