Re: [PATCH v2 20/22] arm64: dts: qcom: pm8150: convert to use dynamic SID/LABEL

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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