[Linaro-mm-sig] Re: [PATCH v2 1/6] dt-bindings: display: Add yamls for JH7110 display system

On 25/10/2023 12:39, Keith Zhao wrote:

StarFive SoCs JH7110 display system:

A nit, subject: drop second/last, redundant "yamls for". The "dt-bindings" prefix is already stating that these are bindings, so format is fixed.

lcd-controller bases verisilicon dc8200 IP, and hdmi bases Innosilicon IP. Add bindings for them.

Please make it a proper sentences, with proper wrapping.

also update MAINTAINERS for dt-bindings

Not a sentence, but also not really needed.

about this patch, I tested the dtbs_check and dt_binding_check with the result pass. Based on the feedback of the previous version, the corresponding arrangement is made

Not relevant, so not really suitable for commit msg.

Signed-off-by: Keith Zhao keith.zhao@starfivetech.com

.../starfive/starfive,display-subsystem.yaml | 41 +++++++ .../starfive/starfive,jh7110-dc8200.yaml | 109 ++++++++++++++++++ .../starfive/starfive,jh7110-inno-hdmi.yaml | 85 ++++++++++++++ MAINTAINERS | 7 ++ 4 files changed, 242 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml

diff --git a/Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml b/Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml new file mode 100644 index 000000000..f45b97b08 --- /dev/null +++ b/Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml @@ -0,0 +1,41 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/starfive/starfive,display-subsystem.ya... +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Starfive DRM master device

What is DRM in hardware? I know Digital Rights Management, but then subsystem seems wrong. If you mean Linux DRM, then Linux is not a hardware, so drop all Linuxisms and describe hardware.

+maintainers:

• • Keith Zhao keith.zhao@starfivetech.com
• • ShengYang Chen shengyang.chen@starfivetech.com

+description:

• The Starfive DRM master device is a virtual device needed to list all

Virtual device? Then not suitable for bindings, sorry.

• display controller or other display interface nodes that comprise the
• graphics subsystem.

+properties:

• compatible:
• const: starfive,display-subsystem
• ports:
• $ref: /schemas/types.yaml#/definitions/phandle-array

No, ports is not phandle-array. ports is object, always.

• description:

•  Should contain a list of phandles pointing to display interface ports
  

•  of display controller devices. Display controller definitions as defined
  

•  in Documentation/devicetree/bindings/display/starfive/
  

•  starfive,jh7110-dc8200.yaml
  

Use standard graph ports, not some own, custom property.

Anyway, entire binding should be dropped. You do not need it even.

+required:

• • compatible
• • ports

+additionalProperties: false

+examples:

• • |
• display-subsystem {

•    compatible = "starfive,display-subsystem";
  

•    ports = <&dc_out>;
  

• };

diff --git a/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml new file mode 100644 index 000000000..87051cddf --- /dev/null +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml @@ -0,0 +1,109 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/starfive/starfive,jh7110-dc8200.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: StarFive display controller

+description:

• The StarFive SoC uses the display controller based on Verisilicon IP
• to transfer the image data from a video memory buffer to an external
• LCD interface.

+maintainers:

• • Keith Zhao keith.zhao@starfivetech.com

+properties:

• compatible:
• const: starfive,jh7110-dc8200
• reg:
• minItems: 1
• items:

•  - description:
  

•      host interface
  

In one line. - description: host interface

•  - description:
  

•      display physical base address and length.
  

Drop redundant parts: base address and length. Everything in reg for MMIO would have it...

• interrupts:
• items:

•  - description: The interrupt will be generated when DC finish one frame
  

• clocks:
• items:

•  - description: Clock for display system noc bus.
  

•  - description: Pixel clock for display channel 0.
  

•  - description: Pixel clock for display channel 1.
  

•  - description: Core clock for display controller.
  

•  - description: Clock for axi bus to access ddr.
  

•  - description: Clock for ahb bus to R/W the phy regs.
  

•  - description: External HDMI pixel clock.
  

•  - description: Parent clock for pixel clock
  

• clock-names:
• items:

•  - const: noc_bus
  

•  - const: channel0
  

•  - const: channel1
  

•  - const: dc_core
  

•  - const: axi_core
  

•  - const: ahb
  

•  - const: hdmi_tx
  

•  - const: dc_parent
  

• resets:
• items:

•  - description: Reset for axi bus.
  

•  - description: Reset for ahb bus.
  

•  - description: Core reset of display controller.
  

• reset-names:
• items:

•  - const: axi
  

•  - const: ahb
  

•  - const: core
  

• port:
• $ref: /schemas/graph.yaml#/properties/port

+required:

• • compatible
• • reg
• • interrupts
• • clocks
• • clock-names
• • resets
• • reset-names
• • port

+additionalProperties: false

+examples:

• • |
• dc8200: lcd-controller@29400000 {

•    compatible = "starfive,jh7110-dc8200";
  

•    reg = <0x29400000 0x100>, <0x29400800 0x2000>;
  

•    interrupts = <95>;
  

•    clocks = <&syscrg 60>,
  

•           <&voutcrg 7>,
  

•           <&voutcrg 8>,
  

•           <&voutcrg 4>,
  

•           <&voutcrg 5>,
  

•           <&voutcrg 6>,
  

•           <&hdmitx0_pixelclk>,
  

•           <&voutcrg 1>;
  

•    clock-names = "noc_bus", "channel0", "channel1",
  

•                  "dc_core", "axi_core", "ahb",
  

•                  "hdmi_tx","dc_parent";
  

•    resets = <&voutcrg 0>, <&voutcrg 1>, <&voutcrg 2>;
  

•    reset-names = "axi", "ahb", "core";
  

•    dc_out: port {
  

•        #address-cells = <1>;
  

•        #size-cells = <0>;
  

•        dc_out_hdmi: endpoint@0 {
  

•            reg = <0>;
  

•            remote-endpoint = <&hdmi_in_dc>;
  

•        };
  

•    };
  

• };

diff --git a/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml new file mode 100644 index 000000000..f6c473a10 --- /dev/null +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml @@ -0,0 +1,85 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/starfive/starfive,jh7110-inno-hdmi.yam... +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Starfive JH7110 HDMI controller

+description:

• The StarFive JH7110 SoC uses the HDMI signal transmiter based on innosilicon IP
• to generate HDMI signal from its input and transmit the signal to the screen.

+maintainers:

• • Keith Zhao keith.zhao@starfivetech.com

+properties:

• compatible:
• const: "starfive,jh7110-inno-hdmi"

It does not look like you tested the bindings, at least after quick look. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint.

Best regards, Krzysztof