Hi Krzysztof: > -----Original Message----- > From: Krzysztof Kozlowski <krzk@xxxxxxxxxx> > Sent: 2024年5月21日 15:30 > To: Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx>; andrzej.hajda@xxxxxxxxx; > neil.armstrong@xxxxxxxxxx; rfoss@xxxxxxxxxx; > Laurent.pinchart@xxxxxxxxxxxxxxxx; jonas@xxxxxxxxx; > jernej.skrabec@xxxxxxxxx; maarten.lankhorst@xxxxxxxxxxxxxxx; > mripard@xxxxxxxxxx; tzimmermann@xxxxxxx; airlied@xxxxxxxxx; > daniel@xxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; > hjc@xxxxxxxxxxxxxx; heiko@xxxxxxxxx; andy.yan@xxxxxxxxxxxxxx; Xingyu Wu > <xingyu.wu@xxxxxxxxxxxxxxxx>; p.zabel@xxxxxxxxxxxxxx; Jack Zhu > <jack.zhu@xxxxxxxxxxxxxxxx>; Shengyang Chen > <shengyang.chen@xxxxxxxxxxxxxxxx> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH v4 01/10] dt-bindings: display: Add YAML schema for JH7110 > display pipeline > > On 21/05/2024 12:58, keith wrote: > > JH7110 SoC display pipeline includes a display controller and hdmi. > > Dc controller IP : Vivante DC8200 Dual Display HDMI IP : INNOSILICON > > HDMI2.0 > > > > As the INNO hdmi ip is also used by rockchip SoC in the driver code, > > the innosilicon,inno-hdmi.yaml schema containing the common properties > > for the INNO DesignWare HDMI TX controller isn't a full device tree > > binding specification, but is meant to be referenced by > > platform-specific bindings for the IP core. > > > > Signed-off-by: keith <keith.zhao@xxxxxxxxxxxxxxxx> > > --- > > .../display/bridge/innosilicon,inno-hdmi.yaml | 49 +++++ > > .../display/rockchip/rockchip,inno-hdmi.yaml | 27 +-- > > .../starfive/starfive,dsi-encoder.yaml | 92 ++++++++++ > > .../starfive/starfive,jh7110-dc8200.yaml | 169 ++++++++++++++++++ > > .../starfive/starfive,jh7110-inno-hdmi.yaml | 75 ++++++++ > > .../soc/starfive/starfive,jh7110-syscon.yaml | 1 + > > MAINTAINERS | 8 + > > 7 files changed, 396 insertions(+), 25 deletions(-) create mode > > 100644 > > Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi > > .yaml create mode 100644 > > Documentation/devicetree/bindings/display/starfive/starfive,dsi-encode > > r.yaml create mode 100644 > > Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8 > > 200.yaml create mode 100644 > > Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inn > > o-hdmi.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hd > > mi.yaml > > b/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hd > > mi.yaml > > new file mode 100644 > > index 000000000000..8540174dcaeb > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/bridge/innosilicon,inn > > +++ o-hdmi.yaml > > @@ -0,0 +1,49 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: > > +http://devicetree.org/schemas/display/bridge/innosilicon,inno-hdmi.ya > > +ml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Common Properties for Innosilicon HDMI TX IP > > Your patch is difficult to review. Split changing existing bindings (and defining > common part) to a separate patch. The background is that because the existing inno hdmi driver on the main line has some interfaces that can be public, its yaml counterpart attempts to make a public reference file > > > + > > +maintainers: > > + - Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx> > > + > > +description: | > > + This document defines device tree properties for the Innosilicon > > +HDMI TX > > Nothing improved here. Don't say obvious that this documents says something. > It cannot do anything else. > > "Innosilicon HDMI TX is a foo bar device present on zap SoC ...." > > > > + controller (INNO HDMI) IP core. It doesn't constitute a full device > > + tree binding specification by itself but is meant to be referenced > > + by device tree bindings for the platform-specific integrations of the INNO > HDMI. > > I don't understand this at all. I don't know what is "full device tree binding > specification". > > > + > > + When referenced from platform device tree bindings the properties > > + defined in this document are defined as follows. The platform > > + device tree bindings are responsible for defining whether each property is > required or optional. > > Nothing improved - drop paragraph. These descriptions are based this file, and because their roles are close, only some key elements have been changed of course, I will make corresponding modifications according to your remarks https://elixir.bootlin.com/linux/v6.9/source/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > > > + > > +properties: > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + ports: > > + $ref: /schemas/graph.yaml#/properties/ports > > + > > + properties: > > + port@0: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: > > + Port node with one endpoint connected to a display controller > node. > > + > > + port@1: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: > > + Port node with one endpoint connected to a hdmi-connector > node. > > + > > + required: > > + - port@0 > > + - port@1 > > ... > > > diff --git > > a/Documentation/devicetree/bindings/display/starfive/starfive,dsi-enco > > der.yaml > > b/Documentation/devicetree/bindings/display/starfive/starfive,dsi-enco > > der.yaml > > new file mode 100644 > > index 000000000000..07aa147a9db1 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/starfive/starfive,dsi- > > +++ encoder.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,dsi-encoder.y > > +aml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: starfive jh7110 SoC Encoder > > + > > +maintainers: > > + - Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx> > > + > > +description: > > + Device-Tree bindings for simple encoder. > > Again you ignored the comments. > > When you receive a comment, apply it everywhere, not in only one part of patch > while keeping wrong code everywhere else. > > > + Simple encoder driver only has basic functionality. > > Not related to bindings, drop. This is about hardware, not your driver. > > > + the hardware of dc8200 has 2 output interface, and uses syscon to > > + select which one to be used. > > Nothing improved. > > Read my previous comments: > > 1. Please make it a proper sentences, with proper wrapping. > > > + > > +properties: > > + compatible: > > + const: starfive,dsi-encoder > > + > > + starfive,syscon: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + items: > > + - items: > > + - description: phandle to syscon that select crtc output. > > + - description: Offset of crtc selection > > + - description: Shift of crtc selection > > + description: > > + A phandle to syscon with two arguments that configure select > output. > > + The argument one is the offset of crtc selection, the > > + argument two is the shift of crtc selection. > > Don't repeat constraints in free form text. Say something useful instead, like > what is its purpose. > > No reg? Then why this is not just part of syscon? That's not a separate device, > no. > > You received a feedback already about this: do not create fake bindings just to > instantiate your drivers. > Yes , "do not create fake bindings just to instantiate your drivers." , I got this message , In lastest patchset, there was a display-subsystem dt-binding with no reg , I deleted it. reg != syscon reg , got it . I will restructure the code structure in the next version to meet this requirement, so that this dts node can not be added, same with yaml file. > > + > > + ports: > > + $ref: /schemas/graph.yaml#/properties/ports > > + > > + properties: > > + port@0: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: > > + The first port should be the input coming from the associated dc > channel. > > + > > + port@1: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: > > + The second port should be the output coming from the > associated bridge. > > + > > + required: > > + - port@0 > > + - port@1 > > + > > +required: > > + - compatible > > + - ports > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + dssctrl: dssctrl@295b0000 { > > Drop node, not erlated. > > > + compatible = "starfive,jh7110-vout-syscon", "syscon"; > > + reg = <0x295b0000 0x90>; > > + }; > > + > > + dsi_encoder: dsi_encoder { > > Totally messed indentation. Use 4 spaces for example indentation. > > Also, drop label. > > Also, underscores are not allowed in node names. > > Also, Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree- > basics.html#generic-names-recommendation > > This is terrible code. > > > + compatible = "starfive,dsi-encoder"; > > + starfive,syscon = <&dssctrl 0x8 0x12>; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + /* input */ > > + port@0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0>; > > + dsi_input0:endpoint@0 { > > + reg = <0>; > > + remote-endpoint = <&dc_out_dpi1>; > > + }; > > + }; > > + /* output */ > > + port@1 { > > + reg = <1>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + dsi_out:endpoint { > > + remote-endpoint = <&mipi_in>; > > + }; > > + }; > > + }; > > + }; > > diff --git > > a/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-d > > c8200.yaml > > b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-d > > c8200.yaml > > new file mode 100644 > > index 000000000000..a28dfdd471b6 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh71 > > +++ 10-dc8200.yaml > > @@ -0,0 +1,169 @@ > > +# 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 JH7110 SoC display controller > > + > > +description: > > + The STARFIVE JH7110 SoC uses the display controller based on > > +Verisilicon IP(DC8200) > > + to transfer the image data from a video memory buffer to an external LCD > interface. > > + > > + pipe0 binds HDMI for primary display, > > + pipe1 binds DSI for external display. > > + > > + +------------------------------+ > > + | | > > + | | > > + +----+ | +-------------------+ | +-------+ +------+ +------+ > > + | +----->+ dc controller 0 +--->----->+HDMICtl| ->+ PHY > +-->+PANEL0+ > > + |AXI | | +-------------------+ | +-------+ +------+ +------+ > > + | | | | > > + | | | | > > + | | | | > > + | | | | > > + |APB | | +-------------------+ +---------+ +------+ +-------+ > > + | +----->+ dc controller 1 +--->---->+ dsiTx +--->+DPHY +->+ > PANEL1+ > > + | | | +-------------------+ +---------+ +------+ +-------+ > > + +----+ | | > > + +------------------------------+ > > + > > +maintainers: > > + - Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx> > > + > > +properties: > > + compatible: > > + const: starfive,jh7110-dc8200 > > + > > + reg: > > + items: > > + - description: host interface address and length > > + - description: display physical base address and length > > + > > + reg-names: > > + items: > > + - const: host > > + - const: base > > + > > + clocks: > > + items: > > + - description: Clock for display system noc bus. > > + - 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: Pixel clock for display channel 0. > > + - description: Pixel clock for display channel 1. > > + - description: Pixel clock from hdmi. > > + - description: Pixel clock for soc . > > + > > + clock-names: > > + items: > > + - const: noc_bus > > + - const: dc_core > > + - const: axi_core > > + - const: ahb > > + - const: channel0 > > + - const: channel1 > > + - 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 > > + > > + interrupts: > > + items: > > + - description: The interrupt will be generated when DC finish > > + one frame > > + > > + ports: > > + $ref: /schemas/graph.yaml#/properties/ports > > + > > + properties: > > + port@0: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: > > + channel 0 output > > + > > + port@1: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: > > + channel 1 output > > + > > + required: > > + - port@0 > > + - port@1 > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + - ports > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/starfive,jh7110-crg.h> > > + #include <dt-bindings/reset/starfive,jh7110-crg.h> > > + dc8200: lcd-controller@29400000 { > > + compatible = "starfive,jh7110-dc8200"; > > + reg = <0x29400000 0x100>, > > + <0x29400800 0x2000>; > > + reg-names = "host", "base"; > > + > > + interrupts = <95>; > > + clocks = <&syscrg JH7110_SYSCLK_NOC_BUS_DISP_AXI>, > > + <&voutcrg JH7110_VOUTCLK_DC8200_CORE>, > > Align the lines. In other places as well. > > > + <&voutcrg JH7110_VOUTCLK_DC8200_AXI>, > > + <&voutcrg JH7110_VOUTCLK_DC8200_AHB>, > > + <&voutcrg JH7110_VOUTCLK_DC8200_PIX0>, > > + <&voutcrg JH7110_VOUTCLK_DC8200_PIX1>, > > + <&hdmitx0_pixelclk>, > > + <&voutcrg JH7110_VOUTCLK_DC8200_PIX>; > > + clock-names = "noc_bus", "dc_core", "axi_core", "ahb", > > + "channel0", "channel1", "hdmi_tx", "dc_parent"; > > + resets = <&voutcrg JH7110_VOUTRST_DC8200_AXI>, > > + <&voutcrg JH7110_VOUTRST_DC8200_AHB>, > > + <&voutcrg JH7110_VOUTRST_DC8200_CORE>; > > + reset-names = "axi","ahb", "core"; > > + > > + crtc_out: ports { > > Totally messed indentation. > > > > diff --git > > a/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-i > > nno-hdmi.yaml > > b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-i > > nno-hdmi.yaml > > new file mode 100644 > > index 000000000000..bfd7dc41fc14 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh71 > > +++ 10-inno-hdmi.yaml > > @@ -0,0 +1,75 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: > > +http://devicetree.org/schemas/display/starfive/starfive,jh7110-inno-h > > +dmi.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Starfive JH7110 Innosilicon HDMI controller > > + > > +maintainers: > > + - Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx> > > + > > +allOf: > > + - $ref: ../bridge/innosilicon,inno-hdmi.yaml# > > + > > +properties: > > + compatible: > > + const: "starfive,jh7110-inno-hdmi" > > You did not test it. Drop quotes. > > Untested bindings, so I will skip review for now. In fact, I have used the check instruction, the result is pass with no warning, is there a difference in the command? make -j8 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/starfive/ ...... DTEX Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.example.dts DTEX Documentation/devicetree/bindings/display/starfive/starfive,dsi-encoder.example.dts DTEX Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.example.dts DTC_CHK Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.example.dtb DTC_CHK Documentation/devicetree/bindings/display/starfive/starfive,dsi-encoder.example.dtb DTC_CHK Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.example.dtb Thanks for your remarks , I hope it does not affect your happy life > > > > > Best regards, > Krzysztof
