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

[Krzysztof Kozlowski]

On 22/10/2022 06:38, Harry Austen wrote:
> 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].

OK, but then below copyright is not correct:
> 
>>
>>> +/*
>>> + * Copyright (c) 2022, The Linux Foundation. All rights reserved.

... unless you work for The Linux Foundation?


>>> + */
>>> +
>>> +#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.

So this is SLEEP_CLK - a PMIC generated 32 kHz clock, which is quite
typical among many PMIC designs. Representing it like this a bit
hack/workaround and proper way is to have proper clock driver.

But on the other hand, this is much easier and already such pattern was
introduced with MSM8996 Xiaomi, so fine by me.

Just name the nodes generic.	


Best regards,
Krzysztof