Re: Re: [PATCH v2 2/3] dt-bindings: thermal: Convert loongson2 to json-schema

[Rob Herring <robh@kernel.org>]

On Thu, Sep 22, 2022 at 09:39:30AM +0800, 朱银波 wrote:
> 
> 
> 
> > -----原始邮件-----
> > 发件人: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
> > 发送时间:2022-09-21 17:31:11 (星期三)
> > 收件人: "朱银波" <zhuyinbo@loongson.cn>
> > 抄送: "Rafael J . Wysocki" <rafael@kernel.org>, "Daniel Lezcano" <daniel.lezcano@linaro.org>, "Amit Kucheria" <amitk@kernel.org>, "Zhang Rui" <rui.zhang@intel.com>, linux-pm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, zhanghongchen <zhanghongchen@loongson.cn>
> > 主题: Re: [PATCH v2 2/3] dt-bindings: thermal: Convert loongson2 to json-schema
> > 
> > On 21/09/2022 11:22, 朱银波 wrote:
> > >> -----原始邮件-----
> > >> 发件人: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
> > >> 发送时间:2022-09-21 15:05:00 (星期三)
> > >> 收件人: "Yinbo Zhu" <zhuyinbo@loongson.cn>, "Rafael J . Wysocki" <rafael@kernel.org>, "Daniel Lezcano" <daniel.lezcano@linaro.org>, "Amit Kucheria" <amitk@kernel.org>, "Zhang Rui" <rui.zhang@intel.com>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, linux-pm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
> > >> 抄送: zhanghongchen <zhanghongchen@loongson.cn>
> > >> 主题: Re: [PATCH v2 2/3] dt-bindings: thermal: Convert loongson2 to json-schema
> > >>
> > >> On 21/09/2022 03:56, Yinbo Zhu wrote:
> > >>> Convert the loongson2 thermal binding to DT schema format using
> > >>> json-schema.
> > >>
> > >> Incorrect subject and incorrect commit msg. There is no conversion here.
> > > Our soc architecture is the loongson2 series, so we will modify it accordingly.
> > 
> > How the soc architecture is related to my comment that you do not
> > perform conversion?
> I got it, and I will aad a conversion.
> > 
> > > 
> > >>
> > >>>
> > >>> Signed-off-by: Yinbo Zhu <c>
> > >>> ---
> > >>> Change in v2:
> > >>> 		1. Add description and type about the "id".	
> > >>> 		2. Make the filename was based on compatible.
> > >>>
> > >>>  .../bindings/thermal/loongson2-thermal.yaml   | 52 +++++++++++++++++++
> > >>>  1 file changed, 52 insertions(+)
> > >>>  create mode 100644 Documentation/devicetree/bindings/thermal/loongson2-thermal.yaml
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/thermal/loongson2-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson2-thermal.yaml
> > >>> new file mode 100644
> > >>> index 000000000000..2994ae3a56aa
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/thermal/loongson2-thermal.yaml
> > >>
> > >>
> > >> No improvements here. You ignore my comments, so I am going to NAK it.
> > > I don't get your point, that dts compatible is "loongson,loongson2-thermal", so this driver file name is named
> > > loongson2-thermal that according what you said about "Filename based on compatible."
> > > If what I understand is not what you expect, please tell me how to modify it.
> > 
> > 
> > Filename must match the compatible, so: loongson,loongson2-thermal.yaml
> I got it, and I will add a conversion.
> > 
> > >>
> > >>
> > >>> @@ -0,0 +1,52 @@
> > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >>> +%YAML 1.2
> > >>> +---
> > >>> +$id: http://devicetree.org/schemas/thermal/loongson2-thermal.yaml#
> > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>> +
> > >>> +title: Thermal sensors on loongson2 SoCs
> > >>> +
> > >>> +maintainers:
> > >>> +  - zhanghongchen <zhanghongchen@loongson.cn>
> > >>> +  - Yinbo Zhu <zhuyinbo@loongson.cn>
> > >>> +
> > >>> +properties:
> > >>> +  compatible:
> > >>> +    const: loongson,loongson2-thermal
> > >>> +
> > >>> +  reg:
> > >>> +    maxItems: 1
> > >>> +
> > >>> +  id:
> > >>> +    $ref: '//schemas/types.yaml#/definitions/uint32'
> > >>
> > >> No improvements here, so let me be specific - you need to really justify
> > >> such property or it cannot go to schema.
> > > The loongson2_thermal.c driver need parse this "id" property.
> > 
> > This is not reason to add properties to DT. DT describes the hardware,
> > not driver behavior.
> > 
> > Why hardware needs arbitrary, additional addressing number instead of
> > standard unit address?
> The loongson2 series soc supports up to four sensors, but the 2K1000 has only one sensor, so the ID must be 0. 
> For the 2K1000, in order to distinguish the differences between different hardware in the Loongson2 SoC series,
> the ID is added to the dts

Differences in SoCs is what 'compatible' is for. If 'loongson2' is not a 
specific SoC, then your compatible string is not specific enough.

> > 
> > >>
> > >>> +    description: |
> > >>> +      Specify the thermal sensor id.
> > >>> +    minimum: 0
> > >>> +    maximum: 3
> > >>> +
> > >>> +  interrupts:
> > >>> +    maxItems: 1
> > >>> +
> > >>> +  "#thermal-sensor-cells":
> > >>> +    const: 1

If one SoC only has 1 sensor, then this could be 0. However, you don't 
have to do that, but it's another way to distinguish differences.

> > >>> +
> > >>> +required:
> > >>> +  - compatible
> > >>> +  - reg
> > >>> +  - id
> > >>> +  - interrupt-parent
> > >>
> > >> Why?
> > > The interrupts of our dts do not specify an interrupt parent,
> > > eg. interrupts = <7 IRQ_TYPE_LEVEL_LOW>
> > > so we need to add an interrupt parent property.
> > 
> > You can add but I am asking why is it required?
> Since there is more than one interrupt controller in the Loongson2 series soc, that need to specify the interrupt 
> controller in the dts, that is, the interrupt parent.   If different interrupt parents are used in dts, the interrupt 
> numbers are different.

It is perfectly valid for the 'interrupt-parent' to be in *any* parent 
node. So it is never required by any binding.

Rob