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

Message ID 1418869646-17071-6-git-send-email-josh.wu@atmel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Josh Wu Dec. 18, 2014, 2:27 a.m. UTC
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
  

Comments

Laurent Pinchart Dec. 18, 2014, 11:56 a.m. UTC | #1
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>;
> +				};
> +			};
> +		};
> +	};
  
Sylwester Nawrocki Dec. 18, 2014, 12:13 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Fabio Estevam Dec. 18, 2014, 12:21 p.m. UTC | #3
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/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Josh Wu Dec. 22, 2014, 10:32 a.m. UTC | #4
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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Sylwester Nawrocki Dec. 22, 2014, 11:47 a.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Patch

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