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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 4/12/25 1:47 PM, Aleksandrs Vinarskis wrote:
> On Sat, 12 Apr 2025 at 01:36, Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> 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@gmail.com>
>>> ---
>>
>> [...]
>>
>>> +     /* 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";
> ```

Yep!

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

It may be that there are also some 'dumb' devices, like program-and-forget
regulators or similar components, that aren't even described in your [SD]SDT

>>> +&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?

The power sequencer is a description of the hardware that's part of the
same die as the wifi logic, IIUC

The inputs are most likely fixed per SoC+PMIC combo for various reasons

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

Let's not introduce it then

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

Let's get what we have merged first

Konrad