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

[PATCH v4 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>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v3 -> v4:
  1.modify the code comment.
  2. Add Laurent's acked by.

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..4c2868c 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)
+{
+	/* Request the reset GPIO deasserted */
+	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
+			GPIOD_OUT_LOW);
+	if (!priv->resetb_gpio)
+		dev_dbg(&client->dev, "resetb gpio is not assigned!\n");
+	else if (IS_ERR(priv->resetb_gpio))
+		return PTR_ERR(priv->resetb_gpio);
+
+	/* Request the power down GPIO asserted */
+	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
+			GPIOD_OUT_HIGH);
+	if (!priv->pwdn_gpio)
+		dev_dbg(&client->dev, "pwdn gpio is not assigned!\n");
+	else if (IS_ERR(priv->pwdn_gpio))
+		return PTR_ERR(priv->pwdn_gpio);
+
+	/* Initialize the soc_camera_subdev_desc */
+	priv->ssdd_dt.power = ov2640_hw_power;
+	priv->ssdd_dt.reset = ov2640_hw_reset;
+	client->dev.platform_data = &priv->ssdd_dt;
+
+	return 0;
+}
+
 /*
  * i2c_driver functions
  */
@@ -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) {
+		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

[PATCH v4 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>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v3 -> v4:
  1. Add Laurent's acked by.

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 4c2868c..1d68bcf 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

[PATCH v4 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>
---
v3 -> v4:
  1. remove aggsigned-clocks as it's general.
  2. refine the explation.

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       | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov2640.txt b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
new file mode 100644
index 0000000..de11ebb
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
@@ -0,0 +1,46 @@
+* Omnivision ov2640 CMOS sensor
+
+The Omnivision OV2640 sensor support multiple resolutions output, such as
+CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
+output format.
+
+Required Properties:
+- compatible: Must be "ovti,ov2640"
+- clocks: reference to the xvclk input clock.
+- clock-names: Must be "xvclk".
+
+Optional Properties:
+- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
+- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
+
+The device node must contain one 'port' child node for its digital output
+video port, in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+	i2c1: i2c@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: [PATCH v4 5/5] media: ov2640: dt: add the device tree binding document

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Thursday 18 December 2014 10:27:26 Josh Wu wrote:
> Add the document for ov2640 dt.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Josh Wu <josh.wu@atmel.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3 -> v4:
>   1. remove aggsigned-clocks as it's general.
>   2. refine the explation.
> 
> 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       | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2640.txt
> b/Documentation/devicetree/bindings/media/i2c/ov2640.txt new file mode
> 100644
> index 0000000..de11ebb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
> @@ -0,0 +1,46 @@
> +* Omnivision ov2640 CMOS sensor
> +
> +The Omnivision OV2640 sensor support multiple resolutions output, such as
> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
> +output format.
> +
> +Required Properties:
> +- compatible: Must be "ovti,ov2640"
> +- clocks: reference to the xvclk input clock.
> +- clock-names: Must be "xvclk".
> +
> +Optional Properties:
> +- resetb-gpios: reference to the GPIO connected to the resetb pin, if any.
> +- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.
> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> +	i2c1: i2c@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: [PATCH v4 5/5] media: ov2640: dt: add the device tree binding document

[Sylwester Nawrocki]

Hi Josh,

On 18/12/14 03:27, Josh Wu wrote:
> Add the document for ov2640 dt.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Josh Wu <josh.wu@atmel.com>

Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

It seems "ovti" is not in the list of vendor prefixes. You may want
to send a patch adding it to Documentation/devicetree/bindings/
vendor-prefixes.txt.

Just few minor comments below..

>  .../devicetree/bindings/media/i2c/ov2640.txt       | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2640.txt b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
> new file mode 100644
> index 0000000..de11ebb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
> @@ -0,0 +1,46 @@
> +* Omnivision ov2640 CMOS sensor

s/ov2640/OV2640 ?

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

I believe it is preferred to put it as "Should contain", rather than
"Must be".

> +- clocks: reference to the xvclk input clock.
> +- clock-names: Must be "xvclk".

--
Regards,
Sylwester

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

[Fabio Estevam]

Hi Sylwester,

On Thu, Dec 18, 2014 at 10:13 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> Hi Josh,
>
> On 18/12/14 03:27, Josh Wu wrote:
>> Add the document for ov2640 dt.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>
> Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>
> It seems "ovti" is not in the list of vendor prefixes. You may want
> to send a patch adding it to Documentation/devicetree/bindings/
> vendor-prefixes.txt.

I have already sent it:
http://patchwork.ozlabs.org/patch/416685/

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

[Josh Wu]

Hi, Sylwester

On 12/18/2014 8:13 PM, Sylwester Nawrocki wrote:
> Hi Josh,
>
> On 18/12/14 03:27, Josh Wu wrote:
>> Add the document for ov2640 dt.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Thanks.

>
> It seems "ovti" is not in the list of vendor prefixes. You may want
> to send a patch adding it to Documentation/devicetree/bindings/
> vendor-prefixes.txt.
>
> Just few minor comments below..
>
>>   .../devicetree/bindings/media/i2c/ov2640.txt       | 46 ++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2640.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2640.txt b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
>> new file mode 100644
>> index 0000000..de11ebb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
>> @@ -0,0 +1,46 @@
>> +* Omnivision ov2640 CMOS sensor
> s/ov2640/OV2640 ?

OK.
>
>> +
>> +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"
> I believe it is preferred to put it as "Should contain", rather than
> "Must be".
I don't have a strong opinion here. After check many documents, it seems 
many people use "Should be".
Is it okay?

Best Regards,
Josh Wu

>
>> +- clocks: reference to the xvclk input clock.
>> +- clock-names: Must be "xvclk".
> --
> Regards,
> Sylwester

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

[Sylwester Nawrocki]

Hi Josh,

On 22/12/14 11:32, Josh Wu wrote:
>>> +Required Properties:
>>> >> +- compatible: Must be "ovti,ov2640"
>> > I believe it is preferred to put it as "Should contain", rather than
>> > "Must be".
>
> I don't have a strong opinion here. After check many documents, it seems 
> many people use "Should be".
> Is it okay?

That's probably slightly better. In general, the point is that the
'compatible' property could potentially contain multiple values, e.g. when
there is introduced a common more generic compatible value for a set
of sensors. However your documentation now says that only one specific value
is allowed. I'm adding Mark at Cc, perhaps he can explain it better.

Please don't consider it as an objection from my side, since we now have
mixture of "must be", "should be", "should contain", etc. across the
DT binding documentation files.

--
Regards,
Sylwester