Re: [PATCH v5 3/4] dt-bindings: display: rockchip: Add schema for RK3588 HDMI TX Controller

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

On 8/31/24 9:13 AM, Krzysztof Kozlowski wrote:
> On Sat, Aug 31, 2024 at 12:55:31AM +0300, Cristian Ciocaltea wrote:
>> Rockchip RK3588 SoC integrates the Synopsys DesignWare HDMI 2.1
>> Quad-Pixel (QP) TX controller IP.
>>
>> Since this is a new IP block, quite different from those used in the
>> previous generations of Rockchip SoCs, add a dedicated binding file.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  .../rockchip/rockchip,rk3588-dw-hdmi-qp.yaml       | 166 +++++++++++++++++++++
>>  1 file changed, 166 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml
>> new file mode 100644
>> index 000000000000..d2919ff6aa23
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml
>> @@ -0,0 +1,166 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Rockchip DW HDMI QP TX Encoder
>> +
>> +maintainers:
>> +  - Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> +
>> +description:
>> +  Rockchip RK3588 SoC integrates the Synopsys DesignWare HDMI QP TX controller
>> +  IP and a HDMI/eDP TX Combo PHY based on a Samsung IP block.
>> +
>> +allOf:
>> +  - $ref: /schemas/display/bridge/synopsys,dw-hdmi-qp.yaml#
>> +  - $ref: /schemas/sound/dai-common.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - rockchip,rk3588-dw-hdmi-qp
>> +
>> +  clocks:
>> +    items:
>> +      - {}
>> +      - {}
>> +      - {}
>> +      - {}
>> +      - description: TMDS/FRL link clock
>> +      - description: Video datapath clock
> 
> Please define all clocks.

The other clocks are defined in the common binding, should we reiterate
them?

>> +
>> +  clock-names:
>> +    items:
>> +      - {}
>> +      - {}
>> +      - {}
>> +      - {}
>> +      - enum: [hdp, hclk_vo1]
>> +      - const: hclk_vo1
>> +
>> +  interrupts:
>> +    items:
>> +      - {}
>> +      - {}
>> +      - {}
>> +      - {}
>> +      - description: HPD interrupt
>> +
>> +  interrupt-names:
>> +    items:
>> +      - {}
>> +      - {}
>> +      - {}
>> +      - {}
>> +      - const: hpd
>> +
>> +  phys:
>> +    maxItems: 1
>> +    description: The HDMI/eDP PHY.
>> +
>> +  phy-names:
>> +    const: hdmi
> 
> Drop phy-names, not really useful if it copies the name of the block.

Sure, will drop it.

>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    minItems: 2
>> +    maxItems: 2
>> +
>> +  reset-names:
>> +    items:
>> +      - const: ref
>> +      - const: hdp
>> +
>> +  "#sound-dai-cells":
>> +    const: 0
>> +
>> +  rockchip,grf:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Some HDMI QP related data is accessed through SYS GRF regs.
>> +
>> +  rockchip,vo-grf:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Additional HDMI QP related data is accessed through VO GRF regs.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - interrupts
>> +  - interrupt-names
>> +  - phys
>> +  - phy-names
>> +  - ports
>> +  - resets
>> +  - reset-names
>> +  - rockchip,grf
>> +  - rockchip,vo-grf
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/power/rk3588-power.h>
>> +    #include <dt-bindings/reset/rockchip,rk3588-cru.h>
>> +
>> +    soc {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +
>> +      hdmi@fde80000 {
>> +        compatible = "rockchip,rk3588-dw-hdmi-qp";
>> +        reg = <0x0 0xfde80000 0x0 0x20000>;
>> +        clocks = <&cru PCLK_HDMITX0>,
>> +                 <&cru CLK_HDMITX0_EARC>,
>> +                 <&cru CLK_HDMITX0_REF>,
>> +                 <&cru MCLK_I2S5_8CH_TX>,
>> +                 <&cru CLK_HDMIHDP0>,
>> +                 <&cru HCLK_VO1>;
>> +        clock-names = "pclk", "earc", "ref", "aud", "hdp", "hclk_vo1";
>> +        interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                     <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                     <GIC_SPI 171 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                     <GIC_SPI 172 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                     <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH 0>;
>> +        interrupt-names = "avp", "cec", "earc", "main", "hpd";
>> +        phys = <&hdptxphy_hdmi0>;
>> +        phy-names = "hdmi";
>> +        power-domains = <&power RK3588_PD_VO1>;
>> +        resets = <&cru SRST_HDMITX0_REF>, <&cru SRST_HDMIHDP0>;
>> +        reset-names = "ref", "hdp";
>> +        rockchip,grf = <&sys_grf>;
>> +        rockchip,vo-grf = <&vo1_grf>;
>> +        #sound-dai-cells = <0>;
>> +
>> +        ports {
>> +          #address-cells = <1>;
>> +          #size-cells = <0>;
>> +
>> +          port@0 {
>> +            reg = <0>;
>> +
>> +            hdmi0_in_vp0: endpoint {
>> +                remote-endpoint = <&vp0_out_hdmi0>;
> 
> Messed indentation.

Ups, somehow I missed this..

Thanks for reviewing,
Cristian