diff mbox

[v2,1/2,media] media: i2c/ov5645: add the device tree binding document

Message ID 1463572208-8826-2-git-send-email-todor.tomov@linaro.org
State New
Headers show

Commit Message

Todor Tomov May 18, 2016, 11:50 a.m. UTC
Add the document for ov5645 device tree binding.

Signed-off-by: Todor Tomov <todor.tomov@linaro.org>

---
 .../devicetree/bindings/media/i2c/ov5645.txt       | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Rob Herring (Arm) May 18, 2016, 11:16 p.m. UTC | #1
On Wed, May 18, 2016 at 02:50:07PM +0300, Todor Tomov wrote:
> Add the document for ov5645 device tree binding.

> 

> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>

> ---

>  .../devicetree/bindings/media/i2c/ov5645.txt       | 56 ++++++++++++++++++++++

>  1 file changed, 56 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt

> 

> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt

> new file mode 100644

> index 0000000..8799000

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt

> @@ -0,0 +1,56 @@

> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor

> +

> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with

> +an active array size of 2592H x 1944V. It is programmable through a serial SCCB


s/SCCB/I2C/ because that is the more common name.

> +interface.

> +

> +Required Properties:

> +- compatible: value should be "ovti,ov5645"

> +- clocks: reference to the xclk clock

> +- clock-names: should be "xclk"

> +- assigned-clocks: reference to the xclk clock


This should be optional as it only makes sense if there is more than one 
option.

> +- assigned-clock-rates: should be "23880000"


Doesn't this depend on the board? Most parts take a range of 
frequencies. The driver should know what the range is and request a rate 
within this range.

> +

> +Optional Properties:

> +- reset-gpios: Chip reset GPIO

> +- pwdn-gpios: Chip power down GPIO


Use enable-gpios as it is more common and would just be the inverse of 
this.

Both need to specify the polarity.

> +- dovdd-supply: Chip IO regulator

> +- dvdd-supply: Chip core regulator

> +- avdd-supply: Chip analog regulator

> +

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Todor Tomov May 19, 2016, 8:14 a.m. UTC | #2
Hi Rob,

Thank you for your time to review. My responses are below:

On 05/19/2016 02:16 AM, Rob Herring wrote:
> On Wed, May 18, 2016 at 02:50:07PM +0300, Todor Tomov wrote:

>> Add the document for ov5645 device tree binding.

>>

>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>

>> ---

>>  .../devicetree/bindings/media/i2c/ov5645.txt       | 56 ++++++++++++++++++++++

>>  1 file changed, 56 insertions(+)

>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt

>>

>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt

>> new file mode 100644

>> index 0000000..8799000

>> --- /dev/null

>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt

>> @@ -0,0 +1,56 @@

>> +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor

>> +

>> +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with

>> +an active array size of 2592H x 1944V. It is programmable through a serial SCCB

> 

> s/SCCB/I2C/ because that is the more common name.

Ok.

> 

>> +interface.

>> +

>> +Required Properties:

>> +- compatible: value should be "ovti,ov5645"

>> +- clocks: reference to the xclk clock

>> +- clock-names: should be "xclk"

>> +- assigned-clocks: reference to the xclk clock

> 

> This should be optional as it only makes sense if there is more than one 

> option.

I have only used assigned-clocks to specify for which clock the
assigned-clock-rates property is. This is the way I understood it.
Isn't this correct? (Also please see below.)

> 

>> +- assigned-clock-rates: should be "23880000"

> 

> Doesn't this depend on the board? Most parts take a range of 

> frequencies. The driver should know what the range is and request a rate 

> within this range.

This is the sensor external clock. Actually the driver depends on this value - 
the sensor mode settings which the driver configures are calculated based on
this value. If you change this clock rate you need to change the sensor mode
settings. However they usually come from the vendor of the sensor so they
usually never change. So this clock rate for this driver is fixed to 23.88MHz
and is not expected to change.

> 

>> +

>> +Optional Properties:

>> +- reset-gpios: Chip reset GPIO

>> +- pwdn-gpios: Chip power down GPIO

> 

> Use enable-gpios as it is more common and would just be the inverse of 

> this.

pwdn is the notation which OV use for this gpio, so I'd personally prefer
to keep the name. Do you think it is still better to change it?

> 

> Both need to specify the polarity.

Ok, I'll add a note for the polarity.

> 

>> +- dovdd-supply: Chip IO regulator

>> +- dvdd-supply: Chip core regulator

>> +- avdd-supply: Chip analog regulator

>> +

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


-- 
Best regards,
Todor Tomov
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
new file mode 100644
index 0000000..8799000
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
@@ -0,0 +1,56 @@ 
+* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
+
+The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
+an active array size of 2592H x 1944V. It is programmable through a serial SCCB
+interface.
+
+Required Properties:
+- compatible: value should be "ovti,ov5645"
+- clocks: reference to the xclk clock
+- clock-names: should be "xclk"
+- assigned-clocks: reference to the xclk clock
+- assigned-clock-rates: should be "23880000"
+
+Optional Properties:
+- reset-gpios: Chip reset GPIO
+- pwdn-gpios: Chip power down GPIO
+- dovdd-supply: Chip IO regulator
+- dvdd-supply: Chip core regulator
+- avdd-supply: Chip analog regulator
+
+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 {
+		...
+
+		ov5645: ov5645@78 {
+			compatible = "ovti,ov5645";
+			reg = <0x78>;
+
+			pwdn-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
+			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&camera_rear_default>;
+
+			clocks = <&clks 200>;
+			clock-names = "xclk";
+			assigned-clocks = <&clks 200>;
+			assigned-clock-rates = <23880000>;
+
+			dovdd-supply = <&camera_dovdd_1v8>;
+			avdd-supply = <&camera_avdd_2v8>;
+			dvdd-supply = <&camera_dvdd_1v2>;
+
+			port {
+				ov5645_ep: endpoint {
+					clock-lanes = <1>;
+					data-lanes = <0 2>;
+					remote-endpoint = <&csi0_ep>;
+				};
+			};
+		};
+	};