Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema

Hi,

Am 27.09.2021 um 19:07 schrieb maxime@cerno.tech:

Hi,

On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote:

quoted

From: Sam Ravnborg <redacted>

Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
Based on .txt binding from Zubair Lutfullah Kakakhel

Signed-off-by: Sam Ravnborg <redacted>
Signed-off-by: H. Nikolaus Schaller <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
---
.../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++
1 file changed, 85 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
new file mode 100644
index 000000000000..5e60cdac4f63
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bindings for Ingenic JZ4780 HDMI Transmitter
+
+maintainers:
+  - H. Nikolaus Schaller <hns@goldelico.com>
+
+description: |
+  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
+  TX controller IP with accompanying PHY IP.
+
+allOf:
+  - $ref: panel/panel-common.yaml#

Is it a panel though?

Good question. 

Appears to have to be changed to

  - $ref: bridge/synopsys,dw-hdmi.yaml#

quoted

+properties:
+  compatible:
+    items:
+      - const: ingenic,jz4780-dw-hdmi

This can just be a const, there's no need for the items

Maybe starting with an enum is better if more compatible strings are to be added.

quoted

+
+  reg:
+    maxItems: 1
+    description: the address & size of the LCD controller registers

There's no need for that description, it's obvious enough

Indeed.

quoted

+  reg-io-width:
+    const: 4

If it's fixed, why do you need it in the first place?

There is a fixed default of 1 if not specified.

quoted

+  interrupts:
+    maxItems: 1
+    description: Specifies the interrupt provided by parent

There's no need for that description, it's obvious enough

Indeed.

quoted

+  clocks:
+    maxItems: 2
+    description: Clock specifiers for isrf and iahb clocks

This can be defined as

clocks:
 items:
   - description: isrf
   - description: iahb

A better description about what these clocks are would be nice as well

Generally I see that this all is nowadays not independent of

Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml

where there is already a description.

On the other hand every SoC specialization runs its own copy. e.g.

Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml
Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam

quoted

+  clock-names:
+    items:
+      - const: isfr

Is it isfr or isrf?

isfr. Seems to be a typo in the description. See bridge/synopsys,dw-hdmi.yaml#

One question to the yaml specialists:

since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we have to repeat?
Or can we reduce to just the changes?

[I am still not familiar enough with the yaml stuff to understand if it has sort
of inheritance like device tree include files, so that you just have to change
relevant properties]

quoted

+      - const: iahb

would it make sense to add additionalItems: false here?

In the jz4780 case there are just two clocks while other specializations
use more and synopsys,dw-hdmi.yaml# defines additionalItems: true.

quoted

+
+  hdmi-regulator: true
+    description: Optional regulator to provide +5V at the connector

regulators need to be suffixed by -supply

My omission...

And, it should be "hdmi-5v-supply" to match driver and device tree.

You also can just provide the description, you don't need the true there

quoted

+  ddc-i2c-bus: true

ditto

Ok

quoted

+    description: An I2C interface if the internal DDC I2C driver is not to be used
+  ports: true

If there's a single port, you don't need ports

There can be two ports - one for input from LCDC and one
for output (HDMI connector). But explicitly defining an output
port is optional to some extent (depending on driver structure).

You should also include /schemas/graph.yaml#/$defs/port-base

Ok.

BR and thanks,
Nikolaus