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;
Date:   Wed, 17 May 2023 11:27:27 +0530
From:   Varadarajan Narayanan <quic_varada@quicinc.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
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>
Subject: Re: [PATCH v3 2/4] dt-bindings: thermal: tsens: Add ipq9574
 compatible
Message-ID: <20230517055726.GA3165@varda-linux.qualcomm.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>
 <1999753b-ceee-d66c-9a48-cbcbb8e6236e@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <1999753b-ceee-d66c-9a48-cbcbb8e6236e@linaro.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Tue, May 16, 2023 at 03:06:40PM +0200, Krzysztof Kozlowski wrote:
> 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?

Not sure if I didn't express properly. There are two parts to this patch.

Part-1 is adding the 'const' entries at the beginning i.e.

	+      - const: qcom,tsens-v0_1
	+      - const: qcom,tsens-v1
	+      - const: qcom,tsens-v2
	+      - const: qcom,ipq8074-tsens

Part-2 is changing from one valid syntax to another i.e.

	+        items:
	+          - enum:
	+              - qcom,ipq9574-tsens
	+          - const: qcom,ipq8074-tsens

Without both of the above changes, either or both of dtbs_check
& dt_binding_check fails. So, it is not possible to just add the
"valid hunk" (part-2) alone.

If having both part-1 and part-2 in the same patch is not
acceptable, shall I split them into two patches? Please let me know.

Thanks
Varada