Re: [PATCH v2 2/5] arm64: dts: qcom: Add AYN QCS8550 Common

[Aaron Kling]

On Thu, Mar 19, 2026 at 6:40 AM Konrad Dybcio
<konrad.dybcio@xxxxxxxxxxxxxxxx> wrote:
>
> On 3/11/26 6:44 PM, Aaron Kling via B4 Relay wrote:
> > From: Teguh Sobirin <teguh@xxxxxxxx>
> >
> > This adds a base dtb of everything common between the AYN QCS8550
> > devices. It is intended to be extended by device specific overlays.
> >
> > Signed-off-by: Teguh Sobirin <teguh@xxxxxxxx>
> > Co-developed-by: Aaron Kling <webgeek1234@xxxxxxxxx>
> > Signed-off-by: Aaron Kling <webgeek1234@xxxxxxxxx>
> > ---
>
> [...]
>
> > +     pwm_fan: pwm-fan {
> > +             compatible = "pwm-fan";
> > +
> > +             fan-supply = <&vdd_fan_5v0>;
> > +             pwms = <&pm8550_pwm 3 50000>;
> > +
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&fan_pwm_active>, <&fan_int>;
>
> property-n
> property-names
>
> in this order, everywhere, please

Ack

> > +
> > +             pulses-per-revolution = <4>;
> > +             interrupt-parent = <&tlmm>;
> > +             interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
>
> interrupts-extended = <&tlmm 13 IRQ_...>;

Ack

> [...]
>
> > +             model = "AYN-Odin2";
> > +             audio-routing =
> > +                             "IN1_HPHL", "HPHL_OUT",
>
> Let's drop this empty linebreak

Ack

>
> > +                             "IN2_HPHR", "HPHR_OUT",
> > +                             "AMIC2", "MIC BIAS2",
> > +                             "TX SWR_INPUT1", "ADC2_OUTPUT";
> > +
> > +             speaker-i2s-dai-link {
> > +                     link-name = "Primary MI2S Playback";
> > +
> > +                     cpu {
> > +                             sound-dai = <&q6apmbedai PRIMARY_MI2S_RX>;
> > +                     };
>
> 'co'dec < 'cp'u, please resort

Ack

> [...]
>
> > +     vdd_fan_5v0: vdd-fan-5v0-regulator {
> > +             compatible = "regulator-fixed";
> > +             regulator-name = "vdd_fan_5v0";
> > +
> > +             regulator-min-microvolt = <5000000>;
> > +             regulator-max-microvolt = <5000000>;
> > +
> > +             gpio = <&tlmm 109 GPIO_ACTIVE_HIGH>;
> > +             enable-active-high;
> > +
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&fan_pwr_active>;
> > +
> > +             regulator-state-mem {
> > +                     regulator-off-in-suspend;
> > +             };
>
> oh, I didn't know this binding existed.. but it seems valid indeed!
>
> [...]
>
> > +&i2c12 {
> > +     clock-frequency = <400000>;
> > +     status = "okay";
>
> Let's uniformly keep a \n before status

Ack

> > +};
> > +
> > +&i2c_master_hub_0 {
> > +     status = "okay";
>
> Please add a clock-frequency
> (you can read it back at runtime running a vendor kernel if you don't have a
> better source)

Mmm. I'll see if I can find that. But I don't see any of the existing
sm8550 devices setting this either.

> [...]
>
> > +     spk_amp_l: spk_amp_l@34 {
>
> underscores are no bueno in node names (between ':' and '@'), and they should
> be generic, let's try amplifier@

Ack

> [...]
>
> > +&iris {
> > +     status = "okay";
>
> firmware-name?

Not needed. These devices are unfused and can use the generic firmware
like the devkits. I have verified this as noted in another response on
this change.

> [...]
>
> > +&sdhc_2 {
> > +     cd-gpios = <&pm8550_gpios 12 GPIO_ACTIVE_LOW>;
> > +     pinctrl-names = "default", "sleep";
> > +     pinctrl-0 = <&sdc2_default &sdc2_card_det_n>;
> > +     pinctrl-1 = <&sdc2_sleep &sdc2_card_det_n>;
> > +     vmmc-supply = <&vreg_l9b_2p9>;
> > +     vqmmc-supply = <&vreg_l8b_1p8>;
> > +     max-sd-hs-hz = <37500000>;
>
> It's already in 8550.dtsi, you can drop it

Ack

> > +     no-sdio;
> > +     no-mmc;
> > +
> > +     qcom,dll-config = <0x0007442c>;
>
> Is that changed in your downstream tree?

I honestly don't know. This existed in the mainline port dtsi mirrored
by the vendor and I picked it up as-is. I grepped the downstream
source release and I don't see anything named 'dll-config' in the
sm8550 dt at all, only in older soc's brought in by the kernel fork. I
know the fork I based on was chasing the issues with high speed sd
cards that seem to have been recently fixed upstream. Maybe this was
part of that. I can drop it given no one knows why it's here.

>
> [...]
>
> > +&swr1 {
> > +     status = "okay";
> > +     wcd_rx: codec@0,4 {
>
> Let's keep a \n between properties and the subsequent subnodes,
> also file-wide

Ack

> [...]
>
> > +&tlmm {
> > +     gpio-reserved-ranges = <32 8>;
> > +
> > +     dsi_p_rst_active: dsi-p-rst-active-state {
> > +             pins = "gpio133";
>
> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>
> Let's order them by the pin index (it's a fairly new development so other
> 8550 devices don't really have that)

Ack

> [...]
>
> > +&usb_dp_qmpphy {
> > +     vdda-phy-supply = <&vreg_l3e_1p2>;
> > +     vdda-pll-supply = <&vreg_l3f_0p88>;
> > +
> > +     mode-switch;
>
> Already present in sm8550.dtsi

Ack

Aaron