Re: [PATCH v14 3/4] dt-bindings: msm: dsi: add yaml schemas for DSI PHY bindings

[Rob Herring <robh@kernel.org>]

On Tue, Mar 30, 2021 at 02:52:29PM +0530, mkrishn@codeaurora.org wrote:
> On 2021-03-29 08:49, Stephen Boyd wrote:
> > Quoting mkrishn@codeaurora.org (2021-03-26 03:36:30)
> > > On 2021-03-26 04:28, Stephen Boyd wrote:
> > > > Quoting Krishna Manikandan (2021-03-25 05:01:00)
> > > >> diff --git
> > > >> a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> > > >> b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> > > >> new file mode 100644
> > > >> index 0000000..4a26bef
> > > >> --- /dev/null
> > > >> +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> > > >> @@ -0,0 +1,68 @@
> > > >> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > > >> +%YAML 1.2
> > > >> +---
> > > >> +$id: http://devicetree.org/schemas/display/msm/dsi-phy-10nm.yaml#
> > > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > >> +
> > > >> +title: Qualcomm Display DSI 10nm PHY
> > > >> +
> > > >> +maintainers:
> > > >> +  - Krishna Manikandan <mkrishn@codeaurora.org>
> > > >> +
> > > >> +allOf:
> > > >> +  - $ref: dsi-phy-common.yaml#
> > > >> +
> > > >> +properties:
> > > >> +  compatible:
> > > >> +    oneOf:
> > [..]
> > > >> and
> > > >> +      connected to VDDA_MIPI_DSI_0_PLL_0P9 pin for sdm845 target
> > > >> +
> > > >> +required:
> > > >> +  - compatible
> > > >> +  - reg
> > > >> +  - reg-names
> > > >> +  - vdds-supply
> > > >> +
> > > >> +unevaluatedProperties: false
> > > >
> > > > additionalProperties: false instead? This comment applies to the other
> > > > bindings in this patch.
> > > 
> > > Hi Stephen,
> > > We are referencing dsi-phy-common.yaml in this file. Since the
> > > properties of dsi-phy-common.yaml are applicable to this file also, I
> > > added unevaluatedProperties: false. If we add additionalProperties:
> > > false instead, then the properties of dsi-phy-common.yaml will not be
> > > applicable here and this will throw an error if we add the properties
> > > from dsi-phy-common.yaml in the example.
> > > 
> > 
> > Does that matter? I was wondering about that and so I peeked at the
> > qcom pinctrl binding and it seems to follow a similar design but doesn't
> > have unevaluatedProperties: false. Instead it has additionalProperies:
> > false. See qcom,sc8180x-pinctrl.yaml for an example. So did you try it
> > or does something say you can't do this?
> 
> Hi Stephen,
> I had tried the same thing in one of my initial patches and I got a comment
> from Rob that we have to use unevaluatedProperties when we are referring
> another file(https://patchwork.kernel.org/project/linux-arm-msm/patch/1589868421-30062-1-git-send-email-mkrishn@codeaurora.org/)

Maybe I had a wrong assumption that you needed the child nodes too?

> In latest dt-schema tool, we will get error if we try to change it to
> additionalProperties: false.
> For example, in this patch "#clock-cells' and '#phy-cells' are mentioned in
> dsi-phy-common.yaml and I am referring this file in dsi-phy-10nm.yaml. If I
> add
> additionalProperties: false instead of unevaluatedProperties: false, I will
> get the error mentioned below.
> 
> I checked qcom,sc8180x-pinctrl.yaml that you had mentioned in the comment
> and this file is compiling without any issues even though it is using
> additionalProperties: false. But I see that the properties mentioned in the
> reference file (in this case, qcom,tlmm-common.yaml) are again declared in
> the main file qcom,sc8180x-pinctrl.yaml even though these are mentioned as
> required properties in the common yaml file. If I remove these properties
> from qcom,sc8180x-pinctrl.yaml, I can see the same error that I am getting
> for my file also if additionalProperties are used. If I follow the same
> approach , ie define the properties again in dsi-phy-10nm.yaml and add
> additionalProperties: false, I dont see any errors during check (working
> change mentioned below). Should I make this change for all the files?
> 
> Error logs:
> mkrishn@mkrishn-linux:/local/mnt/workspace/linux-next-latest/linux-next$
> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
>   CHKDT   Documentation/devicetree/bindings/processed-schema-examples.json
>   SCHEMA  Documentation/devicetree/bindings/processed-schema-examples.json
>   DTEX
> Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.example.dts
>   DTC
> Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.example.dt.yaml
>   CHECK
> Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.example.dt.yaml
> /local/mnt/workspace/linux-next-latest/linux-next/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.example.dt.yaml:
> dsi-phy@ae94400: '#clock-cells', '#phy-cells', 'clock-names', 'clocks' do
> not match any of the regexes: 'pinctrl-[0-9]+'
>         From schema: /local/mnt/workspace/linux-next-latest/linux-next/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> 
> Working Change:
> --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-10nm.yaml
> @@ -30,6 +30,11 @@ properties:
>        - const: dsi_phy_lane
>        - const: dsi_pll
> 
> +  '#clock-cells': true
> +  '#phy-cells': true
> +  clocks: true
> +  clock-names: true
> +
>    vdds-supply:
>      description: |
>        Connected to DSI0_MIPI_DSI_PLL_VDDA0P9 pin for sc7180 target and
> @@ -41,7 +46,7 @@ required:
>    - reg-names
>    - vdds-supply
> 
> -unevaluatedProperties: false
> +additionalProperties: false

This works if you want to use some, but not all properties in a 
referenced schema. If all apply or listing them all here is too much 
duplication (such as child nodes, but that's a judgement call), then use 
'unevaluatedProperties'.

unevaluatedProperties is also currently a nop because the underlying 
tools don't yet support it. So it won't catch any errors and those 
errors will all have to be fixed when the tools add support.

Rob