Re: [PATCH 4/4] arm64: dts: qcom: msm8996: add support for oneplus3(t)

[Harry Austen]

On Friday, October 21st, 2022 at 3:44 PM, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
[...]
> > +++ b/arch/arm64/boot/dts/qcom/msm8996-oneplus-common.dtsi
> > @@ -0,0 +1,794 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> 
> 
> Are you sure this is GPL-2.0 only? Didn't you derive it from downstream
> OnePlus DTS?

Yes development of these devicetrees was aided by downstream DTS, all of which appear to have
GPL-2.0 only headers, e.g. see msm8996-mtp.dts [1].

> 
> > +/*
> > + * Copyright (c) 2022, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include "msm8996.dtsi"
> > +#include "pm8994.dtsi"
> > +#include "pmi8994.dtsi"
> > +#include "pmi8996.dtsi"
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> > +#include <dt-bindings/sound/qcom,q6afe.h>
> > +#include <dt-bindings/sound/qcom,q6asm.h>
> > +#include <dt-bindings/sound/qcom,wcd9335.h>
> > +
> > +/ {
> > + aliases {
> > + serial0 = &blsp1_uart2;
> > + serial1 = &blsp2_uart2;
> > + };
> > +
> > + battery: battery {
> > + compatible = "simple-battery";
> > +
> > + constant-charge-current-max-microamp = <3000000>;
> > + voltage-min-design-microvolt = <3400000>;
> > + };
> > +
> > + chosen {
> > + stdout-path = "serial1:115200n8";
> > + };
> > +
> > + clocks {
> > + compatible = "simple-bus";
> 
> 
> This is not a bus of clocks...

Will remove in v2.

> 
> > +
> > + divclk4: divclk4 {
> 
> 
> Use common suffix or prefix for node names and generic name.
> 
> This clock is anyway a bit weird - same frequency as sleep clk.
> 
> > + compatible = "fixed-clock";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&divclk4_pin_a>;
> 
> 
> This is a PMIC pin? So is it a PMIC clk?

These two clocks are described in the same way as other current MSM8996 DTs (e.g. apq8096-db820c.dts
and msm8996-xiaomi-common.dtsi). Happy to change if you think there is a better way to describe them?
Yes, these clocks originate from within the PM8994 PMIC as per the datasheet [2]. GPIO_15 is
configured with the DIV_CLK1 alt function and routes to the MCLK pin of the WCD9225 audio codec.
GPIO_18 is configured with the SLEEP_CLK5 alt function and provides the SUSCLK_32KHZ input to the
Atheros QCA6174 WiFi/BT chip.

> 
> > + #clock-cells = <0>;
> > + clock-frequency = <32768>;
> > + clock-output-names = "divclk4";
> > + };
> > +
> > + div1_mclk: divclk1 {
> > + compatible = "gpio-gate-clock";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&audio_mclk>;
> > + #clock-cells = <0>;
> > + clocks = <&rpmcc RPM_SMD_DIV_CLK1>;
> > + enable-gpios = <&pm8994_gpios 15 GPIO_ACTIVE_HIGH>;
> > + };
> > + };
> > +
> > + reserved-memory {
> > + ramoops@ac000000 {
> > + compatible = "ramoops";
> > + reg = <0 0xac000000 0 0x200000>;
> > + record-size = <0x20000>;
> > + console-size = <0x100000>;
> > + pmsg-size = <0x80000>;
> > + };
> > + };
> > +
> > + vph_pwr: vph-pwr-regulator {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vph_pwr";
> > + regulator-min-microvolt = <3700000>;
> > + regulator-max-microvolt = <3700000>;
> > + regulator-always-on;
> > + regulator-boot-on;
> > + };
> > +
> > + wlan_en: wlan-en-1-8v {
> 
> 
> Use common suffix or prefix. You already used "-regulator" suffix before.

Will fix in v2.

> 
> > + compatible = "regulator-fixed";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&wlan_en_gpios>;
> > + regulator-name = "wlan-en-regulator";
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > +
> > + gpio = <&pm8994_gpios 8 GPIO_ACTIVE_HIGH>;
> > +
> > + /* WLAN card specific delay */
> > + startup-delay-us = <70000>;
> > + enable-active-high;
> > + };
> > +};
> > +
> > +&adsp_pil {
> > + status = "okay";
> > +};
> > +
> > +&blsp1_i2c3 {
> > + status = "okay";
> > +
> > + tfa9890_amp: audio-codec@36 {
> > + compatible = "nxp,tfa9890";
> > + reg = <0x36>;
> > + #sound-dai-cells = <0>;
> > + };
> > +};
> > +
> > +&blsp1_i2c6 {
> > + status = "okay";
> > +
> > + bq27541: fuel-gauge@55 {
> > + compatible = "ti,bq27541";
> > + reg = <0x55>;
> > + };
> > +};
> > +
> > +&blsp1_uart2 {
> > + label = "BT-UART";
> > + status = "okay";
> 
> 
> Status is a last property.

Will fix all of these in v2.

> 
> Best regards,
> Krzysztof

Thanks for the review!
Harry

[1]: https://github.com/OnePlusOSS/android_kernel_oneplus_msm8996/blob/oneplus3/6.0.1/arch/arm/boot/dts/qcom/msm8996-mtp.dtsi
[2]: https://developer.qualcomm.com/qfile/35466/lm80-p2751-5_c.pdf