[Linaro-mm-sig] Re: [PATCH v1 v1 2/7] dt-bindings: display: Add yamls for JH7110 display system

Hi,

On Tue, Aug 01, 2023 at 06:10:25PM +0800, Keith Zhao wrote:

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..bebe2050c --- /dev/null +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml @@ -0,0 +1,107 @@ +# 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:
• maxItems: 3

What these registers are used for must be documented.

• 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: Clock for axi interface of display controller.
  

•  - description: Core clock for display controller.
  

•  - description: Clock for ahb interface of display controller.
  

•  - description: External HDMI pixel clock.
  

•  - description: Parent clock for pixel clock
  

• clock-names:
• items:

•  - const: vout_noc_disp
  

•  - const: vout_pix0
  

•  - const: vout_pix1
  

•  - const: vout_axi
  

•  - const: vout_core
  

•  - const: vout_vout_ahb
  

•  - const: hdmitx0_pixel
  

•  - const: vout_dc8200
  

The clock-names should reflect what they are used for on the device, not what their name is in the system. So it should rather be something like "noc-bus", "channel0", "channel1", etc.

vout, or the soc model, shouldn't appear there.

• resets:
• items:

•  - description: Reset for axi interface of display controller.
  

•  - description: Reset for ahb interface of display controller.
  

•  - description: Core reset of display controller.
  

• reset-names:
• items:

•  - const: vout_axi
  

•  - const: vout_ahb
  

•  - const: vout_core
  

Ditto.

Also, I'm a bit confused, how can a device be attached to both an AXI and AHB bus? That, plus the multiple registers spaces, make me think that this is multiple devices glued together in a single node, which isn't ok.

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

•  A port node with endpoint definitions as defined in
  

•  Documentation/devicetree/bindings/media/video-interfaces.txt.
  

+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>, <0x295b0000 0x90>;
  

•    interrupts = <95>;
  

•    clocks = <&syscrg 60>,
  

•           <&voutcrg 7>,
  

•           <&voutcrg 8>,
  

•           <&voutcrg 4>,
  

•           <&voutcrg 5>,
  

•           <&voutcrg 6>,
  

•           <&hdmitx0_pixelclk>,
  

•           <&voutcrg 1>;
  

•    clock-names = "vout_noc_disp", "vout_pix0", "vout_pix1",
  

•                  "vout_axi", "vout_core", "vout_vout_ahb",
  

•                  "hdmitx0_pixel","vout_dc8200";
  

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

•    reset-names = "vout_axi", "vout_ahb", "vout_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..f6927acf6 --- /dev/null +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml @@ -0,0 +1,92 @@ +# 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"
• reg:
• minItems: 1
• interrupts:
• items:

•  - description: The HDMI hot plug detection interrupt.
  

• clocks:
• items:

•  - description: System clock of HDMI module.
  

•  - description: Mclk clock of HDMI audio.
  

•  - description: Bclk clock of HDMI audio.
  

•  - description: Pixel clock generated by HDMI module.
  

• clock-names:
• items:

•  - const: sysclk
  

•  - const: mclk
  

•  - const: bclk
  

•  - const: pclk
  

• resets:
• items:

•  - description: Reset for HDMI module.
  

• reset-names:
• items:

•  - const: hdmi_tx
  

If there's only one you don't need reset-names

Maxime