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

[v3][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.

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 (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       |  53 ++++++++
 drivers/media/i2c/soc_camera/ov2640.c              | 141 ++++++++++++++++++---
 drivers/media/platform/soc_camera/soc_camera.c     |   8 +-
 3 files changed, 182 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt

-- 
1.9.1

[v3][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>
---
v2->v3:
  1. check whether icd->control is NULL or not.

 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

[v3][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>
---
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);
 	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

[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

[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

[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 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@ideasonboard.com>

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".

> +	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

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

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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,

-- 
Regards,

Laurent Pinchart

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 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,

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>;
>> +				};
>> +			};
>> +		};
>> +	};