| Message ID | 20230401220810.3563708-1-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: remove duplication in PMIC declarations | expand |
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 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