media: ov2640: add device tree support

Add device tree support for ov2640.

Cc [email protected]

Signed-off-by Josh Wu <[email protected]>

---
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(-)

(quoted diff) +0 -0

b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..e0bcf5b 100644

drivers/media/i2c/soc_camera/ov2640.c +30 -0

@@ -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);

drivers/media/i2c/soc_camera/ov2640.c +15 -6

+	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");

drivers/media/i2c/soc_camera/ov2640.c +12 -0

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

Hi Josh,

Thank you for the patch.

I only have three minor comments. After fixing them,

Acked-by Laurent Pinchart <[email protected]>

Quoted 73 lines

Add device tree support for ov2640.

Cc devicetree at vger.kernel.org

Signed-off-by Josh Wu <[email protected]>

---
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(-)

(quoted diff) +0 -0

b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..e0bcf5b 100644

drivers/media/i2c/soc_camera/ov2640.c +34 -0

@@ -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;

drivers/media/i2c/soc_camera/ov2640.c static struct v4l2_subdev_ops ov2640_subdev_ops = { · line 1089

Quoted 6 lines

drivers/media/i2c/soc_camera/ov2640.c +6 -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".

drivers/media/i2c/soc_camera/ov2640.c static struct v4l2_subdev_ops ov2640_subdev_ops = { · line 1097

Quoted 9 lines

drivers/media/i2c/soc_camera/ov2640.c +8 -0

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

Quoted 36 lines

drivers/media/i2c/soc_camera/ov2640.c +15 -6

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

drivers/media/i2c/soc_camera/ov2640.c static int ov2640_probe(struct i2c_client *client, · line 1142

Quoted 6 lines

drivers/media/i2c/soc_camera/ov2640.c +6 -0

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

Quoted 26 lines

drivers/media/i2c/soc_camera/ov2640.c +12 -0

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

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 [email protected]

Signed-off-by Josh Wu <[email protected]>

---
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(-)

(quoted diff) +0 -0

b/drivers/media/i2c/soc_camera/ov2640.c index e0bcf5b..055702c 100644

drivers/media/i2c/soc_camera/ov2640.c +22 -1

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

Add the document for ov2640 dt.

Cc [email protected]

Signed-off-by Josh Wu <[email protected]>

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

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/media/i2c/ov2640.txt new file mode 100644
index 0000000..958e120

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +6 -0

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

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +8 -0

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

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +18 -0

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

Hi Josh,

Thank you for the patch.

Quoted 30 lines · On Thursday 11 December 2014 15:35:39 Josh Wu wrote:

Add the document for ov2640 dt.

Cc devicetree at vger.kernel.org

Signed-off-by Josh Wu <[email protected]>

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

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/media/i2c/ov2640.txt new file mode 100644
index 0000000..958e120

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +4 -0

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

Documentation/devicetree/bindings/media/i2c/ov2640.txt line 10

Quoted 6 lines

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +6 -0

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

Quoted 9 lines

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +6 -0

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

Quoted 9 lines

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +8 -0

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

Documentation/devicetree/bindings/media/i2c/ov2640.txt

Quoted 10 lines

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +10 -0

+
+Example:
+
+	i2c1: i2c at f0018000 {
+		ov2640: camera at 0x30 {
+			compatible = "ovti,ov2640";
+			reg = <0x30>;
+
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_pck1 &pinctrl_ov2640_pwdn

&pinctrl_ov2640_resetb>;

Quoted 18 lines

Documentation/devicetree/bindings/media/i2c/ov2640.txt new file +18 -0

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