diff mbox series

[v5,1/6] dt-bindings: thermal: mediatek: Rename thermal zone definitions for MT8186 and MT8188

Message ID 20240524-mtk-thermal-mt818x-dtsi-v5-1-56f8579820e7@baylibre.com (mailing list archive)
State New
Delegated to: Daniel Lezcano
Headers show
Series Mediatek thermal sensor driver support for MT8186 and MT8188 | expand

Commit Message

Julien Panis May 24, 2024, 9:04 a.m. UTC
Use thermal zone names that make more sense.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
---
 include/dt-bindings/thermal/mediatek,lvts-thermal.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Conor Dooley May 24, 2024, 6:24 p.m. UTC | #1
On Fri, May 24, 2024 at 11:04:34AM +0200, Julien Panis wrote:
> Use thermal zone names that make more sense.
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>

Removing the defines is an ABI break. If these are all the same devices,
but with more accurate naming, then keep the old defines and add new
ones. However, the GPU1 define changes in the course of this patch which
is more problematic.

Why do these names even make more sense? Where did the old names come
from and where do the new?

Thanks,
Conor.

> ---
>  include/dt-bindings/thermal/mediatek,lvts-thermal.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/dt-bindings/thermal/mediatek,lvts-thermal.h b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
> index bf95309d2525..ddc7302a510a 100644
> --- a/include/dt-bindings/thermal/mediatek,lvts-thermal.h
> +++ b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
> @@ -24,7 +24,7 @@
>  #define MT8186_BIG_CPU1	5
>  #define MT8186_NNA		6
>  #define MT8186_ADSP		7
> -#define MT8186_MFG		8
> +#define MT8186_GPU		8
>  
>  #define MT8188_MCU_LITTLE_CPU0	0
>  #define MT8188_MCU_LITTLE_CPU1	1
> @@ -34,11 +34,11 @@
>  #define MT8188_MCU_BIG_CPU1	5
>  
>  #define MT8188_AP_APU		0
> -#define MT8188_AP_GPU1		1
> -#define MT8188_AP_GPU2		2
> -#define MT8188_AP_SOC1		3
> -#define MT8188_AP_SOC2		4
> -#define MT8188_AP_SOC3		5
> +#define MT8188_AP_GPU0		1
> +#define MT8188_AP_GPU1		2
> +#define MT8188_AP_ADSP		3
> +#define MT8188_AP_VDO		4
> +#define MT8188_AP_INFRA		5
>  #define MT8188_AP_CAM1		6
>  #define MT8188_AP_CAM2		7
>  
> 
> -- 
> 2.37.3
>
Conor Dooley May 24, 2024, 6:27 p.m. UTC | #2
On Fri, May 24, 2024 at 07:24:47PM +0100, Conor Dooley wrote:
> On Fri, May 24, 2024 at 11:04:34AM +0200, Julien Panis wrote:
> > Use thermal zone names that make more sense.
> > 
> > Signed-off-by: Julien Panis <jpanis@baylibre.com>
> 
> Removing the defines is an ABI break. If these are all the same devices,
> but with more accurate naming, then keep the old defines and add new
> ones. However, the GPU1 define changes in the course of this patch which
> is more problematic.

> > [RFC] When PATCH 1/6 and 2/6 are squashed, checkpatch raises this WARNING:
> > "DT binding docs and includes should be a separate patch." That's why I
> > split them in this v5. The problem is that the driver can't be compiled
> > any more at PATCH 1/6. It needs PATCH 2/6 to be compiled. Should the
> > checkpatch warning be ignored here ? Should I finally squash PATCH 1/6
> > and PATCH 2/6 ?

Heh, and there's just one of the issues caused by your ABI break...

> Why do these names even make more sense? Where did the old names come
> from and where do the new?


Thanks,
Conor
Julien Panis May 27, 2024, 8:14 a.m. UTC | #3
On 5/24/24 20:24, Conor Dooley wrote:
> On Fri, May 24, 2024 at 11:04:34AM +0200, Julien Panis wrote:
>> Use thermal zone names that make more sense.
>>
>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> Removing the defines is an ABI break. If these are all the same devices,
> but with more accurate naming, then keep the old defines and add new
> ones. However, the GPU1 define changes in the course of this patch which
> is more problematic.
>
> Why do these names even make more sense? Where did the old names come
> from and where do the new?
>
> Thanks,
> Conor.

Thanks for your comment.

For mt8188, the old name 'soc' came from a document by MTK, which is not
public: MT8188G Functional Specification. This document does not explain
what 'soc' are/do exactly, it just says that some temperature sensors are
located on them.

Then, there was a comment about these 'soc' names:
https://lore.kernel.org/all/ff12e104-da8b-4800-bfbe-a006ffe1b840@collabora.com/

So, I had a discussion with MTK to understand what these 'soc1/2/3'
are/do. The new names comes from this discussion and were given by MTK.

For the other SoC of this series (mt8186), the same kind of document exists
(MT8186G Functional Specification) and explains what 'soc' are/do exactly:
('cam', 'dsp', 'nna'...which are used in 'mt8186.dtsi').
Using the same logic in 'mt8188.dtsi' would be more consistent. Besides,
these names are more explicit.

Julien

>
>> ---
>>   include/dt-bindings/thermal/mediatek,lvts-thermal.h | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/dt-bindings/thermal/mediatek,lvts-thermal.h b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
>> index bf95309d2525..ddc7302a510a 100644
>> --- a/include/dt-bindings/thermal/mediatek,lvts-thermal.h
>> +++ b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
>> @@ -24,7 +24,7 @@
>>   #define MT8186_BIG_CPU1	5
>>   #define MT8186_NNA		6
>>   #define MT8186_ADSP		7
>> -#define MT8186_MFG		8
>> +#define MT8186_GPU		8
>>   
>>   #define MT8188_MCU_LITTLE_CPU0	0
>>   #define MT8188_MCU_LITTLE_CPU1	1
>> @@ -34,11 +34,11 @@
>>   #define MT8188_MCU_BIG_CPU1	5
>>   
>>   #define MT8188_AP_APU		0
>> -#define MT8188_AP_GPU1		1
>> -#define MT8188_AP_GPU2		2
>> -#define MT8188_AP_SOC1		3
>> -#define MT8188_AP_SOC2		4
>> -#define MT8188_AP_SOC3		5
>> +#define MT8188_AP_GPU0		1
>> +#define MT8188_AP_GPU1		2
>> +#define MT8188_AP_ADSP		3
>> +#define MT8188_AP_VDO		4
>> +#define MT8188_AP_INFRA		5
>>   #define MT8188_AP_CAM1		6
>>   #define MT8188_AP_CAM2		7
>>   
>>
>> -- 
>> 2.37.3
>>
Julien Panis May 27, 2024, 3:25 p.m. UTC | #4
On 5/24/24 20:27, Conor Dooley wrote:
> On Fri, May 24, 2024 at 07:24:47PM +0100, Conor Dooley wrote:
>> On Fri, May 24, 2024 at 11:04:34AM +0200, Julien Panis wrote:
>>> Use thermal zone names that make more sense.
>>>
>>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
>> Removing the defines is an ABI break. If these are all the same devices,
>> but with more accurate naming, then keep the old defines and add new
>> ones. However, the GPU1 define changes in the course of this patch which
>> is more problematic.
>>> [RFC] When PATCH 1/6 and 2/6 are squashed, checkpatch raises this WARNING:
>>> "DT binding docs and includes should be a separate patch." That's why I
>>> split them in this v5. The problem is that the driver can't be compiled
>>> any more at PATCH 1/6. It needs PATCH 2/6 to be compiled. Should the
>>> checkpatch warning be ignored here ? Should I finally squash PATCH 1/6
>>> and PATCH 2/6 ?
> Heh, and there's just one of the issues caused by your ABI break...

Conor,

Would Russell's suggestion be acceptable for you ?
I mean, this one:
https://lore.kernel.org/all/ZlDMNkdE2jmFgD8B@shell.armlinux.org.uk/

I could implement it, but before submitting it I would like to make
sure that it suits everyone.

Julien

>
>> Why do these names even make more sense? Where did the old names come
>> from and where do the new?
>
> Thanks,
> Conor
Conor Dooley May 27, 2024, 5:28 p.m. UTC | #5
On Mon, May 27, 2024 at 05:25:35PM +0200, Julien Panis wrote:
> On 5/24/24 20:27, Conor Dooley wrote:
> > On Fri, May 24, 2024 at 07:24:47PM +0100, Conor Dooley wrote:
> > > On Fri, May 24, 2024 at 11:04:34AM +0200, Julien Panis wrote:
> > > > Use thermal zone names that make more sense.
> > > > 
> > > > Signed-off-by: Julien Panis <jpanis@baylibre.com>
> > > Removing the defines is an ABI break. If these are all the same devices,
> > > but with more accurate naming, then keep the old defines and add new
> > > ones. However, the GPU1 define changes in the course of this patch which
> > > is more problematic.
> > > > [RFC] When PATCH 1/6 and 2/6 are squashed, checkpatch raises this WARNING:
> > > > "DT binding docs and includes should be a separate patch." That's why I
> > > > split them in this v5. The problem is that the driver can't be compiled
> > > > any more at PATCH 1/6. It needs PATCH 2/6 to be compiled. Should the
> > > > checkpatch warning be ignored here ? Should I finally squash PATCH 1/6
> > > > and PATCH 2/6 ?
> > Heh, and there's just one of the issues caused by your ABI break...
> 
> Conor,
> 
> Would Russell's suggestion be acceptable for you ?
> I mean, this one:
> https://lore.kernel.org/all/ZlDMNkdE2jmFgD8B@shell.armlinux.org.uk/
> 
> I could implement it, but before submitting it I would like to make
> sure that it suits everyone.

How's that going to work? MT8188_AP_GPU1 currently means 1, after your
series it means 2.
You're gonna need to pick a different naming for the new defines to
avoid that. Additionally, why even delete the old ones? Just define
new names with the same numbering and you don't need to worry about
any compatibility issues.

Thanks,
Conor.
Nicolas Pitre May 27, 2024, 8:57 p.m. UTC | #6
On Mon, 27 May 2024, Conor Dooley wrote:

> On Mon, May 27, 2024 at 05:25:35PM +0200, Julien Panis wrote:
> > On 5/24/24 20:27, Conor Dooley wrote:
> > > On Fri, May 24, 2024 at 07:24:47PM +0100, Conor Dooley wrote:
> > > > On Fri, May 24, 2024 at 11:04:34AM +0200, Julien Panis wrote:
> > > > > Use thermal zone names that make more sense.
> > > > > 
> > > > > Signed-off-by: Julien Panis <jpanis@baylibre.com>
> > > > Removing the defines is an ABI break. If these are all the same devices,
> > > > but with more accurate naming, then keep the old defines and add new
> > > > ones. However, the GPU1 define changes in the course of this patch which
> > > > is more problematic.
> > > > > [RFC] When PATCH 1/6 and 2/6 are squashed, checkpatch raises this WARNING:
> > > > > "DT binding docs and includes should be a separate patch." That's why I
> > > > > split them in this v5. The problem is that the driver can't be compiled
> > > > > any more at PATCH 1/6. It needs PATCH 2/6 to be compiled. Should the
> > > > > checkpatch warning be ignored here ? Should I finally squash PATCH 1/6
> > > > > and PATCH 2/6 ?
> > > Heh, and there's just one of the issues caused by your ABI break...
> > 
> > Conor,
> > 
> > Would Russell's suggestion be acceptable for you ?
> > I mean, this one:
> > https://lore.kernel.org/all/ZlDMNkdE2jmFgD8B@shell.armlinux.org.uk/
> > 
> > I could implement it, but before submitting it I would like to make
> > sure that it suits everyone.
> 
> How's that going to work? MT8188_AP_GPU1 currently means 1, after your
> series it means 2.
> You're gonna need to pick a different naming for the new defines to
> avoid that. Additionally, why even delete the old ones? Just define
> new names with the same numbering and you don't need to worry about
> any compatibility issues.

Isn't this making a mountain out of a molehill here?

Seriously... none of this was present in a released kernel. The naming 
can be adjusted "atomically" so compilation doesn't break, and 
it is within maintainers' discretion to bypass the checkpatch warning in 
such particular case.


Nicolas
Conor Dooley May 27, 2024, 9:07 p.m. UTC | #7
On Mon, May 27, 2024 at 04:57:15PM -0400, Nicolas Pitre wrote:
> On Mon, 27 May 2024, Conor Dooley wrote:

> > > Would Russell's suggestion be acceptable for you ?
> > > I mean, this one:
> > > https://lore.kernel.org/all/ZlDMNkdE2jmFgD8B@shell.armlinux.org.uk/
> > > 
> > > I could implement it, but before submitting it I would like to make
> > > sure that it suits everyone.
> > 
> > How's that going to work? MT8188_AP_GPU1 currently means 1, after your
> > series it means 2.
> > You're gonna need to pick a different naming for the new defines to
> > avoid that. Additionally, why even delete the old ones? Just define
> > new names with the same numbering and you don't need to worry about
> > any compatibility issues.
> 
> Isn't this making a mountain out of a molehill here?
> 
> Seriously... none of this was present in a released kernel. The naming 
> can be adjusted "atomically" so compilation doesn't break, and 
> it is within maintainers' discretion to bypass the checkpatch warning in 
> such particular case.

If that's the case, then great. Provide a fixes tag, and a better commit
message than "Use thermal zone names that make more sense." that
actually explains why it is okay to change the definitions. There'd've
been neither mountain nor molehill were a sufficient commit message
provided.
diff mbox series

Patch

diff --git a/include/dt-bindings/thermal/mediatek,lvts-thermal.h b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
index bf95309d2525..ddc7302a510a 100644
--- a/include/dt-bindings/thermal/mediatek,lvts-thermal.h
+++ b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
@@ -24,7 +24,7 @@ 
 #define MT8186_BIG_CPU1	5
 #define MT8186_NNA		6
 #define MT8186_ADSP		7
-#define MT8186_MFG		8
+#define MT8186_GPU		8
 
 #define MT8188_MCU_LITTLE_CPU0	0
 #define MT8188_MCU_LITTLE_CPU1	1
@@ -34,11 +34,11 @@ 
 #define MT8188_MCU_BIG_CPU1	5
 
 #define MT8188_AP_APU		0
-#define MT8188_AP_GPU1		1
-#define MT8188_AP_GPU2		2
-#define MT8188_AP_SOC1		3
-#define MT8188_AP_SOC2		4
-#define MT8188_AP_SOC3		5
+#define MT8188_AP_GPU0		1
+#define MT8188_AP_GPU1		2
+#define MT8188_AP_ADSP		3
+#define MT8188_AP_VDO		4
+#define MT8188_AP_INFRA		5
 #define MT8188_AP_CAM1		6
 #define MT8188_AP_CAM2		7