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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zta1PN-bvOyPYJCs@hovoldconsulting.com>
Date: Tue, 3 Sep 2024 09:05:32 +0200
From: Johan Hovold <johan@...nel.org>
To: Abel Vesa <abel.vesa@...aro.org>
Cc: Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Trilok Soni <quic_tsoni@...cinc.com>, linux-arm-msm@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] arm64: dts: qcom: x1e80100-t14s: Add external DP
 support

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