| Message ID | 944559ea3bf7ba0a1540f831ccd7d33591622b22.1698743706.git.zhoubinbin@loongson.cn |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 31 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 >
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 > >
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.
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
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
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>; };
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(-)