Re: [PATCH v1 04/17] dt-bindings: display: mediatek: add MT8195 hdmi bindings

On Thu, 22 Sep 2022 09:18, Krzysztof Kozlowski
[off-list ref] wrote:

On 19/09/2022 18:56, Guillaume Ranquet wrote:

quoted

Add mt8195 SoC bindings for hdmi and hdmi-ddc

Make port1 optional for mt8195 as it only supports HDMI tx for now.
Requires a ddc-i2c-bus phandle.
Requires a power-domains phandle.

Signed-off-by: Guillaume Ranquet <redacted>

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml
index bdaf0b51e68c..abb231a0694b 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml

@@ -21,6 +21,10 @@ properties:
       - mediatek,mt7623-hdmi
       - mediatek,mt8167-hdmi
       - mediatek,mt8173-hdmi
+      - mediatek,mt8195-hdmi
+
+  clocks: true
+  clock-names: true

????
Why is this moved?

quoted

   reg:
     maxItems: 1

@@ -28,20 +32,6 @@ properties:
   interrupts:
     maxItems: 1

-  clocks:
-    items:
-      - description: Pixel Clock
-      - description: HDMI PLL
-      - description: Bit Clock
-      - description: S/PDIF Clock
-
-  clock-names:
-    items:
-      - const: pixel
-      - const: pll
-      - const: bclk
-      - const: spdif

Clock definition with constraints should stay here. You just customize
it per variant.

Clocks are different between the two hardwares, so I've tried moving everything
inside the if/else block.

Is there a better way to express this?

quoted

-
   phys:
     maxItems: 1

@@ -58,6 +48,16 @@ properties:
     description: |
       phandle link and register offset to the system configuration registers.

+  ddc-i2c-bus:
+    $ref: '/schemas/types.yaml#/definitions/phandle'

Drop quotes

quoted

+    description: Phandle to the ddc-i2c device

Isn't this property of panel?

It's a property used in panels and connectors.
But since this IP doesn't use a connector per say, I've added the property here.

Which doesn't sound reasonnable when I'm explaining it like this...
I'll see what I can do to fit a connector and have things look a bit
more standard.

quoted

+
+  power-domains:
+    description:
+      A phandle and PM domain specifier as defined by bindings
+      of the power controller specified by phandle. See
+      Documentation/devicetree/bindings/power/power-domain.yaml for details.

No need for this text. This is standard property. You miss maxItems.

quoted

+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports

@@ -76,7 +76,6 @@ properties:

     required:
       - port@0
-      - port@1

 required:
   - compatible

@@ -86,9 +85,55 @@ required:
   - clock-names
   - phys
   - phy-names
-  - mediatek,syscon-hdmi
   - ports

+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mediatek,mt8195-hdmi
+    then:
+      properties:
+        clocks:
+          items:
+            - description: APB
+            - description: HDCP
+            - description: HDCP 24M
+            - description: Split HDMI
+        clock-names:
+          items:
+            - const: hdmi_apb_sel
+            - const: hdcp_sel
+            - const: hdcp24_sel
+            - const: split_hdmi

Clocks are entirely different. I am not sure there is benefit in keeping
these devices in one bindings.

I agree with that, but it was requested by CK that the driver and bindings be
as common as possible.

quoted

+
+      required:
+        - power-domains
+        - ddc-i2c-bus

Blank line,

quoted

+    else:
+      properties:
+        clocks:
+          items:
+            - description: Pixel Clock
+            - description: HDMI PLL
+            - description: Bit Clock
+            - description: S/PDIF Clock
+
+        clock-names:
+          items:
+            - const: pixel
+            - const: pll
+            - const: bclk
+            - const: spdif
+
+        ports:
+          required:
+            - port@1
+
+      required:
+        - mediatek,syscon-hdmi
+
 additionalProperties: false

 examples:

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml
new file mode 100644
index 000000000000..3c80bcebe6d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml

@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mediatek HDMI DDC Device Tree Bindings for mt8195

Drop Device Tree Bindings

quoted

+
+maintainers:
+  - CK Hu [off-list ref]
+  - Jitao shi [off-list ref]
+
+description: |
+  The HDMI DDC i2c controller is used to interface with the HDMI DDC pins.

Why is this different than existing ddc bindings?

This ddc is actually part of the MT8195 hdmi IP.

So it is a bit simpler than the mediatek,hdmi-ddc.yaml
As it has only one clock, no reg, no interrupts.

quoted

+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt8195-hdmi-ddc
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: ddc-i2c
+
+required:
+  - compatible
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    hdmiddc0: ddc_i2c {

No underscores in node names. Generic node names.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel