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

[mkrishn@codeaurora.org]

On 2021-03-31 01:12, Rob Herring wrote:
> 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

Thanks Rob for the clarification. I will make the changes accordingly.

Thanks,
Krishna