Hi, > Am 27.09.2021 um 19:07 schrieb maxime@xxxxxxxxxx: > > Hi, > > On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote: >> From: Sam Ravnborg <sam@xxxxxxxxxxxx> >> >> 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 <sam@xxxxxxxxxxxx> >> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> >> Cc: Rob Herring <robh@xxxxxxxxxx> >> Cc: devicetree@xxxxxxxxxxxxxxx >> --- >> .../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@xxxxxxxxxxxxx> >> + >> +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# > >> +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. > >> + >> + reg: >> + maxItems: 1 >> + description: the address & size of the LCD controller registers > > There's no need for that description, it's obvious enough Indeed. > >> + 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. > >> + interrupts: >> + maxItems: 1 >> + description: Specifies the interrupt provided by parent > > There's no need for that description, it's obvious enough Indeed. > >> + 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 > >> + 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] > >> + - 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. >> + >> + 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 > >> + ddc-i2c-bus: true > > ditto Ok > >> + 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
