Re: [PATCH v2 02/15] dt-bindings: display: bridge: Add starfive,jh7110-inno-hdmi-controller

[Conor Dooley]

On Fri, Aug 28, 2026 at 03:47:26PM +0200, Michal Wilczynski wrote:
> Add the dt-binding for the StarFive JH7110 Innosilicon HDMI controller
> (DRM bridge).
> 
> This device is the second child of the starfive,jh7110-hdmi-subsystem node.
> It consumes register access clocks (sys, mclk, bclk) from the voutcrg and
> both the pixel clock (pclk) and the PHY from its hdmi_phy sibling.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
> ---
>  .../starfive,jh7110-inno-hdmi-controller.yaml      | 109 +++++++++++++++++++++
>  1 file changed, 109 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/starfive,jh7110-inno-hdmi-controller.yaml b/Documentation/devicetree/bindings/display/bridge/starfive,jh7110-inno-hdmi-controller.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..0e584e7b756e24e2f99a62daab3117bbf4b472b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/starfive,jh7110-inno-hdmi-controller.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/bridge/starfive,jh7110-inno-hdmi-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 Innosilicon HDMI Controller
> +
> +maintainers:
> +  - Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
> +
> +description:
> +  The controller portion of the StarFive JH7110 INNO HDMI IP.
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-inno-hdmi-controller
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: System clock for register access
> +      - description: Module clock
> +      - description: Bus clock
> +      - description: Pixel clock from PHY
> +
> +  clock-names:
> +    items:
> +      - const: sys
> +      - const: mclk
> +      - const: bclk
> +      - const: pclk

"module" "bus" "pixel" would be more informative than spending most of
your characters regurgitating what these are by definition?

> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: hdmi_tx
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: hdmi-phy

With the trend of ?pointless? -names properties, I'm surprised you don't
have interrupt-names? I'd ditch 'em all where there's only a single
clock. The _ rather than - in the reset name triggers my OCD.
pw-bot: changes-requested

> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description:
> +      A graph node with one input port and one output port.
> +
> +required:
> +  - compatible
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - phys
> +  - phy-names
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  # Shown as a child of the HDMI subsystem node; see
> +  # Documentation/devicetree/bindings/mfd/starfive,jh7110-hdmi-subsystem.yaml
> +  # for the full node.
> +  - |
> +    #include <dt-bindings/clock/starfive,jh7110-crg.h>
> +    #include <dt-bindings/reset/starfive,jh7110-crg.h>
> +
> +    controller {

"controller" seems over generic for a node, but I guess if the parent is
"hdmi-subsystem" or something it makes sense in that context.

Cheers,
Conor.


> +        compatible = "starfive,jh7110-inno-hdmi-controller";
> +        clocks = <&voutcrg JH7110_VOUTCLK_HDMI_TX_SYS>,
> +                 <&voutcrg JH7110_VOUTCLK_HDMI_TX_MCLK>,
> +                 <&voutcrg JH7110_VOUTCLK_HDMI_TX_BCLK>,
> +                 <&hdmi_phy>;
> +        clock-names = "sys", "mclk", "bclk", "pclk";
> +        interrupts = <99>;
> +        phys = <&hdmi_phy>;
> +        phy-names = "hdmi-phy";
> +        resets = <&voutcrg JH7110_VOUTRST_HDMI_TX_HDMI>;
> +        reset-names = "hdmi_tx";
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +
> +                endpoint {
> +                    remote-endpoint = <&dpu_out_dpi0>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +
> +                endpoint {
> +                    remote-endpoint = <&hdmi_con_in>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> 
> -- 
> 2.34.1
>

Attachment: signature.asc
Description: PGP signature