[PATCH 2/4] media: ov2640: add primary dt support

[PATCH 2/4] media: ov2640: add primary dt support

[Josh Wu]

Add device tree support for ov2640.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Josh Wu <josh.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 drivers/media/i2c/soc_camera/ov2640.c | 95 ++++++++++++++++++++++++++++++++---
 1 file changed, 89 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 9ee910d..6506126 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;
+	int reset_pin;
+	int pwr_down_pin;
 };
 
 /*
@@ -1047,6 +1053,70 @@ 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");
+
+	/* enable or disable the camera */
+	gpio_direction_output(priv->pwr_down_pin, !on);
+
+	return 0;
+}
+
+static int ov2640_hw_reset(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ov2640_priv *priv = to_ov2640(client);
+
+	/* If enabled, give a reset impulse */
+	gpio_direction_output(priv->reset_pin, 0);
+	msleep(20);
+	gpio_direction_output(priv->reset_pin, 1);
+	msleep(100);
+
+	return 0;
+}
+
+static int ov2640_probe_dt(struct i2c_client *client,
+		struct ov2640_priv *priv)
+{
+	struct device_node *np = client->dev.of_node;
+	int ret;
+
+	priv->reset_pin = of_get_named_gpio(np, "reset-gpio", 0);
+	priv->pwr_down_pin = of_get_named_gpio(np, "power-down-gpio", 0);
+	if (!gpio_is_valid(priv->reset_pin) ||
+			!gpio_is_valid(priv->pwr_down_pin))
+		return -EINVAL;
+
+	ret = devm_gpio_request(&client->dev, priv->pwr_down_pin,
+			"power-down-pin");
+	if (ret < 0) {
+		dev_err(&client->dev, "request gpio pin %d failed\n",
+				priv->pwr_down_pin);
+		return ret;
+	}
+
+	ret = devm_gpio_request(&client->dev, priv->reset_pin, "reset_pin");
+	if (ret < 0) {
+		dev_err(&client->dev, "request gpio pin %d failed\n",
+				priv->reset_pin);
+		return ret;
+	}
+
+	/* 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 +1128,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 +1141,18 @@ static int ov2640_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
+	if (!ssdd) {
+		if (client->dev.of_node) {
+			ret = ov2640_probe_dt(client, priv);
+			if (ret)
+				return ret;
+		} else {
+			dev_err(&client->dev,
+				"Missing platform_data for driver\n");
+			return  -EINVAL;
+		}
+	}
+
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
 	v4l2_ctrl_handler_init(&priv->hdl, 2);
 	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
@@ -1123,9 +1199,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 = "omnivision,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

--
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: [PATCH 2/4] media: ov2640: add primary dt support

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Friday 28 November 2014 18:28:25 Josh Wu wrote:
> Add device tree support for ov2640.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
>  drivers/media/i2c/soc_camera/ov2640.c | 95 +++++++++++++++++++++++++++++---
>  1 file changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..6506126 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;
> +	int reset_pin;
> +	int pwr_down_pin;
>  };
> 
>  /*
> @@ -1047,6 +1053,70 @@ 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");
> +
> +	/* enable or disable the camera */
> +	gpio_direction_output(priv->pwr_down_pin, !on);

Isn't there a delay required to wake the chip up ?

> +
> +	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 the reset GPIO isn't specified you should return immediately without 
waiting 120ms.

> +	/* If enabled, give a reset impulse */
> +	gpio_direction_output(priv->reset_pin, 0);
> +	msleep(20);

Please use usleep_range().

> +	gpio_direction_output(priv->reset_pin, 1);
> +	msleep(100);

Is the reset delay documented somewhere ? 100ms seems pretty large to me.

> +
> +	return 0;
> +}
> +
> +static int ov2640_probe_dt(struct i2c_client *client,
> +		struct ov2640_priv *priv)
> +{
> +	struct device_node *np = client->dev.of_node;
> +	int ret;
> +
> +	priv->reset_pin = of_get_named_gpio(np, "reset-gpio", 0);
> +	priv->pwr_down_pin = of_get_named_gpio(np, "power-down-gpio", 0);
> +	if (!gpio_is_valid(priv->reset_pin) ||
> +			!gpio_is_valid(priv->pwr_down_pin))
> +		return -EINVAL;
> +
> +	ret = devm_gpio_request(&client->dev, priv->pwr_down_pin,
> +			"power-down-pin");
> +	if (ret < 0) {
> +		dev_err(&client->dev, "request gpio pin %d failed\n",
> +				priv->pwr_down_pin);
> +		return ret;
> +	}
> +
> +	ret = devm_gpio_request(&client->dev, priv->reset_pin, "reset_pin");
> +	if (ret < 0) {
> +		dev_err(&client->dev, "request gpio pin %d failed\n",
> +				priv->reset_pin);
> +		return ret;
> +	}

Please use the gpiod API with the devm_gpiod_get_index_optional() function, it 
will simply your code and take care about GPIO polarity automatically. Don't 
forget to specify the default state using the GPIOD_* flags.

> +
> +	/* 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 +1128,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 +1141,18 @@ static int ov2640_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  	}
> 
> +	if (!ssdd) {
> +		if (client->dev.of_node) {
> +			ret = ov2640_probe_dt(client, priv);
> +			if (ret)
> +				return ret;
> +		} else {
> +			dev_err(&client->dev,
> +				"Missing platform_data for driver\n");
> +			return  -EINVAL;
> +		}
> +	}
> +
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
>  	v4l2_ctrl_handler_init(&priv->hdl, 2);
>  	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
> @@ -1123,9 +1199,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 = "omnivision,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: [PATCH 2/4] media: ov2640: add primary dt support

[Josh Wu]

Hi, Laurent

On 12/2/2014 6:33 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Friday 28 November 2014 18:28:25 Josh Wu wrote:
>> Add device tree support for ov2640.
>>
>> Cc:devicetree@vger.kernel.org
>> Signed-off-by: Josh Wu<josh.wu@atmel.com>
>> ---
>>   drivers/media/i2c/soc_camera/ov2640.c | 95 +++++++++++++++++++++++++++++---
>>   1 file changed, 89 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
>> b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..6506126 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;
>> +	int reset_pin;
>> +	int pwr_down_pin;
>>   };
>>
>>   /*
>> @@ -1047,6 +1053,70 @@ 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");
>> +
>> +	/* enable or disable the camera */
>> +	gpio_direction_output(priv->pwr_down_pin, !on);
> Isn't there a delay required to wake the chip up ?
I don't see any document mention a delay is needed when waked up.

>
>> +
>> +	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 the reset GPIO isn't specified you should return immediately without
> waiting 120ms.

I find some document that said the reset pulse need >= 3ms. So I think 
use 3~5ms is enough.

>
>> +	/* If enabled, give a reset impulse */
>> +	gpio_direction_output(priv->reset_pin, 0);
>> +	msleep(20);
> Please use usleep_range().

ok.
>
>> +	gpio_direction_output(priv->reset_pin, 1);
>> +	msleep(100);
> Is the reset delay documented somewhere ? 100ms seems pretty large to me.

It seems no need to wait another delay after the reset pulse is generated.
I will remove this line: msleep(100);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ov2640_probe_dt(struct i2c_client *client,
>> +		struct ov2640_priv *priv)
>> +{
>> +	struct device_node *np = client->dev.of_node;
>> +	int ret;
>> +
>> +	priv->reset_pin = of_get_named_gpio(np, "reset-gpio", 0);
>> +	priv->pwr_down_pin = of_get_named_gpio(np, "power-down-gpio", 0);
>> +	if (!gpio_is_valid(priv->reset_pin) ||
>> +			!gpio_is_valid(priv->pwr_down_pin))
>> +		return -EINVAL;
>> +
>> +	ret = devm_gpio_request(&client->dev, priv->pwr_down_pin,
>> +			"power-down-pin");
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "request gpio pin %d failed\n",
>> +				priv->pwr_down_pin);
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_gpio_request(&client->dev, priv->reset_pin, "reset_pin");
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "request gpio pin %d failed\n",
>> +				priv->reset_pin);
>> +		return ret;
>> +	}
> Please use the gpiod API with the devm_gpiod_get_index_optional() function, it
> will simply your code and take care about GPIO polarity automatically. Don't
> forget to specify the default state using the GPIOD_* flags.

Thank, I will use it.
It seems in this case, use devm_gpiod_get_optional() is enough.

Best Regards,
Josh Wu
>
>> +
>> +	/* 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 +1128,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 +1141,18 @@ static int ov2640_probe(struct i2c_client *client,
>>   		return -ENOMEM;
>>   	}
>>
>> +	if (!ssdd) {
>> +		if (client->dev.of_node) {
>> +			ret = ov2640_probe_dt(client, priv);
>> +			if (ret)
>> +				return ret;
>> +		} else {
>> +			dev_err(&client->dev,
>> +				"Missing platform_data for driver\n");
>> +			return  -EINVAL;
>> +		}
>> +	}
>> +
>>   	v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops);
>>   	v4l2_ctrl_handler_init(&priv->hdl, 2);
>>   	v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops,
>> @@ -1123,9 +1199,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 = "omnivision,ov2640", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ov2640_of_match);
>> +
>>   static struct i2c_driver ov2640_i2c_driver = {
>>   	.driver = {
>>   		.name = "ov2640",
>> +		.of_match_table = of_match_ptr(ov2640_of_match),
>>   	},
>>   	.probe    = ov2640_probe,
>>   	.remove   = ov2640_remove,

[PATCH 4/4] 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>
---
 .../devicetree/bindings/media/i2c/ov2640.txt       | 43 ++++++++++++++++++++++
 1 file changed, 43 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..adec147
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
@@ -0,0 +1,43 @@
+* 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 "omnivision,ov2640"
+- reset-gpio      : reset pin
+- power-down-gpio : power down pin
+
+Optional Properties:
+- clocks          : reference master clock, if using external fixed clock, you
+                    no need to have such property.
+- clock-names     : Must be "mck", it means the master clock for ov2640.
+
+For further reading of port node refer Documentation/devicetree/bindings/media/
+video-interfaces.txt.
+
+Example:
+
+	i2c1: i2c@f0018000 {
+		ov2640: camera@0x30 {
+			compatible = "omnivision,ov2640";
+			reg = <0x30>;
+
+			... ...
+
+			reset-gpio = <&pioE 24 GPIO_ACTIVE_HIGH>;
+			power-down-gpio = <&pioE 29 GPIO_ACTIVE_HIGH>;
+
+			/* use pck1 for the master clock of ov2640 */
+			clocks = <&pck1>;
+			clock-names = "mck";
+
+			port {
+				ov2640_0: endpoint {
+					remote-endpoint = <&isi_0>;
+					bus-width = <8>;
+				};
+			};
+		};
+	};
-- 
1.9.1

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

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Friday 28 November 2014 18:28:27 Josh Wu wrote:
> Add the document for ov2640 dt.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
>  .../devicetree/bindings/media/i2c/ov2640.txt       | 43 +++++++++++++++++++
>  1 file changed, 43 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..adec147
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
> @@ -0,0 +1,43 @@
> +* 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 "omnivision,ov2640"

The usual practice is to use the company's stock ticker as a prefix. In this 
case the compatible string would be "ovti,ov2640". You need to add the prefix 
to Documentation/devicetree/bindings/vendor-prefixes.txt.

> +- reset-gpio      : reset pin
> +- power-down-gpio : power down pin

That should be reset-gpios and power-down-gpios, even if there's a single 
GPIO. Furthermore, given that the power down pin is named PWDN you might want 
to name the property pwdn-gpios.

The reset and pwdn signals won't be connected on all boards, so the two 
properties should be optional.

> +Optional Properties:
> +- clocks          : reference master clock, if using external fixed clock,
> you
> +                    no need to have such property.

The clock is required by the chip, so even when using an external fixed clock 
the property should be present, and reference a fixed clock node. The clocks 
and clock-names properties should thus be mandatory.

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

The clock input is named xvclk in the datasheet, you should use the same name 
here.

> +
> +For further reading of port node refer
> Documentation/devicetree/bindings/media/
> +video-interfaces.txt.

Even if you reference that document you should still mention what port node(s) 
these bindings require. Something like the following text should be enough.

"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 = "omnivision,ov2640";
> +			reg = <0x30>;
> +
> +			... ...

No need for an ellipsis, what are you trying to hide ? :-)

> +
> +			reset-gpio = <&pioE 24 GPIO_ACTIVE_HIGH>;
> +			power-down-gpio = <&pioE 29 GPIO_ACTIVE_HIGH>;
> +
> +			/* use pck1 for the master clock of ov2640 */

I think you can drop the comment.

> +			clocks = <&pck1>;
> +			clock-names = "mck";
> +
> +			port {
> +				ov2640_0: endpoint {
> +					remote-endpoint = <&isi_0>;
> +					bus-width = <8>;
> +				};
> +			};
> +		};
> +	};

-- 
Regards,

Laurent Pinchart

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

[Josh Wu]

Hi, Laurent

On 12/2/2014 6:14 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Friday 28 November 2014 18:28:27 Josh Wu wrote:
>> Add the document for ov2640 dt.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>   .../devicetree/bindings/media/i2c/ov2640.txt       | 43 +++++++++++++++++++
>>   1 file changed, 43 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..adec147
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov2640.txt
>> @@ -0,0 +1,43 @@
>> +* 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 "omnivision,ov2640"
> The usual practice is to use the company's stock ticker as a prefix. In this
> case the compatible string would be "ovti,ov2640". You need to add the prefix
> to Documentation/devicetree/bindings/vendor-prefixes.txt.
>
>> +- reset-gpio      : reset pin
>> +- power-down-gpio : power down pin
> That should be reset-gpios and power-down-gpios, even if there's a single
> GPIO. Furthermore, given that the power down pin is named PWDN you might want
> to name the property pwdn-gpios.
>
> The reset and pwdn signals won't be connected on all boards, so the two
> properties should be optional.
>
>> +Optional Properties:
>> +- clocks          : reference master clock, if using external fixed clock,
>> you
>> +                    no need to have such property.
> The clock is required by the chip, so even when using an external fixed clock
> the property should be present, and reference a fixed clock node. The clocks
> and clock-names properties should thus be mandatory.
>
>> +- clock-names     : Must be "mck", it means the master clock for ov2640.
> The clock input is named xvclk in the datasheet, you should use the same name
> here.
>
>> +
>> +For further reading of port node refer
>> Documentation/devicetree/bindings/media/
>> +video-interfaces.txt.
> Even if you reference that document you should still mention what port node(s)
> these bindings require. Something like the following text should be enough.
>
> "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 = "omnivision,ov2640";
>> +			reg = <0x30>;
>> +
>> +			... ...
> No need for an ellipsis, what are you trying to hide ? :-)

I just emit the pinctrl property here. Will add it back.
>
>> +
>> +			reset-gpio = <&pioE 24 GPIO_ACTIVE_HIGH>;
>> +			power-down-gpio = <&pioE 29 GPIO_ACTIVE_HIGH>;
>> +
>> +			/* use pck1 for the master clock of ov2640 */
> I think you can drop the comment.

I will send a new version which will fix all above.
Thanks for your time to review the patch. That help me a lot.

Best Regards,
Josh Wu

>
>> +			clocks = <&pck1>;
>> +			clock-names = "mck";
>> +
>> +			port {
>> +				ov2640_0: endpoint {
>> +					remote-endpoint = <&isi_0>;
>> +					bus-width = <8>;
>> +				};
>> +			};
>> +		};
>> +	};

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

[Laurent Pinchart]

Hi Josh,

(CC'ing the devicetree@vger.kernel.org mailing list)

Thank you for the patch.

On Friday 28 November 2014 18:28:26 Josh Wu wrote:
> The master clock can be optional. It's a common clock framework clock.
> It can make sensor output a pixel clock to the camera interface.
> 
> If you just use a external oscillator clock as the master clock, then,
> just don't need set 'mck' in dt node.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
>  drivers/media/i2c/soc_camera/ov2640.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> b/drivers/media/i2c/soc_camera/ov2640.c index 6506126..06c2aa9 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,8 @@
> 
>  #define VAL_SET(x, mask, rshift, lshift)  \
>  		((((x) >> rshift) & mask) << lshift)
> +#define DEFAULT_MASTER_CLK_FREQ		25000000
> +
>  /*
>   * DSP registers
>   * register offset for BANK_SEL == BANK_SEL_DSP
> @@ -284,6 +287,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 +750,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 = 0;
> 
>  	if (!priv->clk) {
>  		clk = v4l2_clk_get(&client->dev, "mclk");
> @@ -755,6 +760,16 @@ static int ov2640_s_power(struct v4l2_subdev *sd, int
> on) priv->clk = clk;
>  	}
> 
> +	if (!IS_ERR(priv->master_clk)) {

The clock should be mandatory, you can thus drop this check.

> +		if (on)
> +			ret = clk_prepare_enable(priv->master_clk);
> +		else
> +			clk_disable_unprepare(priv->master_clk);
> +
> +		if (ret)
> +			return ret;

You can move the error check inside the first branch of the if and remove the 
ret = 0 initialization above.

> +	}
> +
>  	return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);

If this call fails you should disable or enable the clock to undo the 
enable/disable above.

>  }
> 
> @@ -1153,6 +1168,16 @@ static int ov2640_probe(struct i2c_client *client,
>  		}
>  	}
> 
> +	priv->master_clk = devm_clk_get(&client->dev, "mck");
> +	if (!IS_ERR(priv->master_clk)) {
> +		/* Set ISI_MCK's frequency, it should be faster than pixel
> +		 * clock.
> +		 */
> +		ret = clk_set_rate(priv->master_clk, DEFAULT_MASTER_CLK_FREQ);

The clock frequency should be system-dependent. For the DT case an easy 
implementation would be to use the assigned-clock-rates to set the desired 
clock frequency is case of a variable clock, as adding a sensor-specific 
property to specify the desired clock frequency only to read that property in 
the driver and call clk_set_rate() seems a bit pointless to me.

> +		if (ret < 0)
> +			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,

-- 
Regards,

Laurent Pinchart

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

[Josh Wu]

Hi, Laurent

On 12/2/2014 6:26 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> (CC'ing the devicetree@vger.kernel.org mailing list)
>
> Thank you for the patch.
>
> On Friday 28 November 2014 18:28:26 Josh Wu wrote:
>> The master clock can be optional. It's a common clock framework clock.
>> It can make sensor output a pixel clock to the camera interface.
>>
>> If you just use a external oscillator clock as the master clock, then,
>> just don't need set 'mck' in dt node.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>   drivers/media/i2c/soc_camera/ov2640.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
>> b/drivers/media/i2c/soc_camera/ov2640.c index 6506126..06c2aa9 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,8 @@
>>
>>   #define VAL_SET(x, mask, rshift, lshift)  \
>>   		((((x) >> rshift) & mask) << lshift)
>> +#define DEFAULT_MASTER_CLK_FREQ		25000000
>> +
>>   /*
>>    * DSP registers
>>    * register offset for BANK_SEL == BANK_SEL_DSP
>> @@ -284,6 +287,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 +750,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 = 0;
>>
>>   	if (!priv->clk) {
>>   		clk = v4l2_clk_get(&client->dev, "mclk");
>> @@ -755,6 +760,16 @@ static int ov2640_s_power(struct v4l2_subdev *sd, int
>> on) priv->clk = clk;
>>   	}
>>
>> +	if (!IS_ERR(priv->master_clk)) {
> The clock should be mandatory, you can thus drop this check.
OK. sure.

>
>> +		if (on)
>> +			ret = clk_prepare_enable(priv->master_clk);
>> +		else
>> +			clk_disable_unprepare(priv->master_clk);
>> +
>> +		if (ret)
>> +			return ret;
> You can move the error check inside the first branch of the if and remove the
> ret = 0 initialization above.
>
>> +	}
>> +
>>   	return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
> If this call fails you should disable or enable the clock to undo the
> enable/disable above.

ah, I totally forget this. will fix in next version. Thanks.

>
>>   }
>>
>> @@ -1153,6 +1168,16 @@ static int ov2640_probe(struct i2c_client *client,
>>   		}
>>   	}
>>
>> +	priv->master_clk = devm_clk_get(&client->dev, "mck");
>> +	if (!IS_ERR(priv->master_clk)) {
>> +		/* Set ISI_MCK's frequency, it should be faster than pixel
>> +		 * clock.
>> +		 */
>> +		ret = clk_set_rate(priv->master_clk, DEFAULT_MASTER_CLK_FREQ);
> The clock frequency should be system-dependent. For the DT case an easy
> implementation would be to use the assigned-clock-rates to set the desired
> clock frequency is case of a variable clock, as adding a sensor-specific
> property to specify the desired clock frequency only to read that property in
> the driver and call clk_set_rate() seems a bit pointless to me.

I tried assigned-clock-rates and it works fine.
The only issue is the it cannot used by multiple nodes. But I think I 
can put a comments in the dt node for this.
Thanks.

Best Regards,
Josh Wu

>
>> +		if (ret < 0)
>> +			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,