From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Dzmitry Sankouski <dsankouski@gmail.com>
Cc: linux-kernel@vger.kernel.org, Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Konrad Dybcio <konrad.dybcio@somainline.org>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v2 1/2] arm64: dts: qcom: sagit: add initial device tree for sagit
Date: Mon, 3 Oct 2022 08:45:26 +0200 [thread overview]
Message-ID: <c00f37b9-d1fc-f9fa-f4ef-1d6f48353d1e@linaro.org> (raw)
In-Reply-To: <CABTCjFBWrgTzjugjuJRPykAGtp65AF7JKY6eemzt=zn42udH1w@mail.gmail.com>
On 02/10/2022 20:36, Dzmitry Sankouski wrote:
>>> +
>>> + bluetooth {
>>> + compatible = "qcom,wcn3990-bt";
>>> +
>>> + vddio-supply = <&vreg_s4a_1p8>;
>>> + vddxo-supply = <&vreg_l7a_1p8>;
>>> + vddrf-supply = <&vreg_l17a_1p3>;
>>> + vddch0-supply = <&vreg_l25a_3p3>;
>>> + max-speed = <3200000>;
>>> + };
>>> +};
>>> +
>>> +&blsp1_uart3_on {
>>> + rx {
>>
>> Missing suffix pins
>>
> 'rx' pin should be renamed with the corresponding pin in msm8998.dtsi file,
> since we're overriding the pin here, and rename 'rx' pins in other
> msm8998-based dts. Is that what you mean?
> What are the possible suffix names? AFAIU, it can be either 'active' or
> 'suspend', right?
Trim the replies. It takes time to scroll through unrelated context.
If you override node form msm8998.dtsi, then it is fine.
Otherwise it would be suffix "pins", as I wrote.
>
>>
>>> + /delete-property/ bias-disable;
>>> + /*
>>> + * Configure a pull-up on 46 (RX). This is needed to
>>> + * avoid garbage data when the TX pin of the Bluetooth
>>> + * module is in tri-state (module powered off or not
>>> + * driving the signal yet).
>>> + */
>>> + bias-pull-up;
>>> + };
>>> +
>>> + cts {
>>
>> Missing suffix pins
This also can be skipped, since it is override.
>>
>>> + /delete-property/ bias-disable;
>>> + /*
>>> + * Configure a pull-down on 47 (CTS) to match the pull
>>> + * of the Bluetooth module.
>>> + */
>>> + bias-pull-down;
>>> + };
>>> +};
>>> +
>>> +&blsp2_uart1 {
>>> + status = "okay";
>>> +};
>>> +
>>> +&pm8005_lsid1 {
>>> + pm8005-regulators {
>>
>> This is just "regulators", right?
>>
>>> + compatible = "qcom,pm8005-regulators";
>>> +
>>> + vdd_s1-supply = <&vph_pwr>;
>>> +
>>> + pm8005_s1: s1 { /* VDD_GFX supply */
>>> + regulator-min-microvolt = <524000>;
>>> + regulator-max-microvolt = <1100000>;
>>> + regulator-enable-ramp-delay = <500>;
>>> +
>>> + /* hack until we rig up the gpu consumer */
>>> + regulator-always-on;
>>> + };
>>> + };
>>> +};
>>> +
>>> +&pm8998_gpio {
>>> + vol_up_key_default: vol-up-key-default-state {
>>> + pins = "gpio6";
>>> + function = "normal";
>>> + bias-pull-up;
>>> + input-enable;
>>> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_NO>;
>>> + };
>>> +
>>> + audio_mclk_pin: audio-mclk-pin-active-state {
>>> + pins = "gpio13";
>>> + function = "func2";
>>> + power-source = <0>;
>>> + };
>>> +};
>>> +
>>> +&qusb2phy {
>>> + status = "okay";
>>> +
>>> + vdda-pll-supply = <&vreg_l12a_1p8>;
>>> + vdda-phy-dpdm-supply = <&vreg_l24a_3p075>;
>>> +};
>>> +
>>> +&rpm_requests {
>>> + pm8998-regulators {
>>
>> This is for sure now regulators (and since you have two: regulators-0).
>>
> There's no 'regulators-0' occurrences in 6.0.0-rc6 kernel, i.e. it's a new
> convention. Why do we need this new convention? I mean, other msm8998
> boards regulators are named by driver name, and changing that seems like
> rename refactoring.
Do you see "pm8998-regulators" supported? If not, then how it is "rename
refactoring"?
https://lore.kernel.org/all/20220901093243.134288-1-krzysztof.kozlowski@linaro.org/
>
>>
>>> + compatible = "qcom,rpm-pm8998-regulators";
>>> +
>>> + vdd_s1-supply = <&vph_pwr>;
>>> + vdd_s2-supply = <&vph_pwr>;
>>> + vdd_s3-supply = <&vph_pwr>;
>>> + vdd_s4-supply = <&vph_pwr>;
>>> + vdd_s5-supply = <&vph_pwr>;
>>> + vdd_s6-supply = <&vph_pwr>;
>>> + vdd_s7-supply = <&vph_pwr>;
>>> + vdd_s8-supply = <&vph_pwr>;
>>> + vdd_s9-supply = <&vph_pwr>;
>>> + vdd_s10-supply = <&vph_pwr>;
>>> + vdd_s11-supply = <&vph_pwr>;
>>> + vdd_s12-supply = <&vph_pwr>;
>>> + vdd_s13-supply = <&vph_pwr>;
>>> + vdd_l1_l27-supply = <&vreg_s7a_1p025>;
>>> + vdd_l2_l8_l17-supply = <&vreg_s3a_1p35>;
>>> + vdd_l3_l11-supply = <&vreg_s7a_1p025>;
>>> + vdd_l4_l5-supply = <&vreg_s7a_1p025>;
>>> + vdd_l6-supply = <&vreg_s5a_2p04>;
>>> + vdd_l7_l12_l14_l15-supply = <&vreg_s5a_2p04>;
>>> + vdd_l9-supply = <&vreg_bob>;
>>> + vdd_l10_l23_l25-supply = <&vreg_bob>;
>>> + vdd_l13_l19_l21-supply = <&vreg_bob>;
>>> + vdd_l16_l28-supply = <&vreg_bob>;
>>> + vdd_l18_l22-supply = <&vreg_bob>;
>>> + vdd_l20_l24-supply = <&vreg_bob>;
>>> + vdd_l26-supply = <&vreg_s3a_1p35>;
>>> + vdd_lvs1_lvs2-supply = <&vreg_s4a_1p8>;
>>> +
>>> + vreg_s3a_1p35: s3 {
>>> + regulator-min-microvolt = <1352000>;
>>> + regulator-max-microvolt = <1352000>;
>>> + };
>>> +
>>> + vreg_s4a_1p8: s4 {
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + regulator-allow-set-load;
>>> + };
>>> +
>>> + vreg_s5a_2p04: s5 {
>>> + regulator-min-microvolt = <1904000>;
>>> + regulator-max-microvolt = <2040000>;
>>> + };
>>> +
>>> + vreg_s7a_1p025: s7 {
>>> + regulator-min-microvolt = <900000>;
>>> + regulator-max-microvolt = <1028000>;
>>> + };
>>> +
>>> + vreg_l1a_0p875: l1 {
>>> + regulator-min-microvolt = <880000>;
>>> + regulator-max-microvolt = <880000>;
>>> + };
>>> +
>>> + vreg_l2a_1p2: l2 {
>>> + regulator-min-microvolt = <1200000>;
>>> + regulator-max-microvolt = <1200000>;
>>> + };
>>> +
>>> + vreg_l3a_1p0: l3 {
>>> + regulator-min-microvolt = <1000000>;
>>> + regulator-max-microvolt = <1000000>;
>>> + };
>>> +
>>> + vreg_l5a_0p8: l5 {
>>> + regulator-min-microvolt = <800000>;
>>> + regulator-max-microvolt = <800000>;
>>> + };
>>> +
>>> + vreg_l6a_1p8: l6 {
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + };
>>> +
>>> + vreg_l7a_1p8: l7 {
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + };
>>> +
>>> + vreg_l8a_1p2: l8 {
>>> + regulator-min-microvolt = <1200000>;
>>> + regulator-max-microvolt = <1200000>;
>>> + };
>>> +
>>> + vreg_l9a_1p8: l9 {
>>> + regulator-min-microvolt = <1808000>;
>>> + regulator-max-microvolt = <2960000>;
>>> + };
>>> +
>>> + vreg_l10a_1p8: l10 {
>>> + regulator-min-microvolt = <1808000>;
>>> + regulator-max-microvolt = <2960000>;
>>> + };
>>> +
>>> + vreg_l11a_1p0: l11 {
>>> + regulator-min-microvolt = <1000000>;
>>> + regulator-max-microvolt = <1000000>;
>>> + };
>>> +
>>> + vreg_l12a_1p8: l12 {
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + };
>>> +
>>> + vreg_l13a_2p95: l13 {
>>> + regulator-min-microvolt = <1808000>;
>>> + regulator-max-microvolt = <2960000>;
>>> + };
>>> +
>>> + vreg_l14a_1p8: l14 {
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + };
>>> +
>>> + vreg_l15a_1p8: l15 {
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + };
>>> +
>>> + vreg_l16a_2p7: l16 {
>>> + regulator-min-microvolt = <2704000>;
>>> + regulator-max-microvolt = <2704000>;
>>> + };
>>> +
>>> + vreg_l17a_1p3: l17 {
>>> + regulator-min-microvolt = <1304000>;
>>> + regulator-max-microvolt = <1304000>;
>>> + };
>>> +
>>> + vreg_l18a_2p7: l18 {
>>> + regulator-min-microvolt = <2704000>;
>>> + regulator-max-microvolt = <2704000>;
>>> + };
>>> +
>>> + vreg_l19a_3p0: l19 {
>>> + regulator-min-microvolt = <3008000>;
>>> + regulator-max-microvolt = <3008000>;
>>> + };
>>> +
>>> + vreg_l20a_2p95: l20 {
>>> + regulator-min-microvolt = <2960000>;
>>> + regulator-max-microvolt = <2960000>;
>>> + regulator-allow-set-load;
>>> + };
>>> +
>>> + vreg_l21a_2p95: l21 {
>>> + regulator-min-microvolt = <2960000>;
>>> + regulator-max-microvolt = <2960000>;
>>> + regulator-system-load = <800000>;
>>> + regulator-allow-set-load;
>>> + };
>>> +
>>> + vreg_l22a_2p85: l22 {
>>> + regulator-min-microvolt = <2864000>;
>>> + regulator-max-microvolt = <2864000>;
>>> + };
>>> +
>>> + vreg_l23a_3p3: l23 {
>>> + regulator-min-microvolt = <3312000>;
>>> + regulator-max-microvolt = <3312000>;
>>> + };
>>> +
>>> + vreg_l24a_3p075: l24 {
>>> + regulator-min-microvolt = <3088000>;
>>> + regulator-max-microvolt = <3088000>;
>>> + };
>>> +
>>> + vreg_l25a_3p3: l25 {
>>> + regulator-min-microvolt = <3104000>;
>>> + regulator-max-microvolt = <3312000>;
>>> + };
>>> +
>>> + vreg_l26a_1p2: l26 {
>>> + regulator-min-microvolt = <1200000>;
>>> + regulator-max-microvolt = <1200000>;
>>> + regulator-allow-set-load;
>>> + };
>>> +
>>> + vreg_l28_3p0: l28 {
>>> + regulator-min-microvolt = <3008000>;
>>> + regulator-max-microvolt = <3008000>;
>>> + };
>>> +
>>> + vreg_lvs1a_1p8: lvs1 { };
>>> +
>>> + vreg_lvs2a_1p8: lvs2 { };
>>> + };
>>> +
>>> + pmi8998-regulators {
>>
>> regulators-1
>>
>>> + compatible = "qcom,rpm-pmi8998-regulators";
>>> +
>>> + vdd_bob-supply = <&vph_pwr>;
>>> +
>>> + vreg_bob: bob {
>>> + regulator-min-microvolt = <3312000>;
>>> + regulator-max-microvolt = <3600000>;
>>> + };
>>> + };
>>> +};
>>> +
>>> +&tlmm {
>>> + gpio-reserved-ranges = <0 4>, <81 4>;
>>> +
>>> + cci1_default: cci1-default {
>>
>> Missing suffix state. The same in all other places.
>>
>>> + pins = "gpio18", "gpio19";
>>> + function = "cci_i2c";
>>> + bias-disable;
>>> + drive-strength = <2>;
>>> + };
>>> +
>>
>> (...)
>>
>>> +
>>> +&wifi {
>>> + status = "okay";
>>> + vdd-0.8-cx-mx-supply = <&vreg_l5a_0p8>;
>>> + vdd-1.8-xo-supply = <&vreg_l7a_1p8>;
>>> + vdd-1.3-rfa-supply = <&vreg_l17a_1p3>;
>>> + vdd-3.3-ch0-supply = <&vreg_l25a_3p3>;
>>> +};
>>> diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi
>> b/arch/arm64/boot/dts/qcom/pm8998.dtsi
>>> index d09f2954b6f9..4551af463081 100644
>>> --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
>>> @@ -52,6 +52,12 @@ pm8998_pwrkey: pwrkey {
>>> bias-pull-up;
>>> linux,code = <KEY_POWER>;
>>> };
>>> +
>>> + pm8998_resin: resin {
>>
>> Missing suffix state
This comment is probably wrong - it's resin, not pin control.
>>
>>> + compatible = "qcom,pm8941-resin";
>>> + bias-pull-up;
>>> + interrupts = <GIC_SPI 0x8 1
>> IRQ_TYPE_EDGE_BOTH>;
Best regards,
Krzysztof
next prev parent reply other threads:[~2022-10-03 6:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-28 10:33 [PATCH v2 0/2] add device Xiaomi Mi 6 sagit and board binding Dzmitry Sankouski
2022-09-28 10:33 ` [PATCH v2 1/2] arm64: dts: qcom: sagit: add initial device tree for sagit Dzmitry Sankouski
2022-09-28 11:16 ` Krzysztof Kozlowski
[not found] ` <CABTCjFBWrgTzjugjuJRPykAGtp65AF7JKY6eemzt=zn42udH1w@mail.gmail.com>
2022-10-03 6:45 ` Krzysztof Kozlowski [this message]
2022-09-28 10:33 ` [PATCH v2 2/2] dt-bindings: arm: add xiaomi,sagit board based on msm8998 chip Dzmitry Sankouski
2022-09-28 11:12 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c00f37b9-d1fc-f9fa-f4ef-1d6f48353d1e@linaro.org \
--to=krzysztof.kozlowski@linaro.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=dsankouski@gmail.com \
--cc=konrad.dybcio@somainline.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).