Re: [PATCH v3 2/4] dt-bindings: thermal: tsens: Add ipq9574 compatible

Varadarajan Narayanan
2023-05-17T05:57:52Z

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