Re: [PATCH v2 3/3] arm64: dts: qcom: Add support for X1-based Asus Zenbook A14

[Aleksandrs Vinarskis]

On Sat, 12 Apr 2025 at 01:36, Konrad Dybcio
<konrad.dybcio@xxxxxxxxxxxxxxxx> wrote:
>
> On 4/2/25 10:44 AM, Aleksandrs Vinarskis wrote:
> > Initial support for Asus Zenbook A14. Particular moddel exists
> > in X1-26-100, X1P-42-100 (UX3407QA) and X1E-78-100 (UX3407RA).
> >
> > Mostly similar to other X1-based laptops. Notable differences are:
> > * Wifi/Bluetooth combo being Qualcomm FastConnect 6900 on UX3407QA
> >   and Qualcomm FastConnect 7800 on UX3407RA
> > * USB Type-C retimers are Parade PS8833, appear to behave identical
> >   to Parade PS8830
> > * gpio90 is TZ protected
> >
> > Working:
> > * Keyboard
> > * Touchpad
> > * NVME
> > * Lid switch
> > * Camera LED
> > * eDP (FHD OLED, SDC420D) with brightness control
> > * Bluetooth, WiFi (WCN6855)
> > * USB Type-A port
> > * USB Type-C ports in USB2/USB3/DP (both orientations)
> > * aDSP/cDPS firmware loading, battery info
> > * Sleep/suspend, nothing visibly broken on resume
> >
> > Out of scope of this series:
> > * Audio (Speakers/microphones/headphone jack)
> > * Camera (OmniVision OV02C10)
> > * HDMI (Parade PS185HDM)
> > * EC
> >
> > Add dtsi and create two configurations for UX3407QA, UX3407RA.
> > Tested on UX3407QA with X1-26-100.
> >
> > Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@xxxxxxxxx>
> > ---
>
> [...]
>
> > +     /* Left-side display-adjacent port, PS8833 */
> > +     typec-mux@8 {
> > +             compatible = "parade,ps8830";
>
> What Krzysztof referred to with fallback compatible is this:
>
> diff --git a/Documentation/devicetree/bindings/usb/parade,ps8830.yaml b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
> index 935d57f5d26f..aeb33667818e 100644
> --- a/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
> +++ b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
> @@ -11,8 +11,11 @@ maintainers:
>
>  properties:
>    compatible:
> -    enum:
> -      - parade,ps8830
> +    oneOf:
> +      - items:
> +          - const: parade,ps8833
> +          - const: parade,ps8830
> +      - const: parade,ps8830
>
>
> so that in case there are any sw changes down the line, people with older
> DT receive the fixes, as if "parade,ps8833" is attributed to a driver, it
> will match (due to being the primary entry) and if it's not (like today),
> it will fall back to matching the next compatible (and the driver currently
> looks for just that)
>

Hi,

Thanks, was not aware of that, found it in the examples now, will update.
As Krzysztof suggested to drop the patch adding compatible from
driver's code, If I understand correctly I also need the following dts
change, could you please confirm:

```
compatible = "parade,ps8833", "parade,ps8830";
```

> [...]
>
> > +     eusb6_repeater: redriver@4f {
> > +             compatible = "nxp,ptn3222";
> > +             reg = <0x4f>;
> > +             #phy-cells = <0>;
> > +
> > +             vdd3v3-supply = <&vreg_l13b_3p0>;
> > +             vdd1v8-supply = <&vreg_l4b_1p8>;
> > +
> > +             reset-gpios = <&tlmm 184 GPIO_ACTIVE_LOW>;
> > +
> > +             pinctrl-0 = <&eusb6_reset_n>;
> > +             pinctrl-names = "default";
> > +     };
> > +
> > +     /* EC */
>
> It's customary to leave the i2c address to make it slightly easier for
> the next tinkerer ;)

I am unsure about the address. There are multiple addresses showing up
when discovering the bus, 0x1d, 0x5b, 0x61, 0x6a. Did a quick SMBUS
analysis, they all have non-zero registers responding... My best guess
right now is its 0x5b, as messing with it enabled backlight and broke
the keyboard. Will add that one.

>
> [...]
>
> > +&uart14 {
> > +     status = "okay";
> > +
> > +     bluetooth {
> > +             compatible = "qcom,wcn7850-bt";
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&bt_en_default>;
> > +             enable-gpios = <&tlmm 116 GPIO_ACTIVE_HIGH>;
> > +             max-speed = <3000000>;
>
> You'll need to provide some supplies, coming out of a pwrseq device, see
> e.g. the QCP
>

I assume you are referring to change like this [1]. There were some
discussions on whether the supplies are modelled correctly as it seems
to be different when using m.2 card, so as I was not sure about this
platform I did not add it. At least in the variant that I have, the
wcn6855 is soldered onboard, so I would assume so is the wcn7850
variant.
It seems that the two use quite different supplies - are the platform
dependent, or only wlan card dependent? Ie. Can I just copy pwrseq for
wcn6855 from a different platform?

For  wcn7850's pwrseq, I can add similarly to other platforms, but
cannot verify it. Should I still add it in this case?

A bit unrelated question: in the meantime I managed to bring-up the
sound on Zenbook as well. Would it be more correct to wait for this
series to land, and then send it separately, or is it okay to add it
in directly on re-spin?

Thank you,
Alex

https://lore.kernel.org/all/20250331204610.526672-2-alex.vinarskis@xxxxxxxxx/

> Konrad