| Message ID | 20230401220810.3563708-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: remove duplication in PMIC declarations | expand |
On 02/04/2023 00:07, Dmitry Baryshkov wrote: > Following the commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the > correct PON compatible") and commit f46ef374e0dc ("arm64: dts: qcom: > pmk8350: Specify PBS register for PON") use "qcom,pmk8350-pon" compat > string and add RBS region to the PON device. > > Fixes: ccd3517faf18 ("arm64: dts: qcom: sc8280xp: Add reference device") There is no compatible qcom,pmk8350-pon documented at ccd3517faf18, so backporting it there is incorrect. qcom,pmk8350-pon is neither in v5.19 nor in v6.0. Best regards, Krzysztof
On 02/04/2023 00:08, Dmitry Baryshkov wrote: > SA8450p-based platforms have 4 instances of pm8150. Convert pm8150.dtsi > to use pmic-dyn-header.dtsi in order to support dynamic and label > assignment. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > arch/arm64/boot/dts/qcom/pm8150.dtsi | 53 ++++++++++++------- > arch/arm64/boot/dts/qcom/pmic-dyn-footer.dtsi | 1 + > 2 files changed, 36 insertions(+), 18 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/pm8150.dtsi b/arch/arm64/boot/dts/qcom/pm8150.dtsi > index db90c55fa2cf..77bb325e425b 100644 > --- a/arch/arm64/boot/dts/qcom/pm8150.dtsi > +++ b/arch/arm64/boot/dts/qcom/pm8150.dtsi > @@ -9,13 +9,28 @@ > #include <dt-bindings/spmi/spmi.h> > #include <dt-bindings/iio/qcom,spmi-vadc.h> > > +/* (Sadly) this PMIC can be configured to be at different SIDs */ > +#ifndef PMIC_SID > + #define PMIC_SID 0 > +#endif No, the DTS code must be simple, no ifndefs for some defines. This means that sometimes you expect here define, sometimes not. It's not easy to maintain and understand the code. Define must be simple and always defined, not sometimes. > + > +#ifndef PMIC_SID1 > + #define PMIC_SID1 1 > +#endif > + > +#ifndef PMIC_LABEL > + #define PMIC_LABEL pm8150 > +#endif > + > +#include "pmic-dyn-header.dtsi" > + > / { > thermal-zones { > - pm8150-thermal { > + NODE(thermal) { > polling-delay-passive = <100>; > polling-delay = <0>; > > - thermal-sensors = <&pm8150_temp>; > + thermal-sensors = <&LABEL(temp)>; > > trips { > trip0 { > @@ -41,9 +56,9 @@ trip2 { > }; > > &spmi_bus { > - pm8150_0: pmic@0 { > + pmic@0 { > compatible = "qcom,pm8150", "qcom,spmi-pmic"; > - reg = <0x0 SPMI_USID>; > + reg = <PMIC_SID SPMI_USID>; > #address-cells = <1>; > #size-cells = <0>; > > @@ -55,7 +70,7 @@ pon: pon@800 { > > pon_pwrkey: pwrkey { > compatible = "qcom,pm8941-pwrkey"; > - interrupts = <0x0 0x8 0x0 IRQ_TYPE_EDGE_BOTH>; > + interrupts = <PMIC_SID 0x8 0x0 IRQ_TYPE_EDGE_BOTH>; > debounce = <15625>; > bias-pull-up; > linux,code = <KEY_POWER>; > @@ -65,7 +80,7 @@ pon_pwrkey: pwrkey { > > pon_resin: resin { > compatible = "qcom,pm8941-resin"; > - interrupts = <0x0 0x8 0x1 IRQ_TYPE_EDGE_BOTH>; > + interrupts = <PMIC_SID 0x8 0x1 IRQ_TYPE_EDGE_BOTH>; > debounce = <15625>; > bias-pull-up; > > @@ -73,22 +88,22 @@ pon_resin: resin { > }; > }; > > - pm8150_temp: temp-alarm@2400 { > + LABEL(temp): temp-alarm@2400 { NAK for all defines creating labels. Best regards, Krzysztof
On 02/04/2023 00:07, Dmitry Baryshkov wrote: > The sc8280xp platform uses its own copy of PMIC declarations. This can > easily end up with the issues that are fixed in the main PMIC include > file, but are not fixed for sc8280xp (and vice versa). For example > commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON > compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the > PON device, while sc8280xp-pmic.dtsi still has the incorrect > "qcom,pm8998-pon". > > Another example is pm8280_2_temp_alarm device, which uses interrupts > tied to SID 2, while having SID 3. This can be easily left unnoticed. > > Employ a small amount of C preprocessor magic to make > sc8280xp-pmics.dtsi use standard PMIC include files Preprocessor magic is disliked in DTS. We allow only simple defines, no undefs. Sometimes some nodes or strings could be concatenated, but in obvious way. You should not parametrize it and have different, generated labels in DTS based on something coming external to that DTS. Best regards, Krzysztof
On Sun, 2 Apr 2023 at 12:42, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 02/04/2023 00:07, Dmitry Baryshkov wrote: > > Following the commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the > > correct PON compatible") and commit f46ef374e0dc ("arm64: dts: qcom: > > pmk8350: Specify PBS register for PON") use "qcom,pmk8350-pon" compat > > string and add RBS region to the PON device. > > > > Fixes: ccd3517faf18 ("arm64: dts: qcom: sc8280xp: Add reference device") > > There is no compatible qcom,pmk8350-pon documented at ccd3517faf18, so > backporting it there is incorrect. qcom,pmk8350-pon is neither in v5.19 > nor in v6.0. Well, according to Documentation/process/submitting-patches.rst, Fixes tag is about noting that there was an issue fixed in the commit. The mentioned commit has an issue, as the device should have a second region. I did not intend to have this patch backported (no Cc stable). If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce.
On 02/04/2023 12:25, Dmitry Baryshkov wrote: > On Sun, 2 Apr 2023 at 12:42, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 02/04/2023 00:07, Dmitry Baryshkov wrote: >>> Following the commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the >>> correct PON compatible") and commit f46ef374e0dc ("arm64: dts: qcom: >>> pmk8350: Specify PBS register for PON") use "qcom,pmk8350-pon" compat >>> string and add RBS region to the PON device. >>> >>> Fixes: ccd3517faf18 ("arm64: dts: qcom: sc8280xp: Add reference device") >> >> There is no compatible qcom,pmk8350-pon documented at ccd3517faf18, so >> backporting it there is incorrect. qcom,pmk8350-pon is neither in v5.19 >> nor in v6.0. > > Well, according to Documentation/process/submitting-patches.rst, Fixes > tag is about noting that there was an issue fixed in the commit. The > mentioned commit has an issue, as the device should have a second Depends. If device was working in some limited way with old compatible and one region, there is nothing to fix maybe. It was just incomplete. If second region is needed for the work, then only that commit should be marked as fix. Changing compatible is not a fix of that submission because at the time, the compatible was correct. That time in Git history, the "qcom,pmk8350-pon" was not correct. > region. I did not intend to have this patch backported (no Cc stable). > If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce. AUTOSEL will backport it anyway, if you do not mention otherwise. Best regards, Krzysztof
On 02/04/2023 13:32, Krzysztof Kozlowski wrote: > On 02/04/2023 12:25, Dmitry Baryshkov wrote: >> On Sun, 2 Apr 2023 at 12:42, Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org> wrote: >>> >>> On 02/04/2023 00:07, Dmitry Baryshkov wrote: >>>> Following the commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the >>>> correct PON compatible") and commit f46ef374e0dc ("arm64: dts: qcom: >>>> pmk8350: Specify PBS register for PON") use "qcom,pmk8350-pon" compat >>>> string and add RBS region to the PON device. >>>> >>>> Fixes: ccd3517faf18 ("arm64: dts: qcom: sc8280xp: Add reference device") >>> >>> There is no compatible qcom,pmk8350-pon documented at ccd3517faf18, so >>> backporting it there is incorrect. qcom,pmk8350-pon is neither in v5.19 >>> nor in v6.0. >> >> Well, according to Documentation/process/submitting-patches.rst, Fixes >> tag is about noting that there was an issue fixed in the commit. The >> mentioned commit has an issue, as the device should have a second > > Depends. If device was working in some limited way with old compatible > and one region, there is nothing to fix maybe. It was just incomplete. > > If second region is needed for the work, then only that commit should be > marked as fix. Changing compatible is not a fix of that submission > because at the time, the compatible was correct. That time in Git > history, the "qcom,pmk8350-pon" was not correct. Ack, so we have to backport the schema too. > >> region. I did not intend to have this patch backported (no Cc stable). >> If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce. > > AUTOSEL will backport it anyway, if you do not mention otherwise. Is there a way to influence AUTOSEL to make it also pick up another commit? > > Best regards, > Krzysztof >
On 02/04/2023 13:03, Dmitry Baryshkov wrote: > >> >>> region. I did not intend to have this patch backported (no Cc stable). >>> If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce. >> >> AUTOSEL will backport it anyway, if you do not mention otherwise. > > Is there a way to influence AUTOSEL to make it also pick up another commit? Only via usual stable kernel patch rules/syntax. Best regards, Krzysztof
On 2.04.2023 13:12, Krzysztof Kozlowski wrote: > On 02/04/2023 13:03, Dmitry Baryshkov wrote: >> >>> >>>> region. I did not intend to have this patch backported (no Cc stable). >>>> If I were, I could have also added a Cc stable # 5.19.x 03fccdc76dce. >>> >>> AUTOSEL will backport it anyway, if you do not mention otherwise. >> >> Is there a way to influence AUTOSEL to make it also pick up another commit? > > Only via usual stable kernel patch rules/syntax. Additionally, some patches submitted via Option 1 may have additional patch prerequisites which can be cherry-picked. This can be specified in the following format in the sign-off area: Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle Cc: <stable@vger.kernel.org> # 3.3.x: fd21073: sched: Fix affinity logic Cc: <stable@vger.kernel.org> # 3.3.x Signed-off-by: Ingo Molnar <mingo@elte.hu> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html Konrad > > Best regards, > Krzysztof >
On 2.04.2023 11:55, Krzysztof Kozlowski wrote: > On 02/04/2023 00:07, Dmitry Baryshkov wrote: >> The sc8280xp platform uses its own copy of PMIC declarations. This can >> easily end up with the issues that are fixed in the main PMIC include >> file, but are not fixed for sc8280xp (and vice versa). For example >> commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON >> compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the >> PON device, while sc8280xp-pmic.dtsi still has the incorrect >> "qcom,pm8998-pon". >> >> Another example is pm8280_2_temp_alarm device, which uses interrupts >> tied to SID 2, while having SID 3. This can be easily left unnoticed. >> >> Employ a small amount of C preprocessor magic to make >> sc8280xp-pmics.dtsi use standard PMIC include files > > Preprocessor magic is disliked in DTS. We allow only simple defines, no > undefs. Sometimes some nodes or strings could be concatenated, but in > obvious way. You should not parametrize it and have different, generated > labels in DTS based on something coming external to that DTS. This again begs the question, is it time we start moving parts of the dts code to be autogenerated? Should we keep a separate file for each SID? Or should we consider the SPMI 'interrupts' implementation flawed and work towards one that does not require a SID to be specified within? Currently it's: interrupts = <USID PERIPH_ADDR>>8 IRQ_WITHIN_PERIPH IRQ_TYPE>; So the first two cells are effectively useless and can be retrieved from the parent node and the reg property. Getting rid of that would solve a decent chunk of problems that this patchset concerns. Konrad > > Best regards, > Krzysztof >
On Mon, 3 Apr 2023 at 13:44, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 2.04.2023 11:55, Krzysztof Kozlowski wrote: > > On 02/04/2023 00:07, Dmitry Baryshkov wrote: > >> The sc8280xp platform uses its own copy of PMIC declarations. This can > >> easily end up with the issues that are fixed in the main PMIC include > >> file, but are not fixed for sc8280xp (and vice versa). For example > >> commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON > >> compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the > >> PON device, while sc8280xp-pmic.dtsi still has the incorrect > >> "qcom,pm8998-pon". > >> > >> Another example is pm8280_2_temp_alarm device, which uses interrupts > >> tied to SID 2, while having SID 3. This can be easily left unnoticed. > >> > >> Employ a small amount of C preprocessor magic to make > >> sc8280xp-pmics.dtsi use standard PMIC include files > > > > Preprocessor magic is disliked in DTS. We allow only simple defines, no > > undefs. Sometimes some nodes or strings could be concatenated, but in > > obvious way. You should not parametrize it and have different, generated > > labels in DTS based on something coming external to that DTS. > This again begs the question, is it time we start moving parts of the > dts code to be autogenerated? > > Should we keep a separate file for each SID? > > Or should we consider the SPMI 'interrupts' implementation flawed and > work towards one that does not require a SID to be specified within? > > Currently it's: > > interrupts = <USID PERIPH_ADDR>>8 IRQ_WITHIN_PERIPH IRQ_TYPE>; > > So the first two cells are effectively useless and can be retrieved > from the parent node and the reg property. > > Getting rid of that would solve a decent chunk of problems that this > patchset concerns. While I agree with the USID part, the PERIPH_ADDR part is not always like that, see pm8916.dtsi / audio-codec@f000. I'll give it a thought, but it is probably not in the forthcoming future.
On 03/04/2023 12:44, Konrad Dybcio wrote: > > > On 2.04.2023 11:55, Krzysztof Kozlowski wrote: >> On 02/04/2023 00:07, Dmitry Baryshkov wrote: >>> The sc8280xp platform uses its own copy of PMIC declarations. This can >>> easily end up with the issues that are fixed in the main PMIC include >>> file, but are not fixed for sc8280xp (and vice versa). For example >>> commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON >>> compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the >>> PON device, while sc8280xp-pmic.dtsi still has the incorrect >>> "qcom,pm8998-pon". >>> >>> Another example is pm8280_2_temp_alarm device, which uses interrupts >>> tied to SID 2, while having SID 3. This can be easily left unnoticed. >>> >>> Employ a small amount of C preprocessor magic to make >>> sc8280xp-pmics.dtsi use standard PMIC include files >> >> Preprocessor magic is disliked in DTS. We allow only simple defines, no >> undefs. Sometimes some nodes or strings could be concatenated, but in >> obvious way. You should not parametrize it and have different, generated >> labels in DTS based on something coming external to that DTS. > This again begs the question, is it time we start moving parts of the > dts code to be autogenerated? Do we auto-generate C-code? Just a bit, but in general no. There are exceptions but coding style is here clear: https://elixir.bootlin.com/linux/v6.1-rc2/source/Documentation/process/coding-style.rst#L828 Pre-processor generated code should be narrowed to some cases or simpler structures. For DTS we actually are even stricter. Best regards, Krzysztof
The sc8280xp platform uses its own copy of PMIC declarations. This can easily end up with the issues that are fixed in the main PMIC include file, but are not fixed for sc8280xp (and vice versa). For example commit c0ee8e0ba5cc ("arm64: dts: qcom: pmk8350: Use the correct PON compatible") changed pmk8350 to use "qcom,pmk8350-pon" compat for the PON device, while sc8280xp-pmic.dtsi still has the incorrect "qcom,pm8998-pon". Another example is pm8280_2_temp_alarm device, which uses interrupts tied to SID 2, while having SID 3. This can be easily left unnoticed. Employ a small amount of C preprocessor magic to make sc8280xp-pmics.dtsi use standard PMIC include files Also apply the same approach to sa8540p-pmics/pm8150. Jonathan Cameron Acked merging the dt-bindigns patch together with the rest of the patches to simplify merge process. Dmitry Baryshkov (22): arm64: dts: qcom: pm8350: fix thermal zone node name arm64: dts: qcom: pm8350b: fix thermal zone node name arm64: dts: qcom: sc8280xp-pmics: use pmk8350 specifics for pon device arm64: dts: qcom: sc8280xp-pmics: correct interrupt routing for pm8280_2_temp_alarm dt-bindings: iio: qcom,spmi-adc7-pmk8350.h: include sid into defines arm64: dts: qcom: pmk8350: rename pon label arm64: dts: qcom: pmk8350: port sdam_6 device from sc8280xp-pmics arm64: dts: qcom: pmk8350: rename PMK8350_SID to PMIC_SID arm64: dts: qcom: pmk8350: allow overriding the label arm64: dts: qcom: pmk8350: use interrupts-extended for IRQ specification arm64: dts: qcom: sc8280xp*: use pmk8350.dtsi arm64: dts: qcom: pm8350: allow overriding SID and label arm64: dts: qcom: pm8350: use interrupts-extended for IRQ specification arm64: dts: qcom: sc8280xp*: use pm8350.dtsi arm64: dts: qcom: pm8350c: move thermal zone declaration to the top arm64: dts: qcom: pm8350c: allow overriding SID and label arm64: dts: qcom: pm8350c: use interrupts-extended for IRQ specification arm64: dts: qcom: sc8280xp*: use pm8350c.dtsi arm64: dts: qcom: sc8280xp*: use pmr735a.dtsi arm64: dts: qcom: pm8150: convert to use dynamic SID/LABEL arch: arm64: dts: qcom: pm8150: support SID greater that 9 arm64: dts: qcom sa8540p-pmics: switch to pm8150.dtsi .../bindings/iio/adc/qcom,spmi-vadc.yaml | 2 +- .../bindings/thermal/qcom-spmi-adc-tm5.yaml | 4 +- arch/arm64/boot/dts/qcom/pm8150.dtsi | 53 +++-- arch/arm64/boot/dts/qcom/pm8350.dtsi | 33 ++- arch/arm64/boot/dts/qcom/pm8350b.dtsi | 6 +- arch/arm64/boot/dts/qcom/pm8350c.dtsi | 73 +++--- arch/arm64/boot/dts/qcom/pmic-dyn-footer.dtsi | 23 ++ arch/arm64/boot/dts/qcom/pmic-dyn-header.dtsi | 26 +++ arch/arm64/boot/dts/qcom/pmk8350.dtsi | 51 ++-- arch/arm64/boot/dts/qcom/sa8540p-pmics.dtsi | 96 ++------ arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 2 +- arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 2 +- arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 4 +- .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 8 +- arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 221 ++---------------- .../qcom/sm6375-sony-xperia-murray-pdx225.dts | 7 +- .../boot/dts/qcom/sm7225-fairphone-fp4.dts | 8 +- arch/arm64/boot/dts/qcom/sm8350-mtp.dts | 8 +- .../dts/qcom/sm8350-sony-xperia-sagami.dtsi | 8 +- .../dts/qcom/sm8450-sony-xperia-nagara.dtsi | 4 +- .../dt-bindings/iio/qcom,spmi-adc7-pmk8350.h | 52 ++--- 21 files changed, 279 insertions(+), 412 deletions(-) create mode 100644 arch/arm64/boot/dts/qcom/pmic-dyn-footer.dtsi create mode 100644 arch/arm64/boot/dts/qcom/pmic-dyn-header.dtsi