Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1999753b-ceee-d66c-9a48-cbcbb8e6236e@linaro.org>
Date:   Tue, 16 May 2023 15:06:40 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v3 2/4] dt-bindings: thermal: tsens: Add ipq9574
 compatible
Content-Language: en-US
To:     Varadarajan Narayanan <quic_varada@quicinc.com>
Cc:     agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        amitk@kernel.org, thara.gopinath@gmail.com, rafael@kernel.org,
        daniel.lezcano@linaro.org, rui.zhang@intel.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Praveenkumar I <quic_ipkumar@quicinc.com>
References: <cover.1684140883.git.quic_varada@quicinc.com>
 <37adcf5d8d545a076e8ed971a4fb6c6c2833ef3c.1684140883.git.quic_varada@quicinc.com>
 <b7e749ff-f4f0-0e61-9aae-876db4278fbc@linaro.org>
 <20230516120426.GA1679@varda-linux.qualcomm.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230516120426.GA1679@varda-linux.qualcomm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 16/05/2023 14:04, Varadarajan Narayanan wrote:
> On Mon, May 15, 2023 at 06:10:29PM +0200, Krzysztof Kozlowski wrote:
>> On 15/05/2023 12:13, Varadarajan Narayanan wrote:
>>> From: Praveenkumar I <quic_ipkumar@quicinc.com>
>>>
>>> Qualcomm IPQ9574 has tsens v2.3.1 block, which is similar to IPQ8074 tsens.
>>>
>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>>> ---
>>> [v3]:
>>> 	Fix dt_binding_check & dtbs_check errors (Used
>>> 	Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml
>>> 	as reference/example)
>>>
>>> 	Drop 'Acked-by: Rob Herring' as suggested in review
>>>
>>> [v2]:
>>> 	Thanks to Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> 	for the tip to make qcom,ipq8074-tsens as fallback.
>>> ---
>>>  Documentation/devicetree/bindings/thermal/qcom-tsens.yaml | 13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> index d9aa54c..57e3908 100644
>>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> @@ -19,6 +19,11 @@ description: |
>>>  properties:
>>>    compatible:
>>>      oneOf:
>>> +      - const: qcom,tsens-v0_1
>>> +      - const: qcom,tsens-v1
>>> +      - const: qcom,tsens-v2
>>
>> Nope, these are not correct.
>>
>>> +      - const: qcom,ipq8074-tsens
>>
>> Also nope, this is already there.
>>
>>> +
>>>        - description: msm8960 TSENS based
>>>          items:
>>>            - enum:
>>> @@ -66,8 +71,10 @@ properties:
>>>            - const: qcom,tsens-v2
>>>
>>>        - description: v2 of TSENS with combined interrupt
>>> -        enum:
>>> -          - qcom,ipq8074-tsens
>>
>> Why?
>>
>>> +        items:
>>> +          - enum:
>>> +              - qcom,ipq9574-tsens
>>> +          - const: qcom,ipq8074-tsens
> 
> Without changing it like this either dtbs_check or
> dt_binding_check kept failing.
> 
> 	- description: v2 of TSENS with combined interrupt
> 	  enum:
> 	    - qcom,ipq8074-tsens
> 	    - qcom,ipq9574-tsens

But we do not talk about this... Look, I commented out under specific
hunks which are not correct. Not under the hunk which is correct.

> 
> dtbs_check gave this kind of error
> 	['qcom,ipq9574-tsens', 'qcom,ipq8074-tsens'] is too long
> 
> After changing it like in https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31
> 
> 	- description: v2 of TSENS with combined interrupt
> 	  const: qcom,ipq8074-tsens
> 	  - enum:
> 	      - qcom,ipq9574-tsens
> 	  - const: qcom,ipq8074-tsens
> 
> dt_binding_check gives the following error
> 
> 	Documentation/devicetree/bindings/thermal/qcom-tsens.yaml:70:9: did not find expected key

Because it is not even valid syntax.

> 
> and dtbs_check gives
> 
> 	./Documentation/devicetree/bindings/thermal/qcom-tsens.yaml:70:9: [error] syntax error: expected <block end>, but found '-' (syntax)
> 	  CHKDT   Documentation/devicetree/bindings/processed-schema.json
> 	./Documentation/devicetree/bindings/clock/qcom,gcc-ipq8064.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/thermal/qcom-tsens.yaml
> 	./Documentation/devicetree/bindings/clock/qcom,gcc-apq8064.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/thermal/qcom-tsens.yaml
> 	./Documentation/devicetree/bindings/thermal/qcom-tsens.yaml:70:9: did not find expected key
> 	  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> 	/local/mnt/workspace/varada/v3/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml: ignoring, error parsing file
> 
> If i change it like below,
> 
> 	- description: v2 of TSENS with combined interrupt
> 	  enum:
> 	    - qcom,ipq9574-tsens
> 	  - const: qcom,ipq8074-tsens
> 
> dt_binding_check and dtbs_check gives same error as above.
> 
> Looked around and found Documentation/devicetree/bindings/display/allwinner,sun4i-a10-tcon.yaml
> which seemed to do something similar to what is wanted in this
> case. Hence changed qcom-tsens.yaml similar to the allwinner yaml
> file. After which dt_binding_check and dtbs_check passed. Please
> let me know if there is a better way to solve this. Will go with

Changing one valid syntax to another valid syntax is not related to the
patch. If you think such change as reasonable, please split it, but to
me it does not look justified. As for actual change, so adding new
compatible, it's not really related to the others. Why you cannot add
the proper list (so the only valid hunk) and that's it?

Best regards,
Krzysztof