Re: [PATCH 2/2] arm64: dts: qcom: x1e80100-t14s: Add external DP support

[Johan Hovold <johan@kernel.org>]

On Mon, Sep 02, 2024 at 06:01:36PM +0300, Abel Vesa wrote:
> The Lenovo Thinkpad T14s has only 2 USB Type-C ports, both of them
> supporting external DP altmode. Between each QMP combo PHY and the
> corresponding Type-C port, sits one Parade PS8830 retimer which handles
> both orientation and SBU muxing. Add nodes for each retimer, fix the
> graphs between connectors and the PHYs accordingly add the voltage
> regulators needed by each retimer and then enable DP 0 and 1.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  .../dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts     | 278 ++++++++++++++++++++-
>  1 file changed, 274 insertions(+), 4 deletions(-)
 			};
> @@ -127,6 +143,90 @@ vreg_edp_3p3: regulator-edp-3p3 {
>  		regulator-boot-on;
>  	};
>  
> +	vreg_rtmr0_1p15: regulator-rtmr0-1p15 {

Please consider spelling out "retimer". It seems it's mostly you that
use "rtmr" in the kernel currently, and not sure saving those three
chars is worth the cost in readability.

But if this is what these rails are called in the (CRD) schematics (I
didn't check), then just ignore this comment.

> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_RTMR0_1P15";
> +		regulator-min-microvolt = <1150000>;
> +		regulator-max-microvolt = <1150000>;
> +
> +		gpio = <&pm8550ve_8_gpios 8 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-0 = <&rtmr0_1p15_reg_en>;
> +		pinctrl-names = "default";
> +	};

Please keep the nodes sorted by name (by moving the retimer nodes below
the nvme regulator.

>  	vreg_nvme: regulator-nvme {
>  		compatible = "regulator-fixed";
>  
> @@ -484,6 +584,111 @@ keyboard@3a {
>  	};
>  };
>  
> +&i2c3 {
> +	clock-frequency = <400000>;
> +
> +	status = "okay";
> +
> +	typec-mux@8 {
> +		compatible = "parade,ps8830";
> +		reg = <0x08>;
> +
> +		clocks = <&rpmhcc RPMH_RF_CLK3>;
> +		clock-names = "xo";
> +
> +		vdd15-supply = <&vreg_rtmr0_1p15>;

As Konrad already pointed on the retimer patch, this one should be name
vdd115 or similar.

> +		vdd18-supply = <&vreg_rtmr0_1p8>;
> +		vdd33-supply = <&vreg_rtmr0_3p3>;

> +&i2c7 {
> +	clock-frequency = <400000>;
> +
> +	status = "okay";
> +
> +	typec-mux@8 {
> +		compatible = "parade,ps8830";
> +		reg = <0x8>;
> +
> +		clocks = <&rpmhcc RPMH_RF_CLK4>;
> +		clock-names = "xo";
> +
> +		vdd15-supply = <&vreg_rtmr1_1p15>;
> +		vdd18-supply = <&vreg_rtmr1_1p8>;
> +		vdd33-supply = <&vreg_rtmr1_3p3>;
> +
> +		reset-gpios = <&tlmm 176 GPIO_ACTIVE_HIGH>;
> +
> +		retimer-switch;
> +		orientation-switch;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +
> +				retimer_ss1_ss_out: endpoint {
> +					remote-endpoint = <&pmic_glink_ss1_ss_in>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +
> +				retimer_ss1_ss_in: endpoint {
> +					remote-endpoint = <&usb_1_ss1_qmpphy_out>;
> +				};
> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +
> +				retimer_ss1_con_sbu_out: endpoint {
> +					remote-endpoint = <&pmic_glink_ss1_con_sbu_in>;
> +				};
> +			};
> +

Stray newline.

> +		};
> +	};
> +};

> +	rtmr1_3p3_reg_en: rtmr1-3p3-reg-en-state {
> +		pins = "gpio186";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
> +

Stray newline.

>  	wcd_default: wcd-reset-n-active-state {
>  		pins = "gpio191";
>  		function = "gpio";

Johan