Re: [PATCH 5/5] arm64: dts: qcom: Add Sony Xperia (Loire) X and X Compact support

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

On Sun 20 Oct 08:07 PDT 2019, kholk11@gmail.com wrote:
[..]
> diff --git a/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire-kugo.dts b/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire-kugo.dts
[..]
> +&pm8950_l1 {
> +	regulator-min-microvolt = <1100000>;
> +	regulator-max-microvolt = <1300000>;
> +	qcom,init-voltage = <1200000>;

There's no qcom,init-voltage in the upstream binding.

> +};
> +
> +/* Machine specific pins override */
> +&tlmm {
> +	usb_cable_det_n_gpio: usb_cable_det_n {
> +		mux {

As stated on previous patch, feel free to squash/drop the inner
subnodes.

> +			pins = "gpio107";
> +			function = "gpio";
> +		};
> +		config {
> +			pins = "gpio107";
> +			drive-strength = <2>;
> +			bias-pull-up;
> +			input-enable;
> +		};
> +	};
> +
[..]
> diff --git a/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi b/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi
[..]
> +&soc {
> +	blsp2_uart2: serial@7af0000 {

Specify this, disabled, in msm8976.dtsi and just enable it here.

> +		compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
> +		reg = <0x7af0000 0x200>;
> +		interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&gcc GCC_BLSP2_UART2_APPS_CLK>,
> +			 <&gcc GCC_BLSP2_AHB_CLK>;
> +		clock-names = "core", "iface";
> +		dmas = <&blsp2_dma 1>, <&blsp2_dma 0>;
> +		dma-names = "rx", "tx";
> +		status = "okay";
> +	};
> +
> +	gpio_keys {

gpio_keys is not a mmio device, so please move it to /

> +		compatible = "gpio-keys";
> +		input-name = "gpio-keys";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		autorepeat;
> +
[..]
> +&blsp1_uart1 {
> +	status = "disabled";

If you need to disable these nodes in your first board dtsi, then they
should have been disabled in msm8976.dtsi.

> +};
> +
> +&blsp1_uart2 {
> +	status = "disabled";
> +};
> +
> +/* eMMC */
> +&sdhc_1 {
> +	vmmc-supply = <&pm8950_l8>;
> +	vqmmc-supply = <&pm8950_l5>;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sdc1_clk_on &sdc1_cmd_on &sdc1_data_on>;

I suggest that you specify a single sdc1_on state with subnodes clk, cmd
and data.

> +
> +	status = "ok";
> +};
> +
[..]
> +&smd_rpm_regulators {
> +	vdd_s1-supply = <&vph_pwr>;
> +	vdd_s2-supply = <&vph_pwr>;
> +	vdd_s3-supply = <&vph_pwr>;
> +	vdd_s4-supply = <&vph_pwr>;
> +	vdd_s5-supply = <&vph_pwr>;
> +	vdd_s6-supply = <&vph_pwr>;
> +	vdd_l1_l19-supply = <&pm8950_s3>;
> +	vdd_l2_l23-supply = <&pm8950_s3>;
> +	vdd_l3-supply = <&pm8950_s3>;
> +	vdd_l4_l5_l6_l7_l16-supply = <&pm8950_s4>;
> +	vdd_l8_l11_l12_l17_l22-supply = <&vph_pwr>;
> +	vdd_l20-supply = <&pm8950_s4>;
> +	vdd_l21-supply = <&pm8950_s4>;
> +
> +	s1 {
> +		regulator-min-microvolt = <1000000>;
> +		regulator-max-microvolt = <1162500>;
> +		qcom,init-voltage = <1000000>;

Again, no qcom,init-voltage...

> +		status = "okay";

You shouldn't need to say "okay" here, it's enabled unless you disable
it.

> +	};
[..]
> +&pm8950_gpios {
> +	cdc_pm_mclk_pin: cdc_pm_mclk {
> +		cdc_pm {

Please drop this inner subnode, it's no longer needed (same goes for the
remainder of nodes in this patch).

> +			pins = "gpio1";
> +			function = "func1"; /* SF1 */
> +			output-low;
> +			bias-disable;
> +			qcom,drive-strength = <2>;
> +			power-source = <2>; /* VIN 2 */
> +		};
> +	};
> +

Keep up the good work!

Regards,
Bjorn