devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues
       [not found] <cover.1698743706.git.zhoubinbin@loongson.cn>
@ 2023-10-31 11:05 ` Binbin Zhou
  2023-10-31 16:58   ` Conor Dooley
  2023-10-31 11:05 ` [PATCH 2/2] drivers/thermal/loongson2_thermal: Fix incorrect PTR_ERR() judgment Binbin Zhou
  1 sibling, 1 reply; 7+ messages in thread
From: Binbin Zhou @ 2023-10-31 11:05 UTC (permalink / raw)
  To: Binbin Zhou, Huacai Chen, Rafael J . Wysocki, Daniel Lezcano,
	Amit Kucheria, Zhang Rui, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: Huacai Chen, loongson-kernel, linux-pm, devicetree, Yinbo Zhu,
	WANG Xuerui, loongarch, Binbin Zhou

Add the missing 'thermal-sensor-cells' property which is required for
every thermal sensor as it's used when using phandles.
And add the thermal-sensor.yaml reference.

Fixes: 72684d99a854 ("thermal: dt-bindings: add loongson-2 thermal")
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/thermal/loongson,ls2k-thermal.yaml        | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
index 7538469997f9..b634f57cd011 100644
--- a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
+++ b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
@@ -10,6 +10,9 @@ maintainers:
   - zhanghongchen <zhanghongchen@loongson.cn>
   - Yinbo Zhu <zhuyinbo@loongson.cn>
 
+allOf:
+  - $ref: /schemas/thermal/thermal-sensor.yaml#
+
 properties:
   compatible:
     oneOf:
@@ -26,12 +29,16 @@ properties:
   interrupts:
     maxItems: 1
 
+  '#thermal-sensor-cells':
+    const: 1
+
 required:
   - compatible
   - reg
   - interrupts
+  - '#thermal-sensor-cells'
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
@@ -41,4 +48,5 @@ examples:
         reg = <0x1fe01500 0x30>;
         interrupt-parent = <&liointc0>;
         interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
+        #thermal-sensor-cells = <1>;
     };
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] drivers/thermal/loongson2_thermal: Fix incorrect PTR_ERR() judgment
       [not found] <cover.1698743706.git.zhoubinbin@loongson.cn>
  2023-10-31 11:05 ` [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues Binbin Zhou
@ 2023-10-31 11:05 ` Binbin Zhou
  1 sibling, 0 replies; 7+ messages in thread
From: Binbin Zhou @ 2023-10-31 11:05 UTC (permalink / raw)
  To: Binbin Zhou, Huacai Chen, Rafael J . Wysocki, Daniel Lezcano,
	Amit Kucheria, Zhang Rui, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: Huacai Chen, loongson-kernel, linux-pm, devicetree, Yinbo Zhu,
	WANG Xuerui, loongarch, Binbin Zhou

PTR_ERR() returns -ENODEV when thermal-zones are undefined, and we need
-ENODEV as the right value for comparison.

Otherwise, tz->type is NULL when thermal-zones is undefined, resulting
in the following error:

[   12.290030] CPU 1 Unable to handle kernel paging request at virtual address fffffffffffffff1, era == 900000000355f410, ra == 90000000031579b8
[   12.302877] Oops[#1]:
[   12.305190] CPU: 1 PID: 181 Comm: systemd-udevd Not tainted 6.6.0-rc7+ #5385
[   12.312304] pc 900000000355f410 ra 90000000031579b8 tp 90000001069e8000 sp 90000001069eba10
[   12.320739] a0 0000000000000000 a1 fffffffffffffff1 a2 0000000000000014 a3 0000000000000001
[   12.329173] a4 90000001069eb990 a5 0000000000000001 a6 0000000000001001 a7 900000010003431c
[   12.337606] t0 fffffffffffffff1 t1 54567fd5da9b4fd4 t2 900000010614ec40 t3 00000000000dc901
[   12.346041] t4 0000000000000000 t5 0000000000000004 t6 900000010614ee20 t7 900000000d00b790
[   12.354472] t8 00000000000dc901 u0 54567fd5da9b4fd4 s9 900000000402ae10 s0 900000010614ec40
[   12.362916] s1 90000000039fced0 s2 ffffffffffffffed s3 ffffffffffffffed s4 9000000003acc000
[   12.362931] s5 0000000000000004 s6 fffffffffffff000 s7 0000000000000490 s8 90000001028b2ec8
[   12.362938]    ra: 90000000031579b8 thermal_add_hwmon_sysfs+0x258/0x300
[   12.386411]   ERA: 900000000355f410 strscpy+0xf0/0x160
[   12.391626]  CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)
[   12.397898]  PRMD: 00000004 (PPLV0 +PIE -PWE)
[   12.403678]  EUEN: 00000000 (-FPE -SXE -ASXE -BTE)
[   12.409859]  ECFG: 00071c1c (LIE=2-4,10-12 VS=7)
[   12.415882] ESTAT: 00010000 [PIL] (IS= ECode=1 EsubCode=0)
[   12.415907]  BADV: fffffffffffffff1
[   12.415911]  PRID: 0014a000 (Loongson-64bit, Loongson-2K1000)
[   12.415917] Modules linked in: loongson2_thermal(+) vfat fat uio_pdrv_genirq uio fuse zram zsmalloc
[   12.415950] Process systemd-udevd (pid: 181, threadinfo=00000000358b9718, task=00000000ace72fe3)
[   12.415961] Stack : 0000000000000dc0 54567fd5da9b4fd4 900000000402ae10 9000000002df9358
[   12.415982]         ffffffffffffffed 0000000000000004 9000000107a10aa8 90000001002a3410
[   12.415999]         ffffffffffffffed ffffffffffffffed 9000000107a11268 9000000003157ab0
[   12.416016]         9000000107a10aa8 ffffff80020fc0c8 90000001002a3410 ffffffffffffffed
[   12.416032]         0000000000000024 ffffff80020cc1e8 900000000402b2a0 9000000003acc000
[   12.416048]         90000001002a3410 0000000000000000 ffffff80020f4030 90000001002a3410
[   12.416065]         0000000000000000 9000000002df6808 90000001002a3410 0000000000000000
[   12.416081]         ffffff80020f4030 0000000000000000 90000001002a3410 9000000002df2ba8
[   12.416097]         00000000000000b4 90000001002a34f4 90000001002a3410 0000000000000002
[   12.416114]         ffffff80020f4030 fffffffffffffff0 90000001002a3410 9000000002df2f30
[   12.416131]         ...
[   12.416138] Call Trace:
[   12.416142] [<900000000355f410>] strscpy+0xf0/0x160
[   12.416167] [<90000000031579b8>] thermal_add_hwmon_sysfs+0x258/0x300
[   12.416183] [<9000000003157ab0>] devm_thermal_add_hwmon_sysfs+0x50/0xe0
[   12.416200] [<ffffff80020cc1e8>] loongson2_thermal_probe+0x128/0x200 [loongson2_thermal]
[   12.416232] [<9000000002df6808>] platform_probe+0x68/0x140
[   12.416249] [<9000000002df2ba8>] really_probe+0xc8/0x3c0
[   12.416269] [<9000000002df2f30>] __driver_probe_device+0x90/0x180
[   12.416286] [<9000000002df3058>] driver_probe_device+0x38/0x160
[   12.416302] [<9000000002df33a8>] __driver_attach+0xa8/0x200
[   12.416314] [<9000000002deffec>] bus_for_each_dev+0x8c/0x120
[   12.416330] [<9000000002df198c>] bus_add_driver+0x10c/0x2a0
[   12.416346] [<9000000002df46b4>] driver_register+0x74/0x160
[   12.416358] [<90000000022201a4>] do_one_initcall+0x84/0x220
[   12.416372] [<90000000022f3ab8>] do_init_module+0x58/0x2c0
[   12.416386] [<90000000022f6538>] init_module_from_file+0x98/0x100
[   12.416399] [<90000000022f67f0>] sys_finit_module+0x230/0x3c0
[   12.416412] [<900000000358f7c8>] do_syscall+0x88/0xc0
[   12.416431] [<900000000222137c>] handle_syscall+0xbc/0x158

Fixes: e7e3a7c35791 ("thermal/drivers/loongson-2: Add thermal management support")
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/thermal/loongson2_thermal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
index 133098dc0854..99ca0c7bc41c 100644
--- a/drivers/thermal/loongson2_thermal.c
+++ b/drivers/thermal/loongson2_thermal.c
@@ -127,7 +127,7 @@ static int loongson2_thermal_probe(struct platform_device *pdev)
 		if (!IS_ERR(tzd))
 			break;
 
-		if (PTR_ERR(tzd) != ENODEV)
+		if (PTR_ERR(tzd) != -ENODEV)
 			continue;
 
 		return dev_err_probe(dev, PTR_ERR(tzd), "failed to register");
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues
  2023-10-31 11:05 ` [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues Binbin Zhou
@ 2023-10-31 16:58   ` Conor Dooley
  2023-11-01  1:38     ` Binbin Zhou
  0 siblings, 1 reply; 7+ messages in thread
From: Conor Dooley @ 2023-10-31 16:58 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Binbin Zhou, Huacai Chen, Rafael J . Wysocki, Daniel Lezcano,
	Amit Kucheria, Zhang Rui, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Huacai Chen, loongson-kernel, linux-pm, devicetree,
	Yinbo Zhu, WANG Xuerui, loongarch

[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]

On Tue, Oct 31, 2023 at 07:05:49PM +0800, Binbin Zhou wrote:
> Add the missing 'thermal-sensor-cells' property which is required for
> every thermal sensor as it's used when using phandles.
> And add the thermal-sensor.yaml reference.
> 
> Fixes: 72684d99a854 ("thermal: dt-bindings: add loongson-2 thermal")
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  .../bindings/thermal/loongson,ls2k-thermal.yaml        | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> index 7538469997f9..b634f57cd011 100644
> --- a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> +++ b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> @@ -10,6 +10,9 @@ maintainers:
>    - zhanghongchen <zhanghongchen@loongson.cn>
>    - Yinbo Zhu <zhuyinbo@loongson.cn>
>  
> +allOf:
> +  - $ref: /schemas/thermal/thermal-sensor.yaml#
> +
>  properties:
>    compatible:
>      oneOf:
> @@ -26,12 +29,16 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  '#thermal-sensor-cells':
> +    const: 1
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
> +  - '#thermal-sensor-cells'

Why does it need to be a required property now though?
Adding new required properties is technically an ABI break.

Cheers,
Conor.

>  
> -additionalProperties: false
> +unevaluatedProperties: false
>  
>  examples:
>    - |
> @@ -41,4 +48,5 @@ examples:
>          reg = <0x1fe01500 0x30>;
>          interrupt-parent = <&liointc0>;
>          interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> +        #thermal-sensor-cells = <1>;
>      };
> -- 
> 2.39.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues
  2023-10-31 16:58   ` Conor Dooley
@ 2023-11-01  1:38     ` Binbin Zhou
  2023-11-01  7:59       ` Conor Dooley
  0 siblings, 1 reply; 7+ messages in thread
From: Binbin Zhou @ 2023-11-01  1:38 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Binbin Zhou, Huacai Chen, Rafael J . Wysocki, Daniel Lezcano,
	Amit Kucheria, Zhang Rui, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Huacai Chen, loongson-kernel, linux-pm, devicetree,
	Yinbo Zhu, WANG Xuerui, loongarch

On Tue, Oct 31, 2023 at 10:58 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Tue, Oct 31, 2023 at 07:05:49PM +0800, Binbin Zhou wrote:
> > Add the missing 'thermal-sensor-cells' property which is required for
> > every thermal sensor as it's used when using phandles.
> > And add the thermal-sensor.yaml reference.
> >
> > Fixes: 72684d99a854 ("thermal: dt-bindings: add loongson-2 thermal")
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  .../bindings/thermal/loongson,ls2k-thermal.yaml        | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > index 7538469997f9..b634f57cd011 100644
> > --- a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > +++ b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > @@ -10,6 +10,9 @@ maintainers:
> >    - zhanghongchen <zhanghongchen@loongson.cn>
> >    - Yinbo Zhu <zhuyinbo@loongson.cn>
> >
> > +allOf:
> > +  - $ref: /schemas/thermal/thermal-sensor.yaml#
> > +
> >  properties:
> >    compatible:
> >      oneOf:
> > @@ -26,12 +29,16 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >
> > +  '#thermal-sensor-cells':
> > +    const: 1
> > +
> >  required:
> >    - compatible
> >    - reg
> >    - interrupts
> > +  - '#thermal-sensor-cells'
>
> Why does it need to be a required property now though?
> Adding new required properties is technically an ABI break.

Hi Conor:

I don't think it makes sense to have a separate thermal sensor
definition, it needs thermal-zones to describe specific behaviors,
e.g. cpu-thermal, so we need '#thermal-sensor-cells' to specify the
reference.
And the Loongson-2K1000 has 4 sets of control registers, we need to
specify the id when referencing it.

Thanks.
Binbin
>
> Cheers,
> Conor.
>
> >
> > -additionalProperties: false
> > +unevaluatedProperties: false
> >
> >  examples:
> >    - |
> > @@ -41,4 +48,5 @@ examples:
> >          reg = <0x1fe01500 0x30>;
> >          interrupt-parent = <&liointc0>;
> >          interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> > +        #thermal-sensor-cells = <1>;
> >      };
> > --
> > 2.39.3
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues
  2023-11-01  1:38     ` Binbin Zhou
@ 2023-11-01  7:59       ` Conor Dooley
  2023-11-15  6:50         ` Binbin Zhou
  0 siblings, 1 reply; 7+ messages in thread
From: Conor Dooley @ 2023-11-01  7:59 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Binbin Zhou, Huacai Chen, Rafael J . Wysocki, Daniel Lezcano,
	Amit Kucheria, Zhang Rui, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Huacai Chen, loongson-kernel, linux-pm, devicetree,
	Yinbo Zhu, WANG Xuerui, loongarch

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

On Wed, Nov 01, 2023 at 07:38:39AM +0600, Binbin Zhou wrote:
> On Tue, Oct 31, 2023 at 10:58 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Tue, Oct 31, 2023 at 07:05:49PM +0800, Binbin Zhou wrote:
> > > Add the missing 'thermal-sensor-cells' property which is required for
> > > every thermal sensor as it's used when using phandles.
> > > And add the thermal-sensor.yaml reference.
> > >
> > > Fixes: 72684d99a854 ("thermal: dt-bindings: add loongson-2 thermal")
> > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > ---
> > >  .../bindings/thermal/loongson,ls2k-thermal.yaml        | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > index 7538469997f9..b634f57cd011 100644
> > > --- a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > +++ b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > @@ -10,6 +10,9 @@ maintainers:
> > >    - zhanghongchen <zhanghongchen@loongson.cn>
> > >    - Yinbo Zhu <zhuyinbo@loongson.cn>
> > >
> > > +allOf:
> > > +  - $ref: /schemas/thermal/thermal-sensor.yaml#
> > > +
> > >  properties:
> > >    compatible:
> > >      oneOf:
> > > @@ -26,12 +29,16 @@ properties:
> > >    interrupts:
> > >      maxItems: 1
> > >
> > > +  '#thermal-sensor-cells':
> > > +    const: 1
> > > +
> > >  required:
> > >    - compatible
> > >    - reg
> > >    - interrupts
> > > +  - '#thermal-sensor-cells'
> >
> > Why does it need to be a required property now though?
> > Adding new required properties is technically an ABI break.
> 
> Hi Conor:
> 
> I don't think it makes sense to have a separate thermal sensor
> definition, it needs thermal-zones to describe specific behaviors,
> e.g. cpu-thermal, so we need '#thermal-sensor-cells' to specify the
> reference.
> And the Loongson-2K1000 has 4 sets of control registers, we need to
> specify the id when referencing it.

Unfortunately, none of this is an answer to my question.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues
  2023-11-01  7:59       ` Conor Dooley
@ 2023-11-15  6:50         ` Binbin Zhou
  2023-11-15 14:54           ` Conor Dooley
  0 siblings, 1 reply; 7+ messages in thread
From: Binbin Zhou @ 2023-11-15  6:50 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Binbin Zhou, Huacai Chen, Rafael J . Wysocki, Daniel Lezcano,
	Amit Kucheria, Zhang Rui, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Huacai Chen, loongson-kernel, linux-pm, devicetree,
	Yinbo Zhu, WANG Xuerui, loongarch

On Wed, Nov 1, 2023 at 1:59 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Nov 01, 2023 at 07:38:39AM +0600, Binbin Zhou wrote:
> > On Tue, Oct 31, 2023 at 10:58 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Tue, Oct 31, 2023 at 07:05:49PM +0800, Binbin Zhou wrote:
> > > > Add the missing 'thermal-sensor-cells' property which is required for
> > > > every thermal sensor as it's used when using phandles.
> > > > And add the thermal-sensor.yaml reference.
> > > >
> > > > Fixes: 72684d99a854 ("thermal: dt-bindings: add loongson-2 thermal")
> > > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > > ---
> > > >  .../bindings/thermal/loongson,ls2k-thermal.yaml        | 10 +++++++++-
> > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > index 7538469997f9..b634f57cd011 100644
> > > > --- a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > +++ b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > @@ -10,6 +10,9 @@ maintainers:
> > > >    - zhanghongchen <zhanghongchen@loongson.cn>
> > > >    - Yinbo Zhu <zhuyinbo@loongson.cn>
> > > >
> > > > +allOf:
> > > > +  - $ref: /schemas/thermal/thermal-sensor.yaml#
> > > > +
> > > >  properties:
> > > >    compatible:
> > > >      oneOf:
> > > > @@ -26,12 +29,16 @@ properties:
> > > >    interrupts:
> > > >      maxItems: 1
> > > >
> > > > +  '#thermal-sensor-cells':
> > > > +    const: 1
> > > > +
> > > >  required:
> > > >    - compatible
> > > >    - reg
> > > >    - interrupts
> > > > +  - '#thermal-sensor-cells'
> > >
> > > Why does it need to be a required property now though?
> > > Adding new required properties is technically an ABI break.
> >
> > Hi Conor:
> >
> > I don't think it makes sense to have a separate thermal sensor
> > definition, it needs thermal-zones to describe specific behaviors,
> > e.g. cpu-thermal, so we need '#thermal-sensor-cells' to specify the
> > reference.
> > And the Loongson-2K1000 has 4 sets of control registers, we need to
> > specify the id when referencing it.
>
> Unfortunately, none of this is an answer to my question.

Hi Conor:

Sorry for my late reply.

Over the past few days, I've been communicating offline with Yinbo
(the driver author) about the use of the '#thermal-sensor-cells'
attribute. He retested the attribute and determined that it is
'required'.

We can see that the '#thermal-sensor-cells' attribute in the
dt-binding was dropped between the V12 patchset[1] and the V13
patchset[2]. Yinbo may have misunderstood Daniel's comment and removed
the '#thermal-sensor-cells' attribute from the dt-binding. But the
attribute was carelessly still left in the dts file, resulting in the
issue not being found during functional validation.

Indeed, re-adding the '#thermal-sensor-cells' attribute as "required"
is technically an ABI breakage, but the driver does not work properly
under the current dt-binding rules.
Also, the driver is only valid under LoongArch and will have no effect
on other architectures.

[1]:https://lore.kernel.org/all/20221114024709.7975-2-zhuyinbo@loongson.cn/
[2]:https://lore.kernel.org/all/20230221095355.9799-2-zhuyinbo@loongson.cn/

Thanks.
Binbin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues
  2023-11-15  6:50         ` Binbin Zhou
@ 2023-11-15 14:54           ` Conor Dooley
  0 siblings, 0 replies; 7+ messages in thread
From: Conor Dooley @ 2023-11-15 14:54 UTC (permalink / raw)
  To: Binbin Zhou
  Cc: Binbin Zhou, Huacai Chen, Rafael J . Wysocki, Daniel Lezcano,
	Amit Kucheria, Zhang Rui, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Huacai Chen, loongson-kernel, linux-pm, devicetree,
	Yinbo Zhu, WANG Xuerui, loongarch

[-- Attachment #1: Type: text/plain, Size: 3757 bytes --]

On Wed, Nov 15, 2023 at 12:50:41PM +0600, Binbin Zhou wrote:
> On Wed, Nov 1, 2023 at 1:59 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, Nov 01, 2023 at 07:38:39AM +0600, Binbin Zhou wrote:
> > > On Tue, Oct 31, 2023 at 10:58 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Tue, Oct 31, 2023 at 07:05:49PM +0800, Binbin Zhou wrote:
> > > > > Add the missing 'thermal-sensor-cells' property which is required for
> > > > > every thermal sensor as it's used when using phandles.
> > > > > And add the thermal-sensor.yaml reference.
> > > > >
> > > > > Fixes: 72684d99a854 ("thermal: dt-bindings: add loongson-2 thermal")
> > > > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > > > ---
> > > > >  .../bindings/thermal/loongson,ls2k-thermal.yaml        | 10 +++++++++-
> > > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > > index 7538469997f9..b634f57cd011 100644
> > > > > --- a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > > +++ b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > > @@ -10,6 +10,9 @@ maintainers:
> > > > >    - zhanghongchen <zhanghongchen@loongson.cn>
> > > > >    - Yinbo Zhu <zhuyinbo@loongson.cn>
> > > > >
> > > > > +allOf:
> > > > > +  - $ref: /schemas/thermal/thermal-sensor.yaml#
> > > > > +
> > > > >  properties:
> > > > >    compatible:
> > > > >      oneOf:
> > > > > @@ -26,12 +29,16 @@ properties:
> > > > >    interrupts:
> > > > >      maxItems: 1
> > > > >
> > > > > +  '#thermal-sensor-cells':
> > > > > +    const: 1
> > > > > +
> > > > >  required:
> > > > >    - compatible
> > > > >    - reg
> > > > >    - interrupts
> > > > > +  - '#thermal-sensor-cells'
> > > >
> > > > Why does it need to be a required property now though?
> > > > Adding new required properties is technically an ABI break.
> > >
> > > Hi Conor:
> > >
> > > I don't think it makes sense to have a separate thermal sensor
> > > definition, it needs thermal-zones to describe specific behaviors,
> > > e.g. cpu-thermal, so we need '#thermal-sensor-cells' to specify the
> > > reference.
> > > And the Loongson-2K1000 has 4 sets of control registers, we need to
> > > specify the id when referencing it.
> >
> > Unfortunately, none of this is an answer to my question.
> 
> Hi Conor:
> 
> Sorry for my late reply.
> 
> Over the past few days, I've been communicating offline with Yinbo
> (the driver author) about the use of the '#thermal-sensor-cells'
> attribute. He retested the attribute and determined that it is
> 'required'.
> 
> We can see that the '#thermal-sensor-cells' attribute in the
> dt-binding was dropped between the V12 patchset[1] and the V13
> patchset[2]. Yinbo may have misunderstood Daniel's comment and removed
> the '#thermal-sensor-cells' attribute from the dt-binding. But the
> attribute was carelessly still left in the dts file, resulting in the
> issue not being found during functional validation.
> 
> Indeed, re-adding the '#thermal-sensor-cells' attribute as "required"
> is technically an ABI breakage, but the driver does not work properly
> under the current dt-binding rules.

I was going to say that you should add some comment about this in the
commit message, but at the end of the day - you're not much of a thermal
sensor without having thermal-sensor-cells, so I think your commit
message is actually fine.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

and I probably should have recognised that earlier.

Thanks,
Conor

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-11-15 14:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <cover.1698743706.git.zhoubinbin@loongson.cn>
2023-10-31 11:05 ` [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues Binbin Zhou
2023-10-31 16:58   ` Conor Dooley
2023-11-01  1:38     ` Binbin Zhou
2023-11-01  7:59       ` Conor Dooley
2023-11-15  6:50         ` Binbin Zhou
2023-11-15 14:54           ` Conor Dooley
2023-10-31 11:05 ` [PATCH 2/2] drivers/thermal/loongson2_thermal: Fix incorrect PTR_ERR() judgment Binbin Zhou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).