[PATCH 1/2] ARM: dts: qcom: pm8941: fix vadc channel node names

[PATCH 1/2] ARM: dts: qcom: pm8941: fix vadc channel node names

[Luca Weiss]

Node names for the channel are supposed to be adc-chan@REG.

Use this format and at the same time sort the nodes by reg value.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 arch/arm/boot/dts/qcom-pm8941.dtsi | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
index 9cd49deb9fa7..3c15eecf2f21 100644
--- a/arch/arm/boot/dts/qcom-pm8941.dtsi
+++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
@@ -101,26 +101,33 @@ pm8941_vadc: adc@3100 {
 			#size-cells = <0>;
 			#io-channel-cells = <1>;
 
-			bat_temp {
-				reg = <VADC_LR_MUX1_BAT_THERM>;
+
+			adc-chan@6 {
+				reg = <VADC_VBAT_SNS>;
 			};
-			die_temp {
+
+			adc-chan@8 {
 				reg = <VADC_DIE_TEMP>;
 			};
-			ref_625mv {
+
+			adc-chan@9 {
 				reg = <VADC_REF_625MV>;
 			};
-			ref_1250v {
+
+			adc-chan@10 {
 				reg = <VADC_REF_1250MV>;
 			};
-			ref_gnd {
+
+			adc-chan@14 {
 				reg = <VADC_GND_REF>;
 			};
-			ref_vdd {
+
+			adc-chan@15 {
 				reg = <VADC_VDD_VADC>;
 			};
-			vbat_sns {
-				reg = <VADC_VBAT_SNS>;
+
+			adc-chan@48 {
+				reg = <VADC_LR_MUX1_BAT_THERM>;
 			};
 		};
 
-- 
2.37.3

[PATCH 2/2] ARM: dts: qcom: pm8941: fix iadc node

[Luca Weiss]

The iadc node name is supposed to be just 'adc' and the compatible is
only supposed to be qcom,spmi-iadc according to the bindings.

Adjust the node to match that.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 arch/arm/boot/dts/qcom-pm8941.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
index 3c15eecf2f21..33517cccee01 100644
--- a/arch/arm/boot/dts/qcom-pm8941.dtsi
+++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
@@ -131,8 +131,8 @@ adc-chan@48 {
 			};
 		};
 
-		pm8941_iadc: iadc@3600 {
-			compatible = "qcom,pm8941-iadc", "qcom,spmi-iadc";
+		pm8941_iadc: adc@3600 {
+			compatible = "qcom,spmi-iadc";
 			reg = <0x3600>;
 			interrupts = <0x0 0x36 0x0 IRQ_TYPE_EDGE_RISING>;
 			qcom,external-resistor-micro-ohms = <10000>;
-- 
2.37.3

Re: [PATCH 1/2] ARM: dts: qcom: pm8941: fix vadc channel node names

[Neil Armstrong]

On 25/09/2022 18:18, Luca Weiss wrote:
> Node names for the channel are supposed to be adc-chan@REG.
> 
> Use this format and at the same time sort the nodes by reg value.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>   arch/arm/boot/dts/qcom-pm8941.dtsi | 25 ++++++++++++++++---------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
> index 9cd49deb9fa7..3c15eecf2f21 100644
> --- a/arch/arm/boot/dts/qcom-pm8941.dtsi
> +++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
> @@ -101,26 +101,33 @@ pm8941_vadc: adc@3100 {
>   			#size-cells = <0>;
>   			#io-channel-cells = <1>;
>   
> -			bat_temp {
> -				reg = <VADC_LR_MUX1_BAT_THERM>;
> +
> +			adc-chan@6 {
> +				reg = <VADC_VBAT_SNS>;
>   			};
> -			die_temp {
> +
> +			adc-chan@8 {
>   				reg = <VADC_DIE_TEMP>;
>   			};
> -			ref_625mv {
> +
> +			adc-chan@9 {
>   				reg = <VADC_REF_625MV>;
>   			};
> -			ref_1250v {
> +
> +			adc-chan@10 {
>   				reg = <VADC_REF_1250MV>;
>   			};
> -			ref_gnd {
> +
> +			adc-chan@14 {
>   				reg = <VADC_GND_REF>;
>   			};
> -			ref_vdd {
> +
> +			adc-chan@15 {
>   				reg = <VADC_VDD_VADC>;
>   			};
> -			vbat_sns {
> -				reg = <VADC_VBAT_SNS>;
> +
> +			adc-chan@48 {
> +				reg = <VADC_LR_MUX1_BAT_THERM>;
>   			};
>   		};
>   

Here aswell, I don't see where this is required, bindings doesn't mandate this naming:

patternProperties:

    "^.*@[0-9a-f]+$":
Neil

Re: [PATCH 2/2] ARM: dts: qcom: pm8941: fix iadc node

[Neil Armstrong]

On 25/09/2022 18:18, Luca Weiss wrote:
> The iadc node name is supposed to be just 'adc' and the compatible is
> only supposed to be qcom,spmi-iadc according to the bindings.
> 
> Adjust the node to match that.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>   arch/arm/boot/dts/qcom-pm8941.dtsi | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
> index 3c15eecf2f21..33517cccee01 100644
> --- a/arch/arm/boot/dts/qcom-pm8941.dtsi
> +++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
> @@ -131,8 +131,8 @@ adc-chan@48 {
>   			};
>   		};
>   
> -		pm8941_iadc: iadc@3600 {
> -			compatible = "qcom,pm8941-iadc", "qcom,spmi-iadc";
> +		pm8941_iadc: adc@3600 {
> +			compatible = "qcom,spmi-iadc";
>   			reg = <0x3600>;
>   			interrupts = <0x0 0x36 0x0 IRQ_TYPE_EDGE_RISING>;
>   			qcom,external-resistor-micro-ohms = <10000>;

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 2/2] ARM: dts: qcom: pm8941: fix iadc node

[Krzysztof Kozlowski]

On 25/09/2022 18:18, Luca Weiss wrote:
> The iadc node name is supposed to be just 'adc' and the compatible is
> only supposed to be qcom,spmi-iadc according to the bindings.
> 
> Adjust the node to match that.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>  arch/arm/boot/dts/qcom-pm8941.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
> index 3c15eecf2f21..33517cccee01 100644
> --- a/arch/arm/boot/dts/qcom-pm8941.dtsi
> +++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
> @@ -131,8 +131,8 @@ adc-chan@48 {
>  			};
>  		};
>  
> -		pm8941_iadc: iadc@3600 {
> -			compatible = "qcom,pm8941-iadc", "qcom,spmi-iadc";
> +		pm8941_iadc: adc@3600 {
> +			compatible = "qcom,spmi-iadc";

I am not sure this is correct. Usually specific compatibles are encouraged.

Best regards,
Krzysztof

Re: [PATCH 1/2] ARM: dts: qcom: pm8941: fix vadc channel node names

[Krzysztof Kozlowski]

On 25/09/2022 18:18, Luca Weiss wrote:
> Node names for the channel are supposed to be adc-chan@REG.

Why exactly?

> 
> Use this format and at the same time sort the nodes by reg value.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>  arch/arm/boot/dts/qcom-pm8941.dtsi | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 

Best regards,
Krzysztof

Re: [PATCH 2/2] ARM: dts: qcom: pm8941: fix iadc node

[Luca Weiss]

Hi Krzysztof,

On Montag, 26. September 2022 10:54:23 CEST Krzysztof Kozlowski wrote:
> On 25/09/2022 18:18, Luca Weiss wrote:
> > The iadc node name is supposed to be just 'adc' and the compatible is
> > only supposed to be qcom,spmi-iadc according to the bindings.
> > 
> > Adjust the node to match that.
> > 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > 
> >  arch/arm/boot/dts/qcom-pm8941.dtsi | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi
> > b/arch/arm/boot/dts/qcom-pm8941.dtsi index 3c15eecf2f21..33517cccee01
> > 100644
> > --- a/arch/arm/boot/dts/qcom-pm8941.dtsi
> > +++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
> > @@ -131,8 +131,8 @@ adc-chan@48 {
> > 
> >  			};
> >  		
> >  		};
> > 
> > -		pm8941_iadc: iadc@3600 {
> > -			compatible = "qcom,pm8941-iadc", 
"qcom,spmi-iadc";
> > +		pm8941_iadc: adc@3600 {
> > +			compatible = "qcom,spmi-iadc";
> 
> I am not sure this is correct. Usually specific compatibles are encouraged.

I'm happy to change it the other way also.

But the sibling of this compatible, qcom,spmi-vadc also only has that single 
compatible so it'd align it with that.

Let me know what you think.

Regards
Luca

> 
> Best regards,
> Krzysztof

Re: [PATCH 2/2] ARM: dts: qcom: pm8941: fix iadc node

[Konrad Dybcio]

On 25.09.2022 18:18, Luca Weiss wrote:
> The iadc node name is supposed to be just 'adc' and the compatible is
> only supposed to be qcom,spmi-iadc according to the bindings.
> 
> Adjust the node to match that.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@somainline.org>

Konrad
>  arch/arm/boot/dts/qcom-pm8941.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
> index 3c15eecf2f21..33517cccee01 100644
> --- a/arch/arm/boot/dts/qcom-pm8941.dtsi
> +++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
> @@ -131,8 +131,8 @@ adc-chan@48 {
>  			};
>  		};
>  
> -		pm8941_iadc: iadc@3600 {
> -			compatible = "qcom,pm8941-iadc", "qcom,spmi-iadc";
> +		pm8941_iadc: adc@3600 {
> +			compatible = "qcom,spmi-iadc";
>  			reg = <0x3600>;
>  			interrupts = <0x0 0x36 0x0 IRQ_TYPE_EDGE_RISING>;
>  			qcom,external-resistor-micro-ohms = <10000>;

Re: [PATCH 1/2] ARM: dts: qcom: pm8941: fix vadc channel node names

[Konrad Dybcio]

On 25.09.2022 18:18, Luca Weiss wrote:
> Node names for the channel are supposed to be adc-chan@REG.
> 
> Use this format and at the same time sort the nodes by reg value.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@somainline.org>

Konrad
>  arch/arm/boot/dts/qcom-pm8941.dtsi | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi b/arch/arm/boot/dts/qcom-pm8941.dtsi
> index 9cd49deb9fa7..3c15eecf2f21 100644
> --- a/arch/arm/boot/dts/qcom-pm8941.dtsi
> +++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
> @@ -101,26 +101,33 @@ pm8941_vadc: adc@3100 {
>  			#size-cells = <0>;
>  			#io-channel-cells = <1>;
>  
> -			bat_temp {
> -				reg = <VADC_LR_MUX1_BAT_THERM>;
> +
> +			adc-chan@6 {
> +				reg = <VADC_VBAT_SNS>;
>  			};
> -			die_temp {
> +
> +			adc-chan@8 {
>  				reg = <VADC_DIE_TEMP>;
>  			};
> -			ref_625mv {
> +
> +			adc-chan@9 {
>  				reg = <VADC_REF_625MV>;
>  			};
> -			ref_1250v {
> +
> +			adc-chan@10 {
>  				reg = <VADC_REF_1250MV>;
>  			};
> -			ref_gnd {
> +
> +			adc-chan@14 {
>  				reg = <VADC_GND_REF>;
>  			};
> -			ref_vdd {
> +
> +			adc-chan@15 {
>  				reg = <VADC_VDD_VADC>;
>  			};
> -			vbat_sns {
> -				reg = <VADC_VBAT_SNS>;
> +
> +			adc-chan@48 {
> +				reg = <VADC_LR_MUX1_BAT_THERM>;
>  			};
>  		};
>  

Re: [PATCH 2/2] ARM: dts: qcom: pm8941: fix iadc node

[Krzysztof Kozlowski]

On 26/09/2022 17:05, Luca Weiss wrote:
> Hi Krzysztof,
> 
> On Montag, 26. September 2022 10:54:23 CEST Krzysztof Kozlowski wrote:
>> On 25/09/2022 18:18, Luca Weiss wrote:
>>> The iadc node name is supposed to be just 'adc' and the compatible is
>>> only supposed to be qcom,spmi-iadc according to the bindings.
>>>
>>> Adjust the node to match that.
>>>
>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>>> ---
>>>
>>>  arch/arm/boot/dts/qcom-pm8941.dtsi | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/qcom-pm8941.dtsi
>>> b/arch/arm/boot/dts/qcom-pm8941.dtsi index 3c15eecf2f21..33517cccee01
>>> 100644
>>> --- a/arch/arm/boot/dts/qcom-pm8941.dtsi
>>> +++ b/arch/arm/boot/dts/qcom-pm8941.dtsi
>>> @@ -131,8 +131,8 @@ adc-chan@48 {
>>>
>>>  			};
>>>  		
>>>  		};
>>>
>>> -		pm8941_iadc: iadc@3600 {
>>> -			compatible = "qcom,pm8941-iadc", 
> "qcom,spmi-iadc";
>>> +		pm8941_iadc: adc@3600 {
>>> +			compatible = "qcom,spmi-iadc";
>>
>> I am not sure this is correct. Usually specific compatibles are encouraged.
> 
> I'm happy to change it the other way also.
> 
> But the sibling of this compatible, qcom,spmi-vadc also only has that single 
> compatible so it'd align it with that.

Ugh, there is a mess around them. Some other ADCs have specific
compatibles, some not, so there is no consistency.

I propose to have device specific compatible with qcom,spmi-iadc
fallback, so basically document the DTS in bindings. Maybe other IADC
will need some quirks, so specific compatible helps here.


Best regards,
Krzysztof