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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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
>