Re: [PATCH v6 01/12] dt-bindings: display/msm: split qcom,mdss bindings

[Dmitry Baryshkov]

On 08/09/2022 22:37, Rob Herring wrote:
On Thu, Sep 08, 2022 at 03:37:38PM +0200, Krzysztof Kozlowski wrote:
On 01/09/2022 12:23, Dmitry Baryshkov wrote:
Split Mobile Display SubSystem (MDSS) root node bindings to the separate
yaml file. Changes to the existing (txt) schema:
  - Added optional "vbif_nrt_phys" region used by msm8996
  - Made "bus" and "vsync" clocks optional (they are not used by some
    platforms)
  - Added (optional) "core" clock added recently to the mdss driver
  - Added optional resets property referencing MDSS reset
  - Defined child nodes pointing to corresponding reference schema.
  - Dropped the "lut" clock. It was added to the schema by mistake (it is
    a part of mdp4 schema, not the mdss).
Signed-off-by: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
---
  .../devicetree/bindings/display/msm/mdp5.txt  |  30 +---
  .../devicetree/bindings/display/msm/mdss.yaml | 166 ++++++++++++++++++
  2 files changed, 167 insertions(+), 29 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/display/msm/mdss.yaml

diff --git a/Documentation/devicetree/bindings/display/msm/mdp5.txt 
b/Documentation/devicetree/bindings/display/msm/mdp5.txt
index 43d11279c925..65d03c58dee6 100644
--- a/Documentation/devicetree/bindings/display/msm/mdp5.txt
+++ b/Documentation/devicetree/bindings/display/msm/mdp5.txt
@@ -2,37 +2,9 @@ Qualcomm adreno/snapdragon MDP5 display controller
  
  Description:
  
-This is the bindings documentation for the Mobile Display Subsytem(MDSS) that
-encapsulates sub-blocks like MDP5, DSI, HDMI, eDP etc, and the MDP5 display
+This is the bindings documentation for the MDP5 display
  controller found in SoCs like MSM8974, APQ8084, MSM8916, MSM8994 and MSM8996.
  
-MDSS:
-Required properties:
-- compatible:
-  * "qcom,mdss" - MDSS
-- reg: Physical base address and length of the controller's registers.
-- reg-names: The names of register regions. The following regions are required:
-  * "mdss_phys"
-  * "vbif_phys"
-- interrupts: The interrupt signal from MDSS.
-- interrupt-controller: identifies the node as an interrupt controller.
-- #interrupt-cells: specifies the number of cells needed to encode an interrupt
-  source, should be 1.
-- power-domains: a power domain consumer specifier according to
-  Documentation/devicetree/bindings/power/power_domain.txt
-- clocks: device clocks. See ../clocks/clock-bindings.txt for details.
-- clock-names: the following clocks are required.
-  * "iface"
-  * "bus"
-  * "vsync"
-- #address-cells: number of address cells for the MDSS children. Should be 1.
-- #size-cells: Should be 1.
-- ranges: parent bus address space is the same as the child bus address space.
-
-Optional properties:
-- clock-names: the following clocks are optional:
-  * "lut"
-
  MDP5:
  Required properties:
  - compatible:
diff --git a/Documentation/devicetree/bindings/display/msm/mdss.yaml 
b/Documentation/devicetree/bindings/display/msm/mdss.yaml
new file mode 100644
index 000000000000..8860fc55cca5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/msm/mdss.yaml
@@ -0,0 +1,166 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/msm/mdss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Mobile Display SubSystem (MDSS)
+
+maintainers:
+  - Dmitry Baryshkov <dmitry.barysh...@linaro.org>
+  - Rob Clark <robdcl...@gmail.com>
+
+description:
+  This is the bindings documentation for the Mobile Display Subsytem(MDSS) that
+  encapsulates sub-blocks like MDP5, DSI, HDMI, eDP, etc.
+
+properties:
+  compatible:
+    enum:
+      - qcom,mdss
+
+  reg:
+    minItems: 2
+    maxItems: 3
+
+  reg-names:
+    minItems: 2
+    items:
+      - const: mdss_phys
+      - const: vbif_phys
+      - const: vbif_nrt_phys
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller:
+    true

If there is going to be v7 - please make it one line.

+
+  "#interrupt-cells":
+    const: 1
+
+  power-domains:
+    maxItems: 1
+    description: |
+      The MDSS power domain provided by GCC
+
+  clocks:
+    minItems: 1
+    items:
+      - description: Display abh clock
+      - description: Display axi clock
+      - description: Display vsync clock
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: iface
+      - const: bus
+      - const: vsync
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+  ranges:
+    true

Ditto.

+
+  resets:
+    items:
+      - description: MDSS_CORE reset
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-controller
+  - "#interrupt-cells"
+  - power-domains
+  - clocks
+  - clock-names
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+
+patternProperties:
+  "^mdp@[1-9a-f][0-9a-f]*$":
+    type: object
+    properties:
+      compatible:
+        const: qcom,mdp5
+
+  "^dsi@[1-9a-f][0-9a-f]*$":
+    type: object
+    properties:
+      compatible:
+        const: qcom,mdss-dsi-ctrl

This should be ref to dsi-controller-main.yaml... or based on previous
Rob's feedback you dropped it everywhere in children?

I don't think I said. I thought about it some, as yes, we normally have
done as you suggested. The downside is with a ref we'll do the whole
validation of the child node twice (unless the referenced schema has a
'select: false') whereas here only 'compatible' is validated twice. This
way also complicates checking for unevaluatedProperties/additionalProperties.

So maybe better to keep with the 'normal' way and make this a ref.

Well.. I tried using $ref in the previous iteration, but then I faced 
the fact that I can not use it. Using the $ref the node gets validated 
even if it is disabled, and we do not want to do this. The nodes are 
usually split in way that regulators are specified in the board DT file. 
Thus disabled dsi/dsi-phy nodes do not get all the required regulators. 
And dt-validate happily dumps tons of warnings for disabled nodes. Thus 
I ended up with the compatible rather than $ref.


--
With best wishes
Dmitry