diff mbox

[v5,1/2] media: ov2680: dt: Add bindings for OV2680

Message ID 20180419110056.10342-2-rui.silva@linaro.org (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Rui Miguel Silva April 19, 2018, 11 a.m. UTC 
Add device tree binding documentation for the OV2680 camera sensor.

Reviewed-by: Rob Herring <robh@kernel.org>
CC: devicetree@vger.kernel.org
Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
---
 .../devicetree/bindings/media/i2c/ov2680.txt  | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2680.txt

Comments

Fabio Estevam April 23, 2018, 2:11 p.m. UTC | #1 
Hi Rui,

On Thu, Apr 19, 2018 at 8:00 AM, Rui Miguel Silva <rui.silva@linaro.org> wrote:

> +Optional Properties:
> +- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
> +                    if any. This is an active high signal to the OV2680.

I looked at the OV2680 datasheet and I see a pin called XSHUTDN, which has
the following description:

XSHUTDN: reset and power down (active low with internal pull down resistor)

So it should be active low, not active high.
Rui Miguel Silva April 23, 2018, 3:09 p.m. UTC | #2 
Hi Fabio,
Thanks for the review.

On Mon 23 Apr 2018 at 14:11, Fabio Estevam wrote:
> Hi Rui,
>
> On Thu, Apr 19, 2018 at 8:00 AM, Rui Miguel Silva 
> <rui.silva@linaro.org> wrote:
>
>> +Optional Properties:
>> +- powerdown-gpios: reference to the GPIO connected to the 
>> powerdown pin,
>> +                    if any. This is an active high signal to 
>> the OV2680.
>
> I looked at the OV2680 datasheet and I see a pin called XSHUTDN, 
> which has
> the following description:
>
> XSHUTDN: reset and power down (active low with internal pull 
> down resistor)
>
> So it should be active low, not active high.

Yes, you are correct, I will fix this, and the dts entry.

Thanks.

---
Cheers,
	Rui
Fabio Estevam April 23, 2018, 3:58 p.m. UTC | #3 
On Mon, Apr 23, 2018 at 12:09 PM, Rui Miguel Silva <rui.silva@linaro.org> wrote:

> Yes, you are correct, I will fix this, and the dts entry.

As this pin has both reset and powerdown functionalities, maybe you
can name the property as 'reset-gpios'

Thanks
Fabio Estevam April 24, 2018, 3:53 p.m. UTC | #4 
Hi Rui,

On Thu, Apr 19, 2018 at 8:00 AM, Rui Miguel Silva <rui.silva@linaro.org> wrote:
> Add device tree binding documentation for the OV2680 camera sensor.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> CC: devicetree@vger.kernel.org
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> ---
>  .../devicetree/bindings/media/i2c/ov2680.txt  | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2680.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2680.txt b/Documentation/devicetree/bindings/media/i2c/ov2680.txt
> new file mode 100644
> index 000000000000..0e29f1a113c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2680.txt
> @@ -0,0 +1,40 @@
> +* Omnivision OV2680 MIPI CSI-2 sensor
> +
> +Required Properties:
> +- compatible: should be "ovti,ov2680".
> +- clocks: reference to the xvclk input clock.
> +- clock-names: should be "xvclk".

You missed to pass the camera power supplies as required properties:

DOVDD-supply
AVDD-supply
DVDD-supply
Rui Miguel Silva April 24, 2018, 4:11 p.m. UTC | #5 
Hi Fabio,
On Tue 24 Apr 2018 at 15:53, Fabio Estevam wrote:
> Hi Rui,
>
> On Thu, Apr 19, 2018 at 8:00 AM, Rui Miguel Silva 
> <rui.silva@linaro.org> wrote:
>> Add device tree binding documentation for the OV2680 camera 
>> sensor.
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> CC: devicetree@vger.kernel.org
>> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
>> ---
>>  .../devicetree/bindings/media/i2c/ov2680.txt  | 40 
>>  +++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>  create mode 100644 
>>  Documentation/devicetree/bindings/media/i2c/ov2680.txt
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/media/i2c/ov2680.txt 
>> b/Documentation/devicetree/bindings/media/i2c/ov2680.txt
>> new file mode 100644
>> index 000000000000..0e29f1a113c0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov2680.txt
>> @@ -0,0 +1,40 @@
>> +* Omnivision OV2680 MIPI CSI-2 sensor
>> +
>> +Required Properties:
>> +- compatible: should be "ovti,ov2680".
>> +- clocks: reference to the xvclk input clock.
>> +- clock-names: should be "xvclk".
>
> You missed to pass the camera power supplies as required 
> properties:

Urgh, yes, you are right, I will add this.

---
Cheers,
	Rui

>
> DOVDD-supply
> AVDD-supply
> DVDD-supply
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/ov2680.txt b/Documentation/devicetree/bindings/media/i2c/ov2680.txt
new file mode 100644
index 000000000000..0e29f1a113c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov2680.txt
@@ -0,0 +1,40 @@ 
+* Omnivision OV2680 MIPI CSI-2 sensor
+
+Required Properties:
+- compatible: should be "ovti,ov2680".
+- clocks: reference to the xvclk input clock.
+- clock-names: should be "xvclk".
+
+Optional Properties:
+- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
+		     if any. This is an active high signal to the OV2680.
+
+The device node must contain one 'port' child node for its digital output
+video port, and this port must have a single endpoint in accordance with
+ the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Endpoint node required properties for CSI-2 connection are:
+- remote-endpoint: a phandle to the bus receiver's endpoint node.
+- clock-lanes: should be set to <0> (clock lane on hardware lane 0).
+- data-lanes: should be set to <1> (one CSI-2 lane supported).
+ 
+Example:
+
+&i2c2 {
+	ov2680: camera-sensor@36 {
+		compatible = "ovti,ov2680";
+		reg = <0x36>;
+		clocks = <&osc>;
+		clock-names = "xvclk";
+		powerdown-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
+
+		port {
+			ov2680_mipi_ep: endpoint {
+				remote-endpoint = <&mipi_sensor_ep>;
+				clock-lanes = <0>;
+				data-lanes = <1>;
+			};
+		};
+	};
+};