diff mbox series

[v2,02/22] arm64: dts: qcom: pm8350b: fix thermal zone node name

Message ID 20230401220810.3563708-3-dmitry.baryshkov@linaro.org
State Superseded
Headers show
Series arm64: dts: qcom: remove duplication in PMIC declarations | expand

Commit Message

Dmitry Baryshkov April 1, 2023, 10:07 p.m. UTC
Correct the thermal zone node name to remove the clash with
pm8350c.dtsi. Remove unused labels.

Fixes: 5c1399299d9d ("arm64: dts: qcom: pm8350b: add temp sensor and thermal zone config")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/pm8350b.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Krzysztof Kozlowski April 2, 2023, 10:34 a.m. UTC | #1
On 02/04/2023 00:07, Dmitry Baryshkov wrote:
> Correct the thermal zone node name to remove the clash with
> pm8350c.dtsi. Remove unused labels.
> 
> Fixes: 5c1399299d9d ("arm64: dts: qcom: pm8350b: add temp sensor and thermal zone config")

Please describe the observable bug from that commit being fixed here.
Any future clash, which did not exist that time, is not a bug. It's future.

Naming changes here are more a matter of style, because the old names
were correct according to our coding guidelines, just not precise (c
instead of b). But node names anyway are not important from the point of
view fixes and adding such tag will cause a needless backport.



Best regards,
Krzysztof
Dmitry Baryshkov April 2, 2023, 11:02 a.m. UTC | #2
On 02/04/2023 13:34, Krzysztof Kozlowski wrote:
> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
>> Correct the thermal zone node name to remove the clash with
>> pm8350c.dtsi. Remove unused labels.
>>
>> Fixes: 5c1399299d9d ("arm64: dts: qcom: pm8350b: add temp sensor and thermal zone config")
> 
> Please describe the observable bug from that commit being fixed here.
> Any future clash, which did not exist that time, is not a bug. It's future.
> 
> Naming changes here are more a matter of style, because the old names
> were correct according to our coding guidelines, just not precise (c
> instead of b). But node names anyway are not important from the point of
> view fixes and adding such tag will cause a needless backport.

It is needed. Including both pm8350c.dtsi and pm8350b.dtsi will result 
in one thermal zone overriding another one.
Krzysztof Kozlowski April 3, 2023, 9:09 a.m. UTC | #3
On 02/04/2023 13:02, Dmitry Baryshkov wrote:
> On 02/04/2023 13:34, Krzysztof Kozlowski wrote:
>> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
>>> Correct the thermal zone node name to remove the clash with
>>> pm8350c.dtsi. Remove unused labels.
>>>
>>> Fixes: 5c1399299d9d ("arm64: dts: qcom: pm8350b: add temp sensor and thermal zone config")
>>
>> Please describe the observable bug from that commit being fixed here.
>> Any future clash, which did not exist that time, is not a bug. It's future.
>>
>> Naming changes here are more a matter of style, because the old names
>> were correct according to our coding guidelines, just not precise (c
>> instead of b). But node names anyway are not important from the point of
>> view fixes and adding such tag will cause a needless backport.
> 
> It is needed. Including both pm8350c.dtsi and pm8350b.dtsi will result 
> in one thermal zone overriding another one.

I don't understand. You used future tense "will", but we talk about
past. So where is the bug in commit 5c1399299d9d?

Best regards,
Krzysztof
Dmitry Baryshkov April 3, 2023, 9:50 a.m. UTC | #4
On 03/04/2023 12:09, Krzysztof Kozlowski wrote:
> On 02/04/2023 13:02, Dmitry Baryshkov wrote:
>> On 02/04/2023 13:34, Krzysztof Kozlowski wrote:
>>> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
>>>> Correct the thermal zone node name to remove the clash with
>>>> pm8350c.dtsi. Remove unused labels.
>>>>
>>>> Fixes: 5c1399299d9d ("arm64: dts: qcom: pm8350b: add temp sensor and thermal zone config")
>>>
>>> Please describe the observable bug from that commit being fixed here.
>>> Any future clash, which did not exist that time, is not a bug. It's future.
>>>
>>> Naming changes here are more a matter of style, because the old names
>>> were correct according to our coding guidelines, just not precise (c
>>> instead of b). But node names anyway are not important from the point of
>>> view fixes and adding such tag will cause a needless backport.
>>
>> It is needed. Including both pm8350c.dtsi and pm8350b.dtsi will result
>> in one thermal zone overriding another one.
> 
> I don't understand. You used future tense "will", but we talk about
> past. So where is the bug in commit 5c1399299d9d?

At that time there already existed sm8350-mtp which included both of 
dtsi files.
Konrad Dybcio April 3, 2023, 10 a.m. UTC | #5
On 3.04.2023 11:50, Dmitry Baryshkov wrote:
> On 03/04/2023 12:09, Krzysztof Kozlowski wrote:
>> On 02/04/2023 13:02, Dmitry Baryshkov wrote:
>>> On 02/04/2023 13:34, Krzysztof Kozlowski wrote:
>>>> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
>>>>> Correct the thermal zone node name to remove the clash with
>>>>> pm8350c.dtsi. Remove unused labels.
>>>>>
>>>>> Fixes: 5c1399299d9d ("arm64: dts: qcom: pm8350b: add temp sensor and thermal zone config")
>>>>
>>>> Please describe the observable bug from that commit being fixed here.
>>>> Any future clash, which did not exist that time, is not a bug. It's future.
>>>>
>>>> Naming changes here are more a matter of style, because the old names
>>>> were correct according to our coding guidelines, just not precise (c
>>>> instead of b). But node names anyway are not important from the point of
>>>> view fixes and adding such tag will cause a needless backport.
>>>
>>> It is needed. Including both pm8350c.dtsi and pm8350b.dtsi will result
>>> in one thermal zone overriding another one.
>>
>> I don't understand. You used future tense "will", but we talk about
>> past. So where is the bug in commit 5c1399299d9d?
> 
> At that time there already existed sm8350-mtp which included both of dtsi files.
> 
pm8350[bc] both have a node named /pm8350c-thermal, which means one will
get overriden with the other. Since we sort the includes alphabetically,
the one in pm8350b.dtsi will never be taken into account and hence the
temp alarm will never be associated to any thermal zone.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
Krzysztof Kozlowski April 3, 2023, 10:06 a.m. UTC | #6
On 03/04/2023 12:00, Konrad Dybcio wrote:
> 
> 
> On 3.04.2023 11:50, Dmitry Baryshkov wrote:
>> On 03/04/2023 12:09, Krzysztof Kozlowski wrote:
>>> On 02/04/2023 13:02, Dmitry Baryshkov wrote:
>>>> On 02/04/2023 13:34, Krzysztof Kozlowski wrote:
>>>>> On 02/04/2023 00:07, Dmitry Baryshkov wrote:
>>>>>> Correct the thermal zone node name to remove the clash with
>>>>>> pm8350c.dtsi. Remove unused labels.
>>>>>>
>>>>>> Fixes: 5c1399299d9d ("arm64: dts: qcom: pm8350b: add temp sensor and thermal zone config")
>>>>>
>>>>> Please describe the observable bug from that commit being fixed here.
>>>>> Any future clash, which did not exist that time, is not a bug. It's future.
>>>>>
>>>>> Naming changes here are more a matter of style, because the old names
>>>>> were correct according to our coding guidelines, just not precise (c
>>>>> instead of b). But node names anyway are not important from the point of
>>>>> view fixes and adding such tag will cause a needless backport.
>>>>
>>>> It is needed. Including both pm8350c.dtsi and pm8350b.dtsi will result
>>>> in one thermal zone overriding another one.
>>>
>>> I don't understand. You used future tense "will", but we talk about
>>> past. So where is the bug in commit 5c1399299d9d?
>>
>> At that time there already existed sm8350-mtp which included both of dtsi files.
>>
> pm8350[bc] both have a node named /pm8350c-thermal, which means one will
> get overriden with the other. Since we sort the includes alphabetically,
> the one in pm8350b.dtsi will never be taken into account and hence the
> temp alarm will never be associated to any thermal zone.

OK, if I understand correctly, this overwrite of thermal node was
already happening at commit 5c1399299d9d. It looks good then.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/pm8350b.dtsi b/arch/arm64/boot/dts/qcom/pm8350b.dtsi
index f1c7bd9d079c..95e971b80ccc 100644
--- a/arch/arm64/boot/dts/qcom/pm8350b.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8350b.dtsi
@@ -8,19 +8,19 @@ 
 
 / {
 	thermal-zones {
-		pm8350b_thermal: pm8350c-thermal {
+		pm8350b-thermal {
 			polling-delay-passive = <100>;
 			polling-delay = <0>;
 			thermal-sensors = <&pm8350b_temp_alarm>;
 
 			trips {
-				pm8350b_trip0: trip0 {
+				trip0 {
 					temperature = <95000>;
 					hysteresis = <0>;
 					type = "passive";
 				};
 
-				pm8350b_crit: pm8350c-crit {
+				crit {
 					temperature = <115000>;
 					hysteresis = <0>;
 					type = "critical";