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

[Abel Vesa <abel.vesa@linaro.org>]

On 24-09-03 09:05:32, Johan Hovold wrote:
> 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.

According to schematics, they are called VREG_RTMRx_*. So will keep
that.

> 
> > +		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.

Will do.

> 
> >  	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.

Yes. Will fix.

> 
> > +		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.

Will drop.

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

Will drop.

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

Thanks for reviewing.

Abel