| Message ID | 944559ea3bf7ba0a1540f831ccd7d33591622b22.1698743706.git.zhoubinbin@loongson.cn |
|---|---|
| State | Accepted |
| Commit | 88071e31e994ee23356674e0c5461b25e2a95cdc |
| Headers | show |
| Series | [1/2] dt-bindings: thermal: loongson,ls2k-thermal: Fix binding check issues | expand |
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 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(-)