nokia n900: update dts with camera support

Add camera support to N900 dts. Also add a note about MMC & debugging.

Signed-off-by Pavel Machek <[email protected]>

arch/arm/boot/dts/omap3-n900.dts +87 -0

index 49f3708..b1be53d 100644
@@ -144,6 +144,15 @@
 		io-channel-names = "temp", "bsi", "vbat";
 	};
 
+	rear_camera: camera at 0 {
+		compatible = "linux,camera";
+
+		module {
+			model = "TCM8341MD";
+			sensor = <&cam1>;
+		};
+	};
+
 	pwm9: dmtimer-pwm {
 		compatible = "ti,omap-dmtimer-pwm";
 		#pwm-cells = <3>;

@@ -164,6 +173,31 @@
 	};
 };
 
+&isp {
+	vdds_csib-supply = <&vaux2>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&camera_pins>;
+
+	ports {
+		port at 1 {
+			reg = <1>;
+
+			csi_isp: endpoint {
+				remote-endpoint = <&csi_cam1>;
+				bus-type = <3>; /* CCP2 */
+				clock-lanes = <1>;
+				data-lanes = <0>;
+				lane-polarity = <0 0>;
+				clock-inv = <0>;
+				/* Select strobe = <1> for back camera, <0> for front camera */
+				strobe = <1>;
+				crc = <0>;
+			};
+		};
+	};
+};
+
 &omap3_pmx_core {
 	pinctrl-names = "default";
 

@@ -328,6 +362,22 @@
 			OMAP3_CORE1_IOPAD(0x218e, PIN_OUTPUT | MUX_MODE4)		/* gpio 157 => cmt_bsi */
 		>;
 	};
+
+	camera_pins: pinmux_camera {
+		pinctrl-single,pins = <
+			OMAP3_CORE1_IOPAD(0x210c, PIN_OUTPUT | MUX_MODE7)       /* cam_hs */
+			OMAP3_CORE1_IOPAD(0x210e, PIN_OUTPUT | MUX_MODE7)       /* cam_vs */
+			OMAP3_CORE1_IOPAD(0x2110, PIN_OUTPUT | MUX_MODE0)       /* cam_xclka */
+			OMAP3_CORE1_IOPAD(0x211e, PIN_OUTPUT | MUX_MODE7)       /* cam_d4 */
+			OMAP3_CORE1_IOPAD(0x2122, PIN_INPUT | MUX_MODE0)        /* cam_d6 */
+			OMAP3_CORE1_IOPAD(0x2124, PIN_INPUT | MUX_MODE0)        /* cam_d7 */
+			OMAP3_CORE1_IOPAD(0x2126, PIN_INPUT | MUX_MODE0)        /* cam_d8 */
+			OMAP3_CORE1_IOPAD(0x2128, PIN_INPUT | MUX_MODE0)        /* cam_d9 */
+			OMAP3_CORE1_IOPAD(0x212a, PIN_OUTPUT | MUX_MODE7)       /* cam_d10 */
+			OMAP3_CORE1_IOPAD(0x212e, PIN_OUTPUT | MUX_MODE7)       /* cam_xclkb */
+			OMAP3_CORE1_IOPAD(0x2132, PIN_OUTPUT | MUX_MODE0)       /* cam_strobe */
+		>;
+	};
 };
 
 &i2c1 {

@@ -726,6 +776,40 @@
 		st,max-limit-y = <32>;
 		st,max-limit-z = <32>;
 	};
+
+	cam1: camera at 3e {
+		compatible = "toshiba,et8ek8";
+		reg = <0x3e>;
+
+		vana-supply = <&vaux4>;
+
+		clocks = <&isp 0>;
+		clock-names = "extclk";
+		clock-frequency = <9600000>;
+
+		reset-gpio = <&gpio4 6 GPIO_ACTIVE_HIGH>; /* 102 */
+
+		port {
+			csi_cam1: endpoint {
+				bus-type = <3>; /* CCP2 */
+				strobe = <1>;
+				clock-inv = <0>;
+				crc = <1>;
+
+				remote-endpoint = <&csi_isp>;
+			};
+		};
+	};
+
+	/* D/A converter for auto-focus */
+	ad5820: dac at 0c {
+		compatible = "adi,ad5820";
+		reg = <0x0c>;
+
+		VANA-supply = <&vaux4>;
+
+		#io-channel-cells = <0>;
+	};
 };
 
 &mmc1 {

@@ -733,6 +817,9 @@
 	pinctrl-0 = <&mmc1_pins>;
 	vmmc-supply = <&vmmc1>;
 	bus-width = <4>;
+	/* For debugging, it is often good idea to remove this GPIO.
+	   It means you can remove back cover (to reboot by removing
+	   battery) and still use the MMC card. */
 	cd-gpios = <&gpio6 0 GPIO_ACTIVE_HIGH>; /* 160 */
 };
 

Quoted 4 lines · On Thursday 10 August 2017 22:49:23 Pavel Machek wrote:

Add camera support to N900 dts. Also add a note about MMC & debugging.

Signed-off-by Pavel Machek <[email protected]>

Looks good,

Acked-by Pali Roh?r <[email protected]>

Add camera support to N900 dts. Also add a note about MMC & debugging.

Signed-off-by Pavel Machek <[email protected]>

arch/arm/boot/dts/omap3-n900.dts +0 -0

index 49f3708..b1be53d 100644
@@ -144,6 +144,15 @@
 		io-channel-names = "temp", "bsi", "vbat";

arch/arm/boot/dts/omap3-n900.dts +11 -0

+
 	pwm9: dmtimer-pwm {
 		compatible = "ti,omap-dmtimer-pwm";
 		#pwm-cells = <3>;

@@ -164,6 +173,31 @@
 	};
 };
 
+&isp {
+	vdds_csib-supply = <&vaux2>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&camera_pins>;
+
+	ports {
+		port at 1 {
+			reg = <1>;
+

arch/arm/boot/dts/omap3-n900.dts +58 -0

+			};
+		};
+	};
+};
+
 &omap3_pmx_core {
 	pinctrl-names = "default";
 

@@ -328,6 +362,22 @@
 			OMAP3_CORE1_IOPAD(0x218e, PIN_OUTPUT | MUX_MODE4)		/* gpio 157 => cmt_bsi */
 		>;
 	};
+
+	camera_pins: pinmux_camera {
+		pinctrl-single,pins = <
+			OMAP3_CORE1_IOPAD(0x210c, PIN_OUTPUT | MUX_MODE7)       /* cam_hs */
+			OMAP3_CORE1_IOPAD(0x210e, PIN_OUTPUT | MUX_MODE7)       /* cam_vs */
+			OMAP3_CORE1_IOPAD(0x2110, PIN_OUTPUT | MUX_MODE0)       /* cam_xclka */
+			OMAP3_CORE1_IOPAD(0x211e, PIN_OUTPUT | MUX_MODE7)       /* cam_d4 */
+			OMAP3_CORE1_IOPAD(0x2122, PIN_INPUT | MUX_MODE0)        /* cam_d6 */
+			OMAP3_CORE1_IOPAD(0x2124, PIN_INPUT | MUX_MODE0)        /* cam_d7 */
+			OMAP3_CORE1_IOPAD(0x2126, PIN_INPUT | MUX_MODE0)        /* cam_d8 */
+			OMAP3_CORE1_IOPAD(0x2128, PIN_INPUT | MUX_MODE0)        /* cam_d9 */
+			OMAP3_CORE1_IOPAD(0x212a, PIN_OUTPUT | MUX_MODE7)       /* cam_d10 */
+			OMAP3_CORE1_IOPAD(0x212e, PIN_OUTPUT | MUX_MODE7)       /* cam_xclkb */
+			OMAP3_CORE1_IOPAD(0x2132, PIN_OUTPUT | MUX_MODE0)       /* cam_strobe */
+		>;
+	};
 };
 
 &i2c1 {

@@ -726,6 +776,40 @@
 		st,max-limit-y = <32>;
 		st,max-limit-z = <32>;
 	};
+
+	cam1: camera at 3e {
+		compatible = "toshiba,et8ek8";
+		reg = <0x3e>;
+
+		vana-supply = <&vaux4>;
+
+		clocks = <&isp 0>;
+		clock-names = "extclk";
+		clock-frequency = <9600000>;
+
+		reset-gpio = <&gpio4 6 GPIO_ACTIVE_HIGH>; /* 102 */
+
+		port {
+			csi_cam1: endpoint {
+				bus-type = <3>; /* CCP2 */
+				strobe = <1>;
+				clock-inv = <0>;
+				crc = <1>;
+
+				remote-endpoint = <&csi_isp>;
+			};
+		};
+	};
+
+	/* D/A converter for auto-focus */
+	ad5820: dac at 0c {
+		compatible = "adi,ad5820";
+		reg = <0x0c>;
+
+		VANA-supply = <&vaux4>;
+
+		#io-channel-cells = <0>;
+	};
 };
 
 &mmc1 {

@@ -733,6 +817,9 @@
 	pinctrl-0 = <&mmc1_pins>;
 	vmmc-supply = <&vmmc1>;
 	bus-width = <4>;
+	/* For debugging, it is often good idea to remove this GPIO.
+	   It means you can remove back cover (to reboot by removing
+	   battery) and still use the MMC card. */
 	cd-gpios = <&gpio6 0 GPIO_ACTIVE_HIGH>; /* 160 */
 };
 

Hi Pavel,

Thanks for the patch.

I understand Tony already applied this one. I'd have a few comments below, could you address them in another patch, please?

I understand Tony already applied this one. I'd have a few comments below, could you address them in another patch, please?

Review

Quoted 8 lines

(quoted diff) +8 -0

+	rear_camera: camera at 0 {
+		compatible = "linux,camera";
+
+		module {
+			model = "TCM8341MD";
+			sensor = <&cam1>;
+		};
+	};

There's no reference to rear_camera. I understand that this represents the camera module but as we don't have any documented way to describe camera modules (nor we have really discussed the matter), this should be removed for now.

Perhaps later on, but I be it'd look quite different.

Review

Quoted 8 lines

(quoted diff) +8 -0

+				bus-type = <3>; /* CCP2 */
+				clock-lanes = <1>;
+				data-lanes = <0>;
+				lane-polarity = <0 0>;
+				clock-inv = <0>;
+				/* Select strobe = <1> for back camera, <0> for front camera */
+				strobe = <1>;
+				crc = <0>;

crc is not a documented binding in video-interfaces.txt. Same for clock-inv. clock-inv is used by v4l2-fwnode.c already. Describing both in video-interfaces.txt would certainly make sense though, as well as adding support for crc in CCP2 to V4L2 fwnode.

Could you submit patches for that? I can do that as well but it'll probably take some days at least.

Hi,

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 123

Quoted 10 lines · Pavel Machek

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -0

index 852041a..aade681 100644
@@ -119,6 +119,8 @@ Optional endpoint properties
   as 0 (normal). This property is valid for serial busses only.
 - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used
   with CCP2, for instance.
+- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted
+- crc: crc is in use

booleans in DT are not done with <0> and <1>, but as properties without values. True = property exists, False = property does not exist.

Quoted 16 lines

Hi,

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 123

Quoted 10 lines · Pavel Machek

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -0

index 852041a..aade681 100644
@@ -119,6 +119,8 @@ Optional endpoint properties
   as 0 (normal). This property is valid for serial busses only.
 - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used
   with CCP2, for instance.
+- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted
+- crc: crc is in use

booleans in DT are not done with <0> and <1>, but as properties without values. True = property exists, False = property does not exist.

Well, strobe property above already uses = <0>/<1> format, as do others.

Problem with "false = property does not exist" is that you don't know if it is "someone forgot to define it" or "someone made a typo" or "dts is too old to know about this property" or "the property indeed should be false"...

Quoted 14 lines · Pavel Machek

Quoted 6 lines · Pavel Machek

Quoted 5 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 852041a..aade681 100644
@@ -119,6 +119,8 @@ Optional endpoint properties

Quoted 8 lines

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 123

Quoted 5 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -0

   as 0 (normal). This property is valid for serial busses only.
 - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used
   with CCP2, for instance.
+- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted
+- crc: crc is in use

booleans in DT are not done with <0> and <1>, but as properties without values. True = property exists, False = property does not exist.

Now that you mention it, I remember that now.

Quoted 2 lines

Well, strobe property above already uses = <0>/<1> format, as do others.

Quoted 4 lines

Problem with "false = property does not exist" is that you don't know if it is "someone forgot to define it" or "someone made a typo" or "dts is too old to know about this property" or "the property indeed should be false"...

As this is an established practice, I think we should follow it for bool properties.

We could change the existing ones, too, and leave some extra checks in place to handle old dtbs.

Hi,

Quoted 21 lines · Sakari Ailus

Quoted 14 lines · Pavel Machek

Quoted 6 lines · Pavel Machek

Quoted 5 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +0 -0

index 852041a..aade681 100644
@@ -119,6 +119,8 @@ Optional endpoint properties

Quoted 8 lines

Documentation/devicetree/bindings/media/video-interfaces.txt Optional endpoint properties · line 123

Quoted 5 lines

Documentation/devicetree/bindings/media/video-interfaces.txt +2 -0

   as 0 (normal). This property is valid for serial busses only.
 - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used
   with CCP2, for instance.
+- clock-inv: polarity of clock/strobe signal. 0 - not inverted, 1 - inverted
+- crc: crc is in use

booleans in DT are not done with <0> and <1>, but as properties without values. True = property exists, False = property does not exist.

Now that you mention it, I remember that now.

Quoted 2 lines

Well, strobe property above already uses = <0>/<1> format, as do others.

Quoted 7 lines

Quoted 4 lines

Problem with "false = property does not exist" is that you don't know if it is "someone forgot to define it" or "someone made a typo" or "dts is too old to know about this property" or "the property indeed should be false"...

As this is an established practice, I think we should follow it for bool properties.

Yes it's common practice, there is also device_property_read_bool() to get the value.

Quoted 2 lines

We could change the existing ones, too, and leave some extra checks in place to handle old dtbs.

The following should be downward compatible:

var = read_bool();
if (var && !read_int())

    var = false;

Btw. DT people should be CC'd for DT binding additions/changes.

Quoted 10 lines

Quoted 7 lines

Well, strobe property above already uses = <0>/<1> format, as do others.

Problem with "false = property does not exist" is that you don't know if it is "someone forgot to define it" or "someone made a typo" or "dts is too old to know about this property" or "the property indeed should be false"...

As this is an established practice, I think we should follow it for bool properties.

Yes it's common practice, there is also device_property_read_bool() to get the value.

Quoted 2 lines

We could change the existing ones, too, and leave some extra checks in place to handle old dtbs.

The following should be downward compatible:

var = read_bool();
if (var && !read_int())

    var = false;

Btw. DT people should be CC'd for DT binding additions/changes.

Hi!

Quoted 17 lines

Quoted 10 lines

Quoted 7 lines

Well, strobe property above already uses = <0>/<1> format, as do others.

Problem with "false = property does not exist" is that you don't know if it is "someone forgot to define it" or "someone made a typo" or "dts is too old to know about this property" or "the property indeed should be false"...

As this is an established practice, I think we should follow it for bool properties.

Yes it's common practice, there is also device_property_read_bool() to get the value.

Quoted 2 lines

We could change the existing ones, too, and leave some extra checks in place to handle old dtbs.

Quoted 7 lines

The following should be downward compatible:

var = read_bool();
if (var && !read_int())

    var = false;

Btw. DT people should be CC'd for DT binding additions/changes.

Ok, so for now we do crc=1 by default. Now we'd like to have crc support configurable in the dts. But if we just introduce "crc;" option, it will break old dts users. We could introduce "no-crc;" and that would work in this particular case, but will break when we want different defaults at different devices.

Anyway, introducing "no-crc;" seems pretty ugly to me. I'd rather do "crc=<0>;", in a similar way we handle other options now.

Hi Pavel,

Quoted 33 lines · Pavel Machek

Hi!

Quoted 17 lines

Quoted 10 lines

Quoted 7 lines

Well, strobe property above already uses = <0>/<1> format, as do others.

Problem with "false = property does not exist" is that you don't know if it is "someone forgot to define it" or "someone made a typo" or "dts is too old to know about this property" or "the property indeed should be false"...

As this is an established practice, I think we should follow it for bool properties.

Yes it's common practice, there is also device_property_read_bool() to get the value.

Quoted 2 lines

We could change the existing ones, too, and leave some extra checks in place to handle old dtbs.

Quoted 7 lines

The following should be downward compatible:

var = read_bool();
if (var && !read_int())

     var = false;

Btw. DT people should be CC'd for DT binding additions/changes.

Ok, so for now we do crc=1 by default. Now we'd like to have crc support configurable in the dts. But if we just introduce "crc;" option, it will break old dts users. We could introduce "no-crc;" and that would work in this particular case, but will break when we want different defaults at different devices.

Quoted 2 lines

Anyway, introducing "no-crc;" seems pretty ugly to me. I'd rather do "crc=<0>;", in a similar way we handle other options now.

The same issue actually exists for CSI-2: the CRC is typically enabled and I think you can at least disable the check (in OMAP3 ISP). Do we have a need to disable it at the moment for any purpose? I think I've seen this was disabled somewhere but it may just as well be a thoughtlessly written configuration.

Sakari mentioned that some parts of the dts are not needed and do not have proper documentation, yet.

As the camera works without them, remove them for now.

Signed-off-by Pavel Machek <[email protected]>

arch/arm/boot/dts/omap3-n900.dts +0 -11

@@ -144,15 +144,6 @@
 		io-channel-names = "temp", "bsi", "vbat";
 	};
 
-	rear_camera: camera at 0 {
-		compatible = "linux,camera";
-
-		module {
-			model = "TCM8341MD";
-			sensor = <&cam1>;
-		};
-	};
-
 	pwm9: dmtimer-pwm {
 		compatible = "ti,omap-dmtimer-pwm";
 		#pwm-cells = <3>;

@@ -189,10 +180,8 @@
 				clock-lanes = <1>;
 				data-lanes = <0>;
 				lane-polarity = <0 0>;
-				clock-inv = <0>;
 				/* Select strobe = <1> for back camera, <0> for front camera */
 				strobe = <1>;
-				crc = <0>;
 			};
 		};
 	};

Signature · 3 lines

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html