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@xxxxxxxxxx>
> > ---
> > .../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
