[PATCH v4 2/4] dt-bindings: media: Document bindings for HDMI RX Controller

[Johan Jonker]

On 7/22/24 15:53, Shreeya Patel wrote:
> On Saturday, July 20, 2024 16:14 IST, Johan Jonker <jbx6244 at yandex.com> wrote:
> 
> Hi Johan,
> 
>>
>>
>> On 7/19/24 14:40, Shreeya Patel wrote:
>>> Document bindings for the Synopsys DesignWare HDMI RX Controller.
>>>

>>> Reviewed-by: Rob Herring <robh at kernel.org>
>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko at collabora.com>

Remove to trigger a new review.

>>> Signed-off-by: Shreeya Patel <shreeya.patel at collabora.com>
>>> ---
>>>
>>> Changes in v4 :-
>>>   - No change
>>>
>>> Changes in v3 :-
>>>   - Rename hdmirx_cma to hdmi_receiver_cma
>>>   - Add a Reviewed-by tag
>>>
>>> Changes in v2 :-
>>>   - Add a description for the hardware
>>>   - Rename resets, vo1 grf and HPD properties
>>>   - Add a proper description for grf and vo1-grf phandles
>>>   - Rename the HDMI Input node name to hdmi-receiver
>>>   - Improve the subject line
>>>   - Include gpio header file in example to fix dt_binding_check failure
>>>
>>>  .../bindings/media/snps,dw-hdmi-rx.yaml       | 132 ++++++++++++++++++
>>>  1 file changed, 132 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
>>> new file mode 100644
>>> index 000000000000..96ae1e2d2816
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
>>> @@ -0,0 +1,132 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +# Device Tree bindings for Synopsys DesignWare HDMI RX Controller
>>> +
>>> +---
>>> +$id: http://devicetree.org/schemas/media/snps,dw-hdmi-rx.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Synopsys DesignWare HDMI RX Controller
>>> +
>>> +maintainers:
>>> +  - Shreeya Patel <shreeya.patel at collabora.com>
>>> +
>>> +description:
>>> +  Synopsys DesignWare HDMI Input Controller preset on RK3588 SoCs
>>> +  allowing devices to receive and decode high-resolution video streams
>>> +  from external sources like media players, cameras, laptops, etc.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - const: rockchip,rk3588-hdmirx-ctrler
>>

>>> +      - const: snps,dw-hdmi-rx

remove

>>
>> 1: Compatible strings must be SoC orientated.
>> 2: In Linux there's no priority in which string will probed first. 
>> What's the point of having a fallback string when there's no common code, but instead only the first string is used?
>>
>> +static const struct of_device_id hdmirx_id[] = {
>> +	{ .compatible = "rockchip,rk3588-hdmirx-ctrler" },
>> +	{ },
>> +};
>>
> 

> We believe the HDMIRX driver can be used for the Synopsys IP on other SoCs
> in the future, which is why we have added snps,dw-hdmi-rx as the fallback compatible.
> Currently, we have tested the driver only on the RK3588 Rock5B, so we are using the
> rockchip,rk3588-hdmirx-ctrler compatible in the driver instead of the fallback one.

The rule that compatible strings (for internal SoC components) must be SoC orientated also applies to the fallback string. "snps,xxxx" does not refer to an independent SoC.
Don't invent strings for devices that we don't know yet if it might or might not be compatible in the future.

Johan

> 
> 
> Thanks,
> Shreeya Patel
> 
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 3
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: cec
>>> +      - const: hdmi
>>> +      - const: dma
>>> +
>>> +  clocks:
>>> +    maxItems: 7
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: aclk
>>> +      - const: audio
>>> +      - const: cr_para
>>> +      - const: pclk
>>> +      - const: ref
>>> +      - const: hclk_s_hdmirx
>>> +      - const: hclk_vo1
>>> +
>>> +  power-domains:
>>> +    maxItems: 1
>>> +
>>> +  resets:
>>> +    maxItems: 4
>>> +
>>> +  reset-names:
>>> +    items:
>>> +      - const: axi
>>> +      - const: apb
>>> +      - const: ref
>>> +      - const: biu
>>> +
>>> +  memory-region:
>>> +    maxItems: 1
>>> +
>>> +  hpd-gpios:
>>> +    description: GPIO specifier for HPD.
>>> +    maxItems: 1
>>> +
>>> +  rockchip,grf:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      The phandle of the syscon node for the general register file
>>> +      containing HDMIRX PHY status bits.
>>> +
>>> +  rockchip,vo1-grf:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      The phandle of the syscon node for the Video Output GRF register
>>> +      to enable EDID transfer through SDAIN and SCLIN.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - interrupt-names
>>> +  - clocks
>>> +  - clock-names
>>> +  - power-domains
>>> +  - resets
>>> +  - pinctrl-0
>>> +  - hpd-gpios
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
>>> +    #include <dt-bindings/gpio/gpio.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>
>>> +    hdmi_receiver: hdmi-receiver at fdee0000 {

>>> +      compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";

      compatible = "rockchip,rk3588-hdmirx-ctrler";

>>> +      reg = <0xfdee0000 0x6000>;
>>> +      interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH 0>,
>>> +                   <GIC_SPI 436 IRQ_TYPE_LEVEL_HIGH 0>,
>>> +                   <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH 0>;
>>> +      interrupt-names = "cec", "hdmi", "dma";
>>> +      clocks = <&cru ACLK_HDMIRX>,
>>> +               <&cru CLK_HDMIRX_AUD>,
>>> +               <&cru CLK_CR_PARA>,
>>> +               <&cru PCLK_HDMIRX>,
>>> +               <&cru CLK_HDMIRX_REF>,
>>> +               <&cru PCLK_S_HDMIRX>,
>>> +               <&cru HCLK_VO1>;
>>> +      clock-names = "aclk",
>>> +                    "audio",
>>> +                    "cr_para",
>>> +                    "pclk",
>>> +                    "ref",
>>> +                    "hclk_s_hdmirx",
>>> +                    "hclk_vo1";
>>> +      power-domains = <&power RK3588_PD_VO1>;
>>> +      resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>,
>>> +               <&cru SRST_HDMIRX_REF>, <&cru SRST_A_HDMIRX_BIU>;
>>> +      reset-names = "axi", "apb", "ref", "biu";
>>> +      memory-region = <&hdmi_receiver_cma>;
>>> +      pinctrl-0 = <&hdmim1_rx_cec &hdmim1_rx_hpdin &hdmim1_rx_scl &hdmim1_rx_sda &hdmirx_5v_detection>;
>>> +      pinctrl-names = "default";
>>> +      hpd-gpios = <&gpio1 22 GPIO_ACTIVE_LOW>;
>>> +    };
>