[v1 0/6] Qualcomm CCI & Camera for db410c & db845c

[v1 0/6] Qualcomm CCI & Camera for db410c & db845c

[Robert Foss]

This series depends on the Qualcom CCI I2C driver series:
https://patchwork.kernel.org/cover/10569957/

This series enables basic camera functionality on the Qualcomm db410c and
db845c (RB3) platforms.

This includes building drivers as modules, adding devicetree nodes
for camera controllers, clocks, regulators and sensor nodes.

Loic Poulain (2):
  arm64: dts: msm8916: Add i2c-qcom-cci node
  arm64: dts: apq8016-sbc: Add CCI/Sensor nodes

Robert Foss (4):
  arm64: dts: sdm845: Add i2c-qcom-cci node
  arm64: dts: sdm845-db845c: Add pm_8998 gpio names
  arm64: dts: sdm845-db845c: Add ov8856 & ov7251 camera nodes
  arm64: defconfig: Enable QCOM CAMCC, CAMSS and CCI drivers

 arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi  |  75 ++++++
 arch/arm64/boot/dts/qcom/msm8916.dtsi      |  27 ++
 arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 273 +++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi       | 110 +++++++++
 arch/arm64/configs/defconfig               |   4 +
 5 files changed, 489 insertions(+)

-- 
2.20.1

[v1 1/6] arm64: dts: msm8916: Add i2c-qcom-cci node

[Robert Foss]

From: Loic Poulain <loic.poulain@linaro.org>

The msm8916 CCI controller provides one CCI/I2C bus.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 9f31064f2374..afe1d73e5cd3 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -1503,6 +1503,33 @@
 			};
 		};
 
+		cci@1b0c000 {
+			compatible = "qcom,msm8916-cci";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x1b0c000 0x1000>;
+			interrupts = <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>;
+			clocks = <&gcc GCC_CAMSS_TOP_AHB_CLK>,
+				<&gcc GCC_CAMSS_CCI_AHB_CLK>,
+				<&gcc GCC_CAMSS_CCI_CLK>,
+				<&gcc GCC_CAMSS_AHB_CLK>;
+			clock-names = "camss_top_ahb", "cci_ahb",
+				      "cci", "camss_ahb";
+			assigned-clocks = <&gcc GCC_CAMSS_CCI_AHB_CLK>,
+					  <&gcc GCC_CAMSS_CCI_CLK>;
+			assigned-clock-rates = <80000000>, <19200000>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&cci0_default>;
+			status = "disabled";
+
+			cci0: i2c-bus@0 {
+				reg = <0>;
+				clock-frequency = <400000>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+
 		camss: camss@1b00000 {
 			compatible = "qcom,msm8916-camss";
 			reg = <0x1b0ac00 0x200>,
-- 
2.20.1

[v1 2/6] arm64: dts: apq8016-sbc: Add CCI/Sensor nodes

[Robert Foss]

From: Loic Poulain <loic.poulain@linaro.org>

Add cci device to msm8916.dtsi.
Add default 96boards camera node for db410c (apq8016-sbc).

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 75 +++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
index 037e26b3f8d5..a3e6982f4f93 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
@@ -495,6 +495,81 @@
 		wcnss@a21b000 {
 			status = "okay";
 		};
+
+		camera_vdddo_1v8: fixedregulator@0 {
+			compatible = "regulator-fixed";
+			regulator-name = "camera_vdddo";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-always-on;
+		};
+
+		camera_vdda_2v8: fixedregulator@1 {
+			compatible = "regulator-fixed";
+			regulator-name = "camera_vdda";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <2800000>;
+			regulator-always-on;
+		};
+
+		camera_vddd_1v5: fixedregulator@2 {
+			compatible = "regulator-fixed";
+			regulator-name = "camera_vddd";
+			regulator-min-microvolt = <1500000>;
+			regulator-max-microvolt = <1500000>;
+			regulator-always-on;
+		};
+
+		cci@1b0c000 {
+			status = "ok";
+			i2c-bus@0 {
+				camera_rear@3b {
+					compatible = "ovti,ov5640";
+					reg = <0x3b>;
+
+					enable-gpios = <&msmgpio 34 GPIO_ACTIVE_HIGH>;
+					reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>;
+					pinctrl-names = "default";
+					pinctrl-0 = <&camera_rear_default>;
+
+					clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
+					clock-names = "xclk";
+					clock-frequency = <23880000>;
+
+					vdddo-supply = <&camera_vdddo_1v8>;
+					vdda-supply = <&camera_vdda_2v8>;
+					vddd-supply = <&camera_vddd_1v5>;
+
+					/* No camera mezzanine by default */
+					status = "okay";
+
+					port {
+						ov5640_ep: endpoint {
+							clock-lanes = <1>;
+							data-lanes = <0 2>;
+							remote-endpoint = <&csiphy0_ep>;
+						};
+					};
+				};
+			};
+		};
+
+		camss@1b00000 {
+			status = "ok";
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				port@0 {
+					reg = <0>;
+					csiphy0_ep: endpoint {
+						clock-lanes = <1>;
+						data-lanes = <0 2>;
+						remote-endpoint = <&ov5640_ep>;
+						status = "okay";
+					};
+				};
+			};
+		};
 	};
 
 	usb2513 {
-- 
2.20.1

[v1 3/6] arm64: dts: sdm845: Add i2c-qcom-cci node

[Robert Foss]

The sdm845 SOC ships with a CCI controller, which
has two CCI/I2C buses.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845-db845c.dts |   4 +
 arch/arm64/boot/dts/qcom/sdm845.dtsi       | 110 +++++++++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index eb77aaa6a819..a6b6837c3d68 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -583,3 +583,7 @@
 		bias-pull-up;
 	};
 };
+
+&cci {
+	status = "ok";
+};
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index d42302b8889b..b7f5c0b0f6af 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -5,6 +5,7 @@
  * Copyright (c) 2018, The Linux Foundation. All rights reserved.
  */
 
+#include <dt-bindings/clock/qcom,camcc-sdm845.h>
 #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
 #include <dt-bindings/clock/qcom,gcc-sdm845.h>
 #include <dt-bindings/clock/qcom,gpucc-sdm845.h>
@@ -717,6 +718,14 @@
 			#power-domain-cells = <1>;
 		};
 
+		clock_camcc: clock-controller@ad00000 {
+			compatible = "qcom,sdm845-camcc";
+			reg = <0 0xad00000 0 0x10000>;
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+		};
+
 		qfprom@784000 {
 			compatible = "qcom,qfprom";
 			reg = <0 0x00784000 0 0x8ff>;
@@ -1451,6 +1460,60 @@
 			gpio-ranges = <&tlmm 0 0 150>;
 			wakeup-parent = <&pdc_intc>;
 
+			cci0_default: cci0_default {
+				/* SDA, SCL */
+				pinmux {
+					function = "cci_i2c";
+					pins = "gpio17", "gpio18";
+				};
+				pinconf {
+					pins = "gpio17", "gpio18";
+					bias-pull-up;
+					drive-strength = <2>; /* 2 mA */
+				};
+			};
+
+			cci0_sleep: cci0_sleep {
+				/* SDA, SCL */
+				mux {
+					pins = "gpio17", "gpio18";
+					function = "cci_i2c";
+				};
+
+				config {
+					pins = "gpio17", "gpio18";
+					drive-strength = <2>; /* 2 mA */
+					bias-pull-down;
+				};
+			};
+
+			cci1_default: cci1_default {
+				/* SDA, SCL */
+				pinmux {
+					function = "cci_i2c";
+					pins = "gpio19", "gpio20";
+				};
+				pinconf {
+					pins = "gpio19", "gpio20";
+					bias-pull-up;
+					drive-strength = <2>; /* 2 mA */
+				};
+			};
+
+			cci1_sleep: cci1_sleep {
+				/* SDA, SCL */
+				mux {
+					pins = "gpio19", "gpio20";
+					function = "cci_i2c";
+				};
+
+				config {
+					pins = "gpio19", "gpio20";
+					drive-strength = <2>; /* 2 mA */
+					bias-pull-down;
+				};
+			};
+
 			qspi_clk: qspi-clk {
 				pinmux {
 					pins = "gpio95";
@@ -2608,6 +2671,53 @@
 			#reset-cells = <1>;
 		};
 
+		cci: cci@ac4a000 {
+			compatible = "qcom,sdm845-cci";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			reg = <0 0xac4a000 0 0x4000>;
+			interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
+			power-domains = <&clock_camcc TITAN_TOP_GDSC>;
+
+			clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
+				<&clock_camcc CAM_CC_SOC_AHB_CLK>,
+				<&clock_camcc CAM_CC_SLOW_AHB_CLK_SRC>,
+				<&clock_camcc CAM_CC_CPAS_AHB_CLK>,
+				<&clock_camcc CAM_CC_CCI_CLK>,
+				<&clock_camcc CAM_CC_CCI_CLK_SRC>;
+			clock-names = "camnoc_axi_clk",
+				"soc_ahb_clk",
+				"slow_ahb_src_clk",
+				"cpas_ahb_clk",
+				"cci",
+				"cci_clk_src";
+
+			assigned-clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
+				<&clock_camcc CAM_CC_CCI_CLK>;
+			assigned-clock-rates = <80000000>, <37500000>;
+
+			pinctrl-names = "default", "sleep";
+			pinctrl-0 = <&cci0_default &cci1_default>;
+			pinctrl-1 = <&cci0_sleep &cci1_sleep>;
+
+			status = "disabled";
+
+			i2c-bus@0 {
+				reg = <0>;
+				clock-frequency = <1000000>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			i2c-bus@1 {
+				reg = <1>;
+				clock-frequency = <1000000>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+
 		mdss: mdss@ae00000 {
 			compatible = "qcom,sdm845-mdss";
 			reg = <0 0x0ae00000 0 0x1000>;
-- 
2.20.1

[v1 4/6] arm64: dts: sdm845-db845c: Add pm_8998 gpio names

[Robert Foss]

Add pm_8998 GPIO trace names. These names are defined in
the 96boards db845c mezzanine schematic.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index a6b6837c3d68..e8c056d02ace 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -584,6 +584,36 @@
 	};
 };
 
+&pm8998_gpio {
+	gpio-line-names =
+		"NC",
+		"NC",
+		"WLAN_SW_CTRL",
+		"NC",
+		"PM_GPIO5_BLUE_BT_LED",
+		"VOL_UP_N",
+		"NC",
+		"ADC_IN1",
+		"PM_GPIO9_YEL_WIFI_LED",
+		"CAM0_AVDD_EN",
+		"NC",
+		"CAM0_DVDD_EN",
+		"PM_GPIO13_GREEN_U4_LED",
+		"DIV_CLK2",
+		"NC",
+		"NC",
+		"NC",
+		"SMB_STAT",
+		"NC",
+		"NC",
+		"ADC_IN2",
+		"OPTION1",
+		"WCSS_PWR_REQ",
+		"PM845_GPIO24",
+		"OPTION2",
+		"PM845_SLB";
+};
+
 &cci {
 	status = "ok";
 };
-- 
2.20.1

[v1 5/6] arm64: dts: sdm845-db845c: Add ov8856 & ov7251 camera nodes

[Robert Foss]

Enable the ov8856 main camera and the ov7251 b/w tracking camera
used on the Qualcomm RB3 kit.

Currently the camera nodes have not yet been attached to an to a
CSI2 endpoint, since no driver currently supports the ISP that the the
SDM845/db845c ships with.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 239 +++++++++++++++++++++
 1 file changed, 239 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index e8c056d02ace..660550197ce9 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -110,6 +110,53 @@
 		// enable-active-high;
 	};
 
+	cam0_dvdd_1v2: reg_cam0_dvdd_1v2 {
+		compatible = "regulator-fixed";
+		regulator-name = "CAM0_DVDD_1V2";
+		regulator-min-microvolt = <1200000>;
+		regulator-max-microvolt = <1200000>;
+		enable-active-high;
+		gpio = <&pm8998_gpio 12 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&cam0_dvdd_1v2_en_default>;
+		vin-supply = <&vbat>;
+	};
+
+	cam0_avdd_2v8: reg_cam0_avdd_2v8 {
+		compatible = "regulator-fixed";
+		regulator-name = "CAM0_AVDD_2V8";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		enable-active-high;
+		gpio = <&pm8998_gpio 10 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&cam0_avdd_2v8_en_default>;
+		vin-supply = <&vbat>;
+	};
+
+	/* This regulator is enabled when the VREG_LVS1A_1P8 trace is enabled */
+	cam3_avdd_2v8: reg_cam3_avdd_2v8 {
+		compatible = "regulator-fixed";
+		regulator-name = "CAM3_AVDD_2V8";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		regulator-always-on;
+		vin-supply = <&vbat>;
+	};
+
+	/* This regulator does not really exits, but a 'vddd-supply' is
+	 * required for the ov7251 driver, but no 'vddd' regulator is used
+	 * in the schematic
+	 */
+	cam3_vddd_1v2: reg_cam3_vddd_1v2 {
+		compatible = "regulator-fixed";
+		regulator-name = "CAM3_VDDD_1V2_DUMMY";
+		regulator-min-microvolt = <1200000>;
+		regulator-max-microvolt = <1200000>;
+		regulator-always-on;
+		vin-supply = <&vbat>;
+	};
+
 	pcie0_3p3v_dual: vldo-3v3-regulator {
 		compatible = "regulator-fixed";
 		regulator-name = "VLDO_3V3";
@@ -406,6 +453,81 @@
 };
 
 &tlmm {
+	pcie0_default_state: pcie0-default {
+		clkreq {
+			pins = "gpio36";
+			function = "pci_e0";
+			bias-pull-up;
+		};
+
+		reset-n {
+			pins = "gpio35";
+			function = "gpio";
+
+			drive-strength = <2>;
+			output-low;
+			bias-pull-down;
+		};
+
+		wake-n {
+			pins = "gpio37";
+			function = "gpio";
+
+			drive-strength = <2>;
+			bias-pull-up;
+		};
+	};
+
+	cam0_default: cam0_default {
+		mux_rst {
+			function = "gpio";
+			pins = "gpio9";
+		};
+		config_rst {
+			pins = "gpio9";
+			drive-strength = <16>;
+			bias-disable;
+		};
+
+		mux_mclk0 {
+			function = "cam_mclk";
+			pins = "gpio13";
+		};
+		config_mclk0 {
+			pins = "gpio13";
+			drive-strength = <16>;
+			bias-disable;
+		};
+	};
+
+	cam3_default: cam3_default {
+			mux_rst {
+				function = "gpio";
+				pins = "gpio21";
+			};
+			config_rst {
+				pins = "gpio21";
+				drive-strength = <16>;
+				bias-disable;
+			};
+
+			mux_mclk3 {
+				function = "cam_mclk";
+				pins = "gpio16";
+			};
+			config_mclk3 {
+				pins = "gpio16";
+				drive-strength = <16>;
+				bias-disable;
+			};
+	};
+
+	lt9611_irq_pin: lt9611-irq {
+		pins = "gpio84";
+		function = "gpio";
+		bias-disable;
+	};
+
 	pcie0_pwren_state: pcie0-pwren {
 		pins = "gpio90";
 		function = "gpio";
@@ -612,8 +734,125 @@
 		"PM845_GPIO24",
 		"OPTION2",
 		"PM845_SLB";
+
+	cam0_dvdd_1v2_en_default: cam0_dvdd_1v2_en_pinctrl {
+		pins = "gpio12";
+		function = "normal";
+
+		bias-pull-up;
+		drive-push-pull;
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+	};
+
+	cam0_avdd_2v8_en_default: cam0_avdd_2v8_en_pinctrl {
+		pins = "gpio10";
+		function = "normal";
+
+		bias-pull-up;
+		drive-push-pull;
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+	};
 };
 
 &cci {
 	status = "ok";
+
+	i2c-bus@0 {
+		cam0@10 {
+			compatible = "ovti,ov8856";
+
+			/* The Qualcomm RB3 camera mezzanine schematic lists
+			 * 0x20 as I2C address of this device, but the Linux
+			 * kernel documentation lists 0x10 I2C address.
+			 */
+			reg = <0x10>;
+
+			// CAM0_RST_N
+			reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&cam0_default>;
+			gpios = <&tlmm 13 0>,
+				<&tlmm 9 0>;
+
+			clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
+			clock-names = "xvclk";
+			clock-frequency = <19200000>;
+
+
+			/* The &vreg_s4a_1p8 trace is powered on as a
+			 * part of the TITAN_TOP_GDSC power domain.
+			 * So it is represented by a fixed regulator.
+			 *
+			 * The 2.8V vdda-supply and 1.2V vddd-supply regulators
+			 * both have to be enabled through the power management
+			 * gpios.
+			 */
+			power-domains = <&clock_camcc TITAN_TOP_GDSC>;
+
+			dovdd-supply = <&vreg_lvs1a_1p8>;
+			avdd-supply = <&cam0_avdd_2v8>;
+			dvdd-supply = <&cam0_dvdd_1v2>;
+
+			/* No camera mezzanine by default */
+			status = "ok";
+
+			port {
+				ov8856_ep: endpoint {
+					clock-lanes = <1>;
+					link-frequencies = /bits/ 64
+						<360000000 180000000>;
+					data-lanes = <1 2 3 4>;
+//					remote-endpoint = <&csiphy0_ep>;
+				};
+			};
+		};
+	};
+
+	i2c-bus@1 {
+		cam3@60 {
+			compatible = "ovti,ov7251";
+
+			// I2C address as per ov7251.txt linux documentation
+			reg = <0x60>;
+
+			// CAM3_RST_N
+			enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&cam3_default>;
+			gpios = <&tlmm 16 0>,
+				<&tlmm 21 0>;
+
+			clocks = <&clock_camcc CAM_CC_MCLK3_CLK>;
+			clock-names = "xclk";
+			clock-frequency = <24000000>;
+
+			/* The &vreg_s4a_1p8 trace is powered on as a
+			 * part of the TITAN_TOP_GDSC power domain.
+			 * So it is represented by a fixed regulator.
+			 *
+			 * The 2.8V vdda-supply regulator is enabled when the
+			 * vreg_s4a_1p8 trace is pulled high.
+			 * It too is represented by a fixed regulator.
+			 *
+			 * No 1.2V vddd-supply regulator is used, a fixed
+			 * regulator represents it.
+			 */
+			power-domains = <&clock_camcc TITAN_TOP_GDSC>;
+
+			vdddo-supply = <&vreg_lvs1a_1p8>;
+			vdda-supply = <&cam3_avdd_2v8>;
+			vddd-supply = <&cam3_vddd_1v2>;
+
+			/* No camera mezzanine by default */
+			status = "ok";
+
+			port {
+				ov7251_ep: endpoint {
+					clock-lanes = <1>;
+					data-lanes = <0 1>;
+//					remote-endpoint = <&csiphy3_ep>;
+				};
+			};
+		};
+	};
 };
-- 
2.20.1

[v1 6/6] arm64: defconfig: Enable QCOM CAMCC, CAMSS and CCI drivers

[Robert Foss]

Build camera clock, isp and controller drivers as modules.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 arch/arm64/configs/defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 4db223dbc549..7cb6989249ab 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -376,6 +376,7 @@ CONFIG_I2C_MESON=y
 CONFIG_I2C_MV64XXX=y
 CONFIG_I2C_OWL=y
 CONFIG_I2C_PXA=y
+CONFIG_I2C_QCOM_CCI=m
 CONFIG_I2C_QCOM_GENI=m
 CONFIG_I2C_QUP=y
 CONFIG_I2C_RK3X=y
@@ -530,6 +531,7 @@ CONFIG_VIDEO_SAMSUNG_S5P_MFC=m
 CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC=m
 CONFIG_VIDEO_RENESAS_FCP=m
 CONFIG_VIDEO_RENESAS_VSP1=m
+CONFIG_VIDEO_QCOM_CAMSS=m
 CONFIG_DRM=m
 CONFIG_DRM_I2C_NXP_TDA998X=m
 CONFIG_DRM_NOUVEAU=m
@@ -732,6 +734,7 @@ CONFIG_MSM_GCC_8994=y
 CONFIG_MSM_MMCC_8996=y
 CONFIG_MSM_GCC_8998=y
 CONFIG_QCS_GCC_404=y
+CONFIG_SDM_CAMCC_845=m
 CONFIG_SDM_GCC_845=y
 CONFIG_SM_GCC_8150=y
 CONFIG_QCOM_HFPLL=y
@@ -762,6 +765,7 @@ CONFIG_QCOM_COMMAND_DB=y
 CONFIG_QCOM_GENI_SE=y
 CONFIG_QCOM_GLINK_SSR=m
 CONFIG_QCOM_RMTFS_MEM=m
+CONFIG_SDM_CAMCC_845=m
 CONFIG_QCOM_RPMH=y
 CONFIG_QCOM_RPMHPD=y
 CONFIG_QCOM_SMEM=y
-- 
2.20.1

Re: [v1 1/6] arm64: dts: msm8916: Add i2c-qcom-cci node

[Bjorn Andersson]

On Wed 11 Mar 05:34 PDT 2020, Robert Foss wrote:

> From: Loic Poulain <loic.poulain@linaro.org>
> 
> The msm8916 CCI controller provides one CCI/I2C bus.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 9f31064f2374..afe1d73e5cd3 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -1503,6 +1503,33 @@
>  			};
>  		};
>  
> +		cci@1b0c000 {

This deserves a label, so that it's possible to reference it and alter
the status in the board dts.

> +			compatible = "qcom,msm8916-cci";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x1b0c000 0x1000>;

Please sort nodes my address (and then by name).

Apart from these two nits, this looks good.

Regards,
Bjorn

> +			interrupts = <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>;
> +			clocks = <&gcc GCC_CAMSS_TOP_AHB_CLK>,
> +				<&gcc GCC_CAMSS_CCI_AHB_CLK>,
> +				<&gcc GCC_CAMSS_CCI_CLK>,
> +				<&gcc GCC_CAMSS_AHB_CLK>;
> +			clock-names = "camss_top_ahb", "cci_ahb",
> +				      "cci", "camss_ahb";
> +			assigned-clocks = <&gcc GCC_CAMSS_CCI_AHB_CLK>,
> +					  <&gcc GCC_CAMSS_CCI_CLK>;
> +			assigned-clock-rates = <80000000>, <19200000>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&cci0_default>;
> +			status = "disabled";
> +
> +			cci0: i2c-bus@0 {
> +				reg = <0>;
> +				clock-frequency = <400000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +
>  		camss: camss@1b00000 {
>  			compatible = "qcom,msm8916-camss";
>  			reg = <0x1b0ac00 0x200>,
> -- 
> 2.20.1
> 

Re: [v1 2/6] arm64: dts: apq8016-sbc: Add CCI/Sensor nodes

[Bjorn Andersson]

On Wed 11 Mar 05:34 PDT 2020, Robert Foss wrote:

> From: Loic Poulain <loic.poulain@linaro.org>
> 
> Add cci device to msm8916.dtsi.
> Add default 96boards camera node for db410c (apq8016-sbc).
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 75 +++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> index 037e26b3f8d5..a3e6982f4f93 100644
> --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> @@ -495,6 +495,81 @@
>  		wcnss@a21b000 {
>  			status = "okay";
>  		};
> +
> +		camera_vdddo_1v8: fixedregulator@0 {

While "fixedregulator" is a seemingly good name, you're not allows to
use a unit address on the node if there's no address information in the
node. So you need to give these nodes a non-generic name.


And please move nodes without a reg (i.e. not on an mmio bus) out of
/soc, i.e. place it near /chosen.

> +			compatible = "regulator-fixed";
> +			regulator-name = "camera_vdddo";
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +			regulator-always-on;
> +		};
> +
> +		camera_vdda_2v8: fixedregulator@1 {
> +			compatible = "regulator-fixed";
> +			regulator-name = "camera_vdda";
> +			regulator-min-microvolt = <2800000>;
> +			regulator-max-microvolt = <2800000>;
> +			regulator-always-on;
> +		};
> +
> +		camera_vddd_1v5: fixedregulator@2 {
> +			compatible = "regulator-fixed";
> +			regulator-name = "camera_vddd";
> +			regulator-min-microvolt = <1500000>;
> +			regulator-max-microvolt = <1500000>;
> +			regulator-always-on;
> +		};
> +
> +		cci@1b0c000 {

Please ensure that cci and camss have labels and reference them by &cci
and &camss below the / {}, sorted by label name.

> +			status = "ok";
> +			i2c-bus@0 {

Please reference this by its label as well.

> +				camera_rear@3b {
> +					compatible = "ovti,ov5640";
> +					reg = <0x3b>;
> +
> +					enable-gpios = <&msmgpio 34 GPIO_ACTIVE_HIGH>;
> +					reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>;
> +					pinctrl-names = "default";
> +					pinctrl-0 = <&camera_rear_default>;
> +
> +					clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
> +					clock-names = "xclk";
> +					clock-frequency = <23880000>;
> +
> +					vdddo-supply = <&camera_vdddo_1v8>;
> +					vdda-supply = <&camera_vdda_2v8>;
> +					vddd-supply = <&camera_vddd_1v5>;
> +
> +					/* No camera mezzanine by default */

This comment gives me the feeling that this node should have been status
disabled, please confirm.

Regards,
Bjorn

> +					status = "okay";
> +
> +					port {
> +						ov5640_ep: endpoint {
> +							clock-lanes = <1>;
> +							data-lanes = <0 2>;
> +							remote-endpoint = <&csiphy0_ep>;
> +						};
> +					};
> +				};
> +			};
> +		};
> +
> +		camss@1b00000 {
> +			status = "ok";
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				port@0 {
> +					reg = <0>;
> +					csiphy0_ep: endpoint {
> +						clock-lanes = <1>;
> +						data-lanes = <0 2>;
> +						remote-endpoint = <&ov5640_ep>;
> +						status = "okay";
> +					};
> +				};
> +			};
> +		};
>  	};
>  
>  	usb2513 {
> -- 
> 2.20.1
> 

Re: [v1 3/6] arm64: dts: sdm845: Add i2c-qcom-cci node

[Bjorn Andersson]

On Wed 11 Mar 05:34 PDT 2020, Robert Foss wrote:

> The sdm845 SOC ships with a CCI controller, which
> has two CCI/I2C buses.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-db845c.dts |   4 +
>  arch/arm64/boot/dts/qcom/sdm845.dtsi       | 110 +++++++++++++++++++++
>  2 files changed, 114 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> index eb77aaa6a819..a6b6837c3d68 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> @@ -583,3 +583,7 @@
>  		bias-pull-up;
>  	};
>  };
> +
> +&cci {
> +	status = "ok";
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index d42302b8889b..b7f5c0b0f6af 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -5,6 +5,7 @@
>   * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>   */
>  
> +#include <dt-bindings/clock/qcom,camcc-sdm845.h>
>  #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
>  #include <dt-bindings/clock/qcom,gcc-sdm845.h>
>  #include <dt-bindings/clock/qcom,gpucc-sdm845.h>
> @@ -717,6 +718,14 @@
>  			#power-domain-cells = <1>;
>  		};
>  
> +		clock_camcc: clock-controller@ad00000 {
> +			compatible = "qcom,sdm845-camcc";
> +			reg = <0 0xad00000 0 0x10000>;

Please pad address (i.e. the second cell) to 8 digits and maintain sort
order by address.

> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +		};
> +
>  		qfprom@784000 {
>  			compatible = "qcom,qfprom";
>  			reg = <0 0x00784000 0 0x8ff>;
> @@ -1451,6 +1460,60 @@
>  			gpio-ranges = <&tlmm 0 0 150>;
>  			wakeup-parent = <&pdc_intc>;
>  
> +			cci0_default: cci0_default {

No _ in the node name (i.e you can do cci0_default: cci0-default).

> +				/* SDA, SCL */
> +				pinmux {

You no longer need this intermediate node, instead you can write this
as:

cci0_default: cci0-default {
	pins = "gpio17", "gpio18";
	function = "cci_i2c";
	
	bias-pull-up;
	drive-strength = <2>;
};

Or alternatively if you would like to group things in subnodes, do so by
pin (to allow different pinconf per pin in a nice way), i.e:

cci0_default: cci0-default {
	sda {
		pins = "gpio17";
		function = "cci_i2c";
		
		bias-pull-up;
		drive-strength = <2>;
	};

	scl {
		pins = "gpio18";
		function = "cci_i2c";
		
		bias-pull-up;
		drive-strength = <2>;
	};
};

> +					function = "cci_i2c";
> +					pins = "gpio17", "gpio18";
> +				};
> +				pinconf {
> +					pins = "gpio17", "gpio18";
> +					bias-pull-up;
> +					drive-strength = <2>; /* 2 mA */
> +				};
> +			};
> +
> +			cci0_sleep: cci0_sleep {
> +				/* SDA, SCL */
> +				mux {
> +					pins = "gpio17", "gpio18";
> +					function = "cci_i2c";
> +				};
> +
> +				config {
> +					pins = "gpio17", "gpio18";
> +					drive-strength = <2>; /* 2 mA */
> +					bias-pull-down;
> +				};
> +			};
> +
> +			cci1_default: cci1_default {
> +				/* SDA, SCL */
> +				pinmux {
> +					function = "cci_i2c";
> +					pins = "gpio19", "gpio20";
> +				};
> +				pinconf {
> +					pins = "gpio19", "gpio20";
> +					bias-pull-up;
> +					drive-strength = <2>; /* 2 mA */
> +				};
> +			};
> +
> +			cci1_sleep: cci1_sleep {
> +				/* SDA, SCL */
> +				mux {
> +					pins = "gpio19", "gpio20";
> +					function = "cci_i2c";
> +				};
> +
> +				config {
> +					pins = "gpio19", "gpio20";
> +					drive-strength = <2>; /* 2 mA */
> +					bias-pull-down;
> +				};
> +			};
> +
>  			qspi_clk: qspi-clk {
>  				pinmux {
>  					pins = "gpio95";
> @@ -2608,6 +2671,53 @@
>  			#reset-cells = <1>;
>  		};
>  
> +		cci: cci@ac4a000 {
> +			compatible = "qcom,sdm845-cci";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			reg = <0 0xac4a000 0 0x4000>;

Please pad 0xac4a000 to 8 digits.

> +			interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
> +			power-domains = <&clock_camcc TITAN_TOP_GDSC>;
> +
> +			clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
> +				<&clock_camcc CAM_CC_SOC_AHB_CLK>,
> +				<&clock_camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> +				<&clock_camcc CAM_CC_CPAS_AHB_CLK>,
> +				<&clock_camcc CAM_CC_CCI_CLK>,
> +				<&clock_camcc CAM_CC_CCI_CLK_SRC>;
> +			clock-names = "camnoc_axi_clk",
> +				"soc_ahb_clk",
> +				"slow_ahb_src_clk",
> +				"cpas_ahb_clk",
> +				"cci",
> +				"cci_clk_src";

Please drop the "_clk" suffix from these (iirc, these strings aren't
significant to the binding anyways).

> +
> +			assigned-clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
> +				<&clock_camcc CAM_CC_CCI_CLK>;
> +			assigned-clock-rates = <80000000>, <37500000>;
> +
> +			pinctrl-names = "default", "sleep";
> +			pinctrl-0 = <&cci0_default &cci1_default>;
> +			pinctrl-1 = <&cci0_sleep &cci1_sleep>;
> +
> +			status = "disabled";
> +
> +			i2c-bus@0 {

Please give these labels, to make it easy to reference each bus in the
board dts and to add children.

Regards,
Bjorn

> +				reg = <0>;
> +				clock-frequency = <1000000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +
> +			i2c-bus@1 {
> +				reg = <1>;
> +				clock-frequency = <1000000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +
>  		mdss: mdss@ae00000 {
>  			compatible = "qcom,sdm845-mdss";
>  			reg = <0 0x0ae00000 0 0x1000>;
> -- 
> 2.20.1
> 

Re: [v1 4/6] arm64: dts: sdm845-db845c: Add pm_8998 gpio names

[Bjorn Andersson]

On Wed 11 Mar 05:34 PDT 2020, Robert Foss wrote:

> Add pm_8998 GPIO trace names. These names are defined in
> the 96boards db845c mezzanine schematic.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> index a6b6837c3d68..e8c056d02ace 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> @@ -584,6 +584,36 @@
>  	};
>  };
>  
> +&pm8998_gpio {

Please sort nodes alphabetically by label.

Regards,
Bjorn

> +	gpio-line-names =
> +		"NC",
> +		"NC",
> +		"WLAN_SW_CTRL",
> +		"NC",
> +		"PM_GPIO5_BLUE_BT_LED",
> +		"VOL_UP_N",
> +		"NC",
> +		"ADC_IN1",
> +		"PM_GPIO9_YEL_WIFI_LED",
> +		"CAM0_AVDD_EN",
> +		"NC",
> +		"CAM0_DVDD_EN",
> +		"PM_GPIO13_GREEN_U4_LED",
> +		"DIV_CLK2",
> +		"NC",
> +		"NC",
> +		"NC",
> +		"SMB_STAT",
> +		"NC",
> +		"NC",
> +		"ADC_IN2",
> +		"OPTION1",
> +		"WCSS_PWR_REQ",
> +		"PM845_GPIO24",
> +		"OPTION2",
> +		"PM845_SLB";
> +};
> +
>  &cci {
>  	status = "ok";
>  };
> -- 
> 2.20.1
> 

Re: [v1 5/6] arm64: dts: sdm845-db845c: Add ov8856 & ov7251 camera nodes

[Bjorn Andersson]

On Wed 11 Mar 05:35 PDT 2020, Robert Foss wrote:

> Enable the ov8856 main camera and the ov7251 b/w tracking camera
> used on the Qualcomm RB3 kit.
> 
> Currently the camera nodes have not yet been attached to an to a
> CSI2 endpoint, since no driver currently supports the ISP that the the
> SDM845/db845c ships with.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 239 +++++++++++++++++++++
>  1 file changed, 239 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> index e8c056d02ace..660550197ce9 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> @@ -110,6 +110,53 @@
>  		// enable-active-high;
>  	};
>  
> +	cam0_dvdd_1v2: reg_cam0_dvdd_1v2 {

cam0_dvdd_1v2: cam0-dvdd-1v2 {

> +		compatible = "regulator-fixed";
> +		regulator-name = "CAM0_DVDD_1V2";
> +		regulator-min-microvolt = <1200000>;
> +		regulator-max-microvolt = <1200000>;
> +		enable-active-high;
> +		gpio = <&pm8998_gpio 12 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&cam0_dvdd_1v2_en_default>;
> +		vin-supply = <&vbat>;
> +	};
> +
> +	cam0_avdd_2v8: reg_cam0_avdd_2v8 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "CAM0_AVDD_2V8";
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <2800000>;
> +		enable-active-high;
> +		gpio = <&pm8998_gpio 10 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&cam0_avdd_2v8_en_default>;
> +		vin-supply = <&vbat>;
> +	};
> +
> +	/* This regulator is enabled when the VREG_LVS1A_1P8 trace is enabled */
> +	cam3_avdd_2v8: reg_cam3_avdd_2v8 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "CAM3_AVDD_2V8";
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <2800000>;
> +		regulator-always-on;
> +		vin-supply = <&vbat>;
> +	};
> +
> +	/* This regulator does not really exits, but a 'vddd-supply' is
> +	 * required for the ov7251 driver, but no 'vddd' regulator is used
> +	 * in the schematic
> +	 */

Looking at the driver you should be able to just omit vddd-supply from
the DT node, in which case the driver will get a dummy regulator and
should function properly.

Presumably you can then skip defining this dummy regulator as well.

> +	cam3_vddd_1v2: reg_cam3_vddd_1v2 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "CAM3_VDDD_1V2_DUMMY";
> +		regulator-min-microvolt = <1200000>;
> +		regulator-max-microvolt = <1200000>;
> +		regulator-always-on;
> +		vin-supply = <&vbat>;
> +	};
> +
>  	pcie0_3p3v_dual: vldo-3v3-regulator {
>  		compatible = "regulator-fixed";
>  		regulator-name = "VLDO_3V3";
> @@ -406,6 +453,81 @@
>  };
>  
>  &tlmm {
> +	pcie0_default_state: pcie0-default {
> +		clkreq {
> +			pins = "gpio36";
> +			function = "pci_e0";
> +			bias-pull-up;
> +		};
> +
> +		reset-n {
> +			pins = "gpio35";
> +			function = "gpio";
> +
> +			drive-strength = <2>;
> +			output-low;
> +			bias-pull-down;
> +		};
> +
> +		wake-n {
> +			pins = "gpio37";
> +			function = "gpio";
> +
> +			drive-strength = <2>;
> +			bias-pull-up;
> +		};
> +	};

This looks like leftovers from your workspace.

> +
> +	cam0_default: cam0_default {
> +		mux_rst {

Please combine *_rst into one "rst" subnode and *_mclk0 into a mclk {}.

> +			function = "gpio";
> +			pins = "gpio9";
> +		};
> +		config_rst {
> +			pins = "gpio9";
> +			drive-strength = <16>;
> +			bias-disable;
> +		};
> +
> +		mux_mclk0 {
> +			function = "cam_mclk";
> +			pins = "gpio13";
> +		};
> +		config_mclk0 {
> +			pins = "gpio13";
> +			drive-strength = <16>;
> +			bias-disable;
> +		};
> +	};
> +
> +	cam3_default: cam3_default {

Ditto.

Also, please check the indentation of this block.

> +			mux_rst {
> +				function = "gpio";
> +				pins = "gpio21";
> +			};
> +			config_rst {
> +				pins = "gpio21";
> +				drive-strength = <16>;
> +				bias-disable;
> +			};
> +
> +			mux_mclk3 {
> +				function = "cam_mclk";
> +				pins = "gpio16";
> +			};
> +			config_mclk3 {
> +				pins = "gpio16";
> +				drive-strength = <16>;
> +				bias-disable;
> +			};
> +	};
> +
> +	lt9611_irq_pin: lt9611-irq {
> +		pins = "gpio84";
> +		function = "gpio";
> +		bias-disable;
> +	};

This node shouldn't be here either.

> +
>  	pcie0_pwren_state: pcie0-pwren {
>  		pins = "gpio90";
>  		function = "gpio";
> @@ -612,8 +734,125 @@
>  		"PM845_GPIO24",
>  		"OPTION2",
>  		"PM845_SLB";
> +
> +	cam0_dvdd_1v2_en_default: cam0_dvdd_1v2_en_pinctrl {

Use '-' in the node name, and you can drop the _pinctrl suffix, given
that the name only has to be unique in this parent node.

> +		pins = "gpio12";
> +		function = "normal";
> +
> +		bias-pull-up;
> +		drive-push-pull;
> +		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> +	};
> +
> +	cam0_avdd_2v8_en_default: cam0_avdd_2v8_en_pinctrl {
> +		pins = "gpio10";
> +		function = "normal";
> +
> +		bias-pull-up;
> +		drive-push-pull;
> +		qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> +	};
>  };
>  
>  &cci {
>  	status = "ok";
> +
> +	i2c-bus@0 {

Please reference this by label instead.

> +		cam0@10 {

camera@10

> +			compatible = "ovti,ov8856";
> +
> +			/* The Qualcomm RB3 camera mezzanine schematic lists
> +			 * 0x20 as I2C address of this device, but the Linux
> +			 * kernel documentation lists 0x10 I2C address.
> +			 */

This is a  normal discrepancy in how different people lists
addresses. Feel free to omit this comment.

> +			reg = <0x10>;
> +
> +			// CAM0_RST_N
> +			reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&cam0_default>;
> +			gpios = <&tlmm 13 0>,
> +				<&tlmm 9 0>;

s/0/GPIO_ACTIVE_HIGH/g

> +
> +			clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
> +			clock-names = "xvclk";
> +			clock-frequency = <19200000>;
> +
> +

Extra newline.

> +			/* The &vreg_s4a_1p8 trace is powered on as a
> +			 * part of the TITAN_TOP_GDSC power domain.

Rather vreg_s4a_1p8 is simply always on, unrelated to TITAN_TOP_GDSC.
The GDSC is likely to control the power to the CCI controller itself
though.

> +			 * So it is represented by a fixed regulator.
> +			 *
> +			 * The 2.8V vdda-supply and 1.2V vddd-supply regulators
> +			 * both have to be enabled through the power management
> +			 * gpios.
> +			 */
> +			power-domains = <&clock_camcc TITAN_TOP_GDSC>;
> +
> +			dovdd-supply = <&vreg_lvs1a_1p8>;
> +			avdd-supply = <&cam0_avdd_2v8>;
> +			dvdd-supply = <&cam0_dvdd_1v2>;
> +
> +			/* No camera mezzanine by default */

I think it's fine to assume that everyone has their mezzanine mounted on
their db845c, so feel free to omit this comment and the status below.

(Assuming that not having the camera connected will be handled somewhat
gracefully)


Given though that we're lacking the rest of the camera subsystem it
might be suitable to status = "disable" this for now.

> +			status = "ok";
> +
> +			port {
> +				ov8856_ep: endpoint {
> +					clock-lanes = <1>;
> +					link-frequencies = /bits/ 64
> +						<360000000 180000000>;
> +					data-lanes = <1 2 3 4>;
> +//					remote-endpoint = <&csiphy0_ep>;
> +				};
> +			};
> +		};
> +	};
> +
> +	i2c-bus@1 {

Same comments as for the first bus...

Regards,
Bjorn

> +		cam3@60 {
> +			compatible = "ovti,ov7251";
> +
> +			// I2C address as per ov7251.txt linux documentation
> +			reg = <0x60>;
> +
> +			// CAM3_RST_N
> +			enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&cam3_default>;
> +			gpios = <&tlmm 16 0>,
> +				<&tlmm 21 0>;
> +
> +			clocks = <&clock_camcc CAM_CC_MCLK3_CLK>;
> +			clock-names = "xclk";
> +			clock-frequency = <24000000>;
> +
> +			/* The &vreg_s4a_1p8 trace is powered on as a
> +			 * part of the TITAN_TOP_GDSC power domain.
> +			 * So it is represented by a fixed regulator.
> +			 *
> +			 * The 2.8V vdda-supply regulator is enabled when the
> +			 * vreg_s4a_1p8 trace is pulled high.
> +			 * It too is represented by a fixed regulator.
> +			 *
> +			 * No 1.2V vddd-supply regulator is used, a fixed
> +			 * regulator represents it.
> +			 */
> +			power-domains = <&clock_camcc TITAN_TOP_GDSC>;
> +
> +			vdddo-supply = <&vreg_lvs1a_1p8>;
> +			vdda-supply = <&cam3_avdd_2v8>;
> +			vddd-supply = <&cam3_vddd_1v2>;
> +
> +			/* No camera mezzanine by default */
> +			status = "ok";
> +
> +			port {
> +				ov7251_ep: endpoint {
> +					clock-lanes = <1>;
> +					data-lanes = <0 1>;
> +//					remote-endpoint = <&csiphy3_ep>;
> +				};
> +			};
> +		};
> +	};
>  };
> -- 
> 2.20.1
> 

Re: [v1 6/6] arm64: defconfig: Enable QCOM CAMCC, CAMSS and CCI drivers

[Bjorn Andersson]

On Wed 11 Mar 05:35 PDT 2020, Robert Foss wrote:

> Build camera clock, isp and controller drivers as modules.
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


Thanks for the series Robert!

Regards,
Bjorn

> ---
>  arch/arm64/configs/defconfig | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 4db223dbc549..7cb6989249ab 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -376,6 +376,7 @@ CONFIG_I2C_MESON=y
>  CONFIG_I2C_MV64XXX=y
>  CONFIG_I2C_OWL=y
>  CONFIG_I2C_PXA=y
> +CONFIG_I2C_QCOM_CCI=m
>  CONFIG_I2C_QCOM_GENI=m
>  CONFIG_I2C_QUP=y
>  CONFIG_I2C_RK3X=y
> @@ -530,6 +531,7 @@ CONFIG_VIDEO_SAMSUNG_S5P_MFC=m
>  CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC=m
>  CONFIG_VIDEO_RENESAS_FCP=m
>  CONFIG_VIDEO_RENESAS_VSP1=m
> +CONFIG_VIDEO_QCOM_CAMSS=m
>  CONFIG_DRM=m
>  CONFIG_DRM_I2C_NXP_TDA998X=m
>  CONFIG_DRM_NOUVEAU=m
> @@ -732,6 +734,7 @@ CONFIG_MSM_GCC_8994=y
>  CONFIG_MSM_MMCC_8996=y
>  CONFIG_MSM_GCC_8998=y
>  CONFIG_QCS_GCC_404=y
> +CONFIG_SDM_CAMCC_845=m
>  CONFIG_SDM_GCC_845=y
>  CONFIG_SM_GCC_8150=y
>  CONFIG_QCOM_HFPLL=y
> @@ -762,6 +765,7 @@ CONFIG_QCOM_COMMAND_DB=y
>  CONFIG_QCOM_GENI_SE=y
>  CONFIG_QCOM_GLINK_SSR=m
>  CONFIG_QCOM_RMTFS_MEM=m
> +CONFIG_SDM_CAMCC_845=m
>  CONFIG_QCOM_RPMH=y
>  CONFIG_QCOM_RPMHPD=y
>  CONFIG_QCOM_SMEM=y
> -- 
> 2.20.1
> 

Re: [v1 6/6] arm64: defconfig: Enable QCOM CAMCC, CAMSS and CCI drivers

[Robert Foss]

On Thu, 12 Mar 2020 at 06:35, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Wed 11 Mar 05:35 PDT 2020, Robert Foss wrote:
>
> > Build camera clock, isp and controller drivers as modules.
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
>
> Thanks for the series Robert!

Thanks for the quick & thorough review, I'll send out a v2 with all of
the feedback corrected.

>
> Regards,
> Bjorn
>
> > ---
> >  arch/arm64/configs/defconfig | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index 4db223dbc549..7cb6989249ab 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -376,6 +376,7 @@ CONFIG_I2C_MESON=y
> >  CONFIG_I2C_MV64XXX=y
> >  CONFIG_I2C_OWL=y
> >  CONFIG_I2C_PXA=y
> > +CONFIG_I2C_QCOM_CCI=m
> >  CONFIG_I2C_QCOM_GENI=m
> >  CONFIG_I2C_QUP=y
> >  CONFIG_I2C_RK3X=y
> > @@ -530,6 +531,7 @@ CONFIG_VIDEO_SAMSUNG_S5P_MFC=m
> >  CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC=m
> >  CONFIG_VIDEO_RENESAS_FCP=m
> >  CONFIG_VIDEO_RENESAS_VSP1=m
> > +CONFIG_VIDEO_QCOM_CAMSS=m
> >  CONFIG_DRM=m
> >  CONFIG_DRM_I2C_NXP_TDA998X=m
> >  CONFIG_DRM_NOUVEAU=m
> > @@ -732,6 +734,7 @@ CONFIG_MSM_GCC_8994=y
> >  CONFIG_MSM_MMCC_8996=y
> >  CONFIG_MSM_GCC_8998=y
> >  CONFIG_QCS_GCC_404=y
> > +CONFIG_SDM_CAMCC_845=m
> >  CONFIG_SDM_GCC_845=y
> >  CONFIG_SM_GCC_8150=y
> >  CONFIG_QCOM_HFPLL=y
> > @@ -762,6 +765,7 @@ CONFIG_QCOM_COMMAND_DB=y
> >  CONFIG_QCOM_GENI_SE=y
> >  CONFIG_QCOM_GLINK_SSR=m
> >  CONFIG_QCOM_RMTFS_MEM=m
> > +CONFIG_SDM_CAMCC_845=m
> >  CONFIG_QCOM_RPMH=y
> >  CONFIG_QCOM_RPMHPD=y
> >  CONFIG_QCOM_SMEM=y
> > --
> > 2.20.1
> >