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

  Document bindings for the Synopsys DesignWare HDMI RX Controller.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Shreeya Patel <shreeya.patel@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
  

On Fri, 19 Jul 2024 18:10:30 +0530, Shreeya Patel wrote:
> Document bindings for the Synopsys DesignWare HDMI RX Controller.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@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
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.example.dts:53.38-39 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240719124032.26852-3-shreeya.patel@collabora.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
  

On 7/19/24 14:40, Shreeya Patel wrote:
> Document bindings for the Synopsys DesignWare HDMI RX Controller.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@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@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

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" },
+	{ },
+};

> +
> +  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@fdee0000 {
> +      compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
> +      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>;
> +    };
  

On Saturday, July 20, 2024 16:14 IST, Johan Jonker <jbx6244@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@kernel.org>
> > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Signed-off-by: Shreeya Patel <shreeya.patel@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@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
> 
> 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.


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@fdee0000 {
> > +      compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
> > +      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>;
> > +    };
  

On 7/22/24 15:53, Shreeya Patel wrote:
> On Saturday, July 20, 2024 16:14 IST, Johan Jonker <jbx6244@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@kernel.org>
>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Remove to trigger a new review.

>>> Signed-off-by: Shreeya Patel <shreeya.patel@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@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@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>;
>>> +    };
>
  

Hi,

On Tue, Jul 23, 2024 at 01:16:00PM GMT, Johan Jonker wrote:
> On 7/22/24 15:53, Shreeya Patel wrote:
> > On Saturday, July 20, 2024 16:14 IST, Johan Jonker <jbx6244@yandex.com> wrote:
> >> On 7/19/24 14:40, Shreeya Patel wrote:
> >>> Document bindings for the Synopsys DesignWare HDMI RX Controller.
> >>>
> 
> >>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> 
> Remove to trigger a new review.

Rob and Dmitry both already reviewed the version with the fallback
compatible. I don't think the rename of hdmirx_cma to hdmi_receiver_cma
warrant a new review. Also FWIW:

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

> >>> Signed-off-by: Shreeya Patel <shreeya.patel@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@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. 

Where did you learn that? Having non-SoC specific generic fallback
compatibles is pretty much standard throughout the kernel. See for
example these RK3588 DesignWare compatibles:

Synopsys Serial Controller:
    Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
    compatible = "rockchip,rk3588-uart", "snps,dw-apb-uart";

Synopsys USB3 Controller:
    Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
    compatible = "rockchip,rk3588-dwc3", "snps,dwc3";

Synopsys Ethernet Controller:
    Documentation/devicetree/bindings/net/snps,dwmac.yaml
    compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";

Synsopsys SATA Controller:
    Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml
    compatible = "rockchip,rk3588-dwc-ahci", "snps,dwc-ahci"

It's also not specific to Synopsys (but RK3588 has a lot of Synopsys
design incl. the HDMI-RX IP currently worked on by Shreeya). Here
are some other examples:

ARM Mali GPU:
    Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
    compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";

Generic EHCI:
    Documentation/devicetree/bindings/usb/generic-ehci.yaml
    compatible = "rockchip,rk3588-ehci", "generic-ehci";

As you can see almost everything in RK3588 has a non SoC specific
fallback :) It's also not a Rockchip/RK3588 specific thing, but
I think you should be able to find enough references yourself by
looking into the kernel's DTS files.

> Don't invent strings for devices that we don't know yet if it
> might or might not be compatible in the future.

Right now it's a sensible assumption, that an operating system driver
for this hardware (i.e. not necessarily the one submitted by Shreeya
right now) can handle the Synopsys HDMI receiver hardware from different
SoCs just like it is the case for other Synopsys IP.

Whatever is being done now is set in stone, since DT is considered
ABI. So without the fallback compatible being available in DT from
the beginning we need to carry the RK3588 specific compatible in the
kernel driver forever. OTOH if we add the generic one now, the kernel
can switch to use the generic one at any point in time and ignore the
RK3588 specific one.

Greetings,

-- Sebastian

> 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@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>;
> >>> +    };
> > 
> _______________________________________________
> Kernel mailing list -- kernel@mailman.collabora.com
> To unsubscribe send an email to kernel-leave@mailman.collabora.com
> This list is managed by https://mailman.collabora.com
  

On 7/23/24 19:28, Sebastian Reichel wrote:
> Hi,
>
> On Tue, Jul 23, 2024 at 01:16:00PM GMT, Johan Jonker wrote:
>> On 7/22/24 15:53, Shreeya Patel wrote:
>>> On Saturday, July 20, 2024 16:14 IST, Johan Jonker <jbx6244@yandex.com> wrote:
>>>> On 7/19/24 14:40, Shreeya Patel wrote:
>>>>> Document bindings for the Synopsys DesignWare HDMI RX Controller.
>>>>>
>>
>>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>
>> Remove to trigger a new review.
>
> Rob and Dmitry both already reviewed the version with the fallback
> compatible. I don't think the rename of hdmirx_cma to hdmi_receiver_cma
> warrant a new review. Also FWIW:
>

> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Please have a look at the comments below before you tag.

>
>>>>> Signed-off-by: Shreeya Patel <shreeya.patel@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@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
>>
>>>>

Relevant compatible methods in use for Rockchip drivers:

===================================================================================================

Compatible method #1:
Probe is triggered by a SoC orientated string.

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

If for example a new SoC rk3599 is released that has the same device properties
then the old string can be used as fallback string.

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

The driver structure:
{ .compatible = "rockchip,rk3588-hdmirx-ctrler" },

===================================================================================================
Compatible method #2:
Probe is triggered by a IP orientated fallback string.

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

If for example a new SoC rk3599 is released that has the same device properties
then add the same fallback string.

compatible = ""rockchip,rk3599-hdmirx-ctrler" , "snps,dw-hdmi-rx";

The driver structure:
{ .compatible = "snps,dw-hdmi-rx" },

If for example a new SoC rk3599 is released that has NOT the same device properties
then use method #1.

The driver structure:
{ .compatible = "rockchip,rk3599-hdmirx-ctrler" .data = &rk3599_ops },
{ .compatible = "snps,dw-hdmi-rx" },

===================================================================================================

Compatible method #3:
Probe is triggered by a vendor orientated fallback string.

Special case only useful if the driver is written long after all SoCs are released.
The standalone IP has a version register and the driver can handle all the feature difference
inside the IP depending on the version register.

compatible = "rockchip,sfc";

The driver structure:
{ .compatible = "rockchip,sfc"},

===================================================================================================

The rules:

1: Compatible strings must be SoC orientated.
2: In Linux there's no priority in which string will probed first.
3: There is a commitment that old DT's should still work with newer kernels.

>>>> 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" },
>>>> +	{ },
>>>> +};
>>>>

The consequence of the third rule is that drivers must continue to support this string once added and
can not be removed as suggested below.

If for example the fallback is added later it will trigger 2 probes and it breaks rule #2.
Only one of string is allowed to trigger a probe in the driver.

This is wrong:
compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";

{ .compatible = "rockchip,rk3588-hdmirx-ctrler" },
{ .compatible = "snps,dw-hdmi-rx" },

Ones a compatible method is chosen the driver must stick to it.

===================================================================================================

>>>
>>
>>> 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.

This refers to compatible method #1.

>
> Where did you learn that? Having non-SoC specific generic fallback
> compatibles is pretty much standard throughout the kernel. See for
> example these RK3588 DesignWare compatibles:
>
> Synopsys Serial Controller:
>     Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
>     compatible = "rockchip,rk3588-uart", "snps,dw-apb-uart";

Compatible method #2:
	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },

>
> Synopsys USB3 Controller:
>     Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
>     compatible = "rockchip,rk3588-dwc3", "snps,dwc3";

Compatible method #2:
	{
		.compatible = "snps,dwc3"
	},

>
> Synopsys Ethernet Controller:
>     Documentation/devicetree/bindings/net/snps,dwmac.yaml
>     compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";

Compatible method #1:
	{ .compatible = "rockchip,rk3588-gmac", .data = &rk3588_ops },

	    of_device_is_compatible(np, "snps,dwmac-4.20a") ||

>
> Synsopsys SATA Controller:
>     Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml
>     compatible = "rockchip,rk3588-dwc-ahci", "snps,dwc-ahci"

Compatible method #2:
	{ .compatible = "snps,dwc-ahci", &ahci_dwc_plat },

>
> It's also not specific to Synopsys (but RK3588 has a lot of Synopsys
> design incl. the HDMI-RX IP currently worked on by Shreeya). Here
> are some other examples:
>
> ARM Mali GPU:
>     Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
>     compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";

Should be compatible method #2:
	{ .compatible = "rockchip,rk3588-mali" },
	{ .compatible = "arm,mali-valhall-csf" },

This is wrong!
Each strings will trigger a probe.
The string "rockchip,rk3588-mali" should be removed.

Review was done by Collabora people and without including the Rockchip mail list.
https://patchwork.freedesktop.org/patch/msgid/20240229162230.2634044-12-boris.brezillon@collabora.com

Could someone look at this and test.

>
> Generic EHCI:
>     Documentation/devicetree/bindings/usb/generic-ehci.yaml
>     compatible = "rockchip,rk3588-ehci", "generic-ehci";

compatible method #2:
	{ .compatible = "generic-ehci", },

>
> As you can see almost everything in RK3588 has a non SoC specific
> fallback :) It's also not a Rockchip/RK3588 specific thing, but
> I think you should be able to find enough references yourself by
> looking into the kernel's DTS files.

You are mixing up 2 compatible methods.
The driver has compatible method #1 and the DT has method #2.

>
>> Don't invent strings for devices that we don't know yet if it
>> might or might not be compatible in the future.
>
> Right now it's a sensible assumption, that an operating system driver
> for this hardware (i.e. not necessarily the one submitted by Shreeya
> right now) can handle the Synopsys HDMI receiver hardware from different
> SoCs just like it is the case for other Synopsys IP.
>
> Whatever is being done now is set in stone, since DT is considered
> ABI. So without the fallback compatible being available in DT from
> the beginning we need to carry the RK3588 specific compatible in the

> kernel driver forever. OTOH if we add the generic one now, the kernel
> can switch to use the generic one at any point in time and ignore the
> RK3588 specific one.

Ignoring breaks rule #3 as explained above.

For you the task to select a compatible method:

If the IP device registers are guaranteed remain the same then choose compatible method #2 and fix the driver.
If in doubt choose compatible method #1 and fix the binding.

Johan

>
> Greetings,
>
> -- Sebastian
>
>> 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@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>;
>>>>> +    };
>>>
>> _______________________________________________
>> Kernel mailing list -- kernel@mailman.collabora.com
>> To unsubscribe send an email to kernel-leave@mailman.collabora.com
>> This list is managed by https://mailman.collabora.com
  

On 24/07/2024 15:20, Johan Jonker wrote:
> 
>>
>> Where did you learn that? Having non-SoC specific generic fallback
>> compatibles is pretty much standard throughout the kernel. See for
>> example these RK3588 DesignWare compatibles:
>>
>> Synopsys Serial Controller:
>>     Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
>>     compatible = "rockchip,rk3588-uart", "snps,dw-apb-uart";
> 
> Compatible method #2:
> 	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
> 
>>
>> Synopsys USB3 Controller:
>>     Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
>>     compatible = "rockchip,rk3588-dwc3", "snps,dwc3";
> 
> Compatible method #2:
> 	{
> 		.compatible = "snps,dwc3"
> 	},
> 
>>
>> Synopsys Ethernet Controller:
>>     Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>     compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
> 
> Compatible method #1:
> 	{ .compatible = "rockchip,rk3588-gmac", .data = &rk3588_ops },
> 
> 	    of_device_is_compatible(np, "snps,dwmac-4.20a") ||
> 
>>
>> Synsopsys SATA Controller:
>>     Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml
>>     compatible = "rockchip,rk3588-dwc-ahci", "snps,dwc-ahci"
> 
> Compatible method #2:
> 	{ .compatible = "snps,dwc-ahci", &ahci_dwc_plat },
> 
>>
>> It's also not specific to Synopsys (but RK3588 has a lot of Synopsys
>> design incl. the HDMI-RX IP currently worked on by Shreeya). Here
>> are some other examples:
>>
>> ARM Mali GPU:
>>     Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
>>     compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
> 
> Should be compatible method #2:
> 	{ .compatible = "rockchip,rk3588-mali" },
> 	{ .compatible = "arm,mali-valhall-csf" },
> 
> This is wrong!

Except that it is pointless and redundant, why is it wrong? You did not
bring any argument, except "will trigger 2 probes" which is clearly false.

> Each strings will trigger a probe.

What? That's not true.

> The string "rockchip,rk3588-mali" should be removed.
> 
> Review was done by Collabora people and without including the Rockchip mail list.
> https://patchwork.freedesktop.org/patch/msgid/20240229162230.2634044-12-boris.brezillon@collabora.com
> 
> Could someone look at this and test.

No need, just read how device matching and probing works...




Best regards,
Krzysztof
  

On 25/07/2024 08:35, Krzysztof Kozlowski wrote:
> On 24/07/2024 15:20, Johan Jonker wrote:
>>
>>>
>>> Where did you learn that? Having non-SoC specific generic fallback
>>> compatibles is pretty much standard throughout the kernel. See for
>>> example these RK3588 DesignWare compatibles:
>>>
>>> Synopsys Serial Controller:
>>>     Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
>>>     compatible = "rockchip,rk3588-uart", "snps,dw-apb-uart";
>>
>> Compatible method #2:
>> 	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
>>
>>>
>>> Synopsys USB3 Controller:
>>>     Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
>>>     compatible = "rockchip,rk3588-dwc3", "snps,dwc3";
>>
>> Compatible method #2:
>> 	{
>> 		.compatible = "snps,dwc3"
>> 	},
>>
>>>
>>> Synopsys Ethernet Controller:
>>>     Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>     compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
>>
>> Compatible method #1:
>> 	{ .compatible = "rockchip,rk3588-gmac", .data = &rk3588_ops },
>>
>> 	    of_device_is_compatible(np, "snps,dwmac-4.20a") ||
>>
>>>
>>> Synsopsys SATA Controller:
>>>     Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml
>>>     compatible = "rockchip,rk3588-dwc-ahci", "snps,dwc-ahci"
>>
>> Compatible method #2:
>> 	{ .compatible = "snps,dwc-ahci", &ahci_dwc_plat },
>>
>>>
>>> It's also not specific to Synopsys (but RK3588 has a lot of Synopsys
>>> design incl. the HDMI-RX IP currently worked on by Shreeya). Here
>>> are some other examples:
>>>
>>> ARM Mali GPU:
>>>     Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
>>>     compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
>>
>> Should be compatible method #2:
>> 	{ .compatible = "rockchip,rk3588-mali" },
>> 	{ .compatible = "arm,mali-valhall-csf" },
>>
>> This is wrong!
> 
> Except that it is pointless and redundant, why is it wrong? You did not
> bring any argument, except "will trigger 2 probes" which is clearly false.
> 
>> Each strings will trigger a probe.
> 
> What? That's not true.

Although if you meant "any string will trigger one probe in total", then
it would be true, so maybe that's what you meant.

But then - what's wrong with this (except needless redundancy)? You did
not bring any argument but keep calling more than once "wrong". So what
is wrong?

Best regards,
Krzysztof
  

Il 24/07/24 15:20, Johan Jonker ha scritto:
> 
> 
> On 7/23/24 19:28, Sebastian Reichel wrote:
>> Hi,
>>
>> On Tue, Jul 23, 2024 at 01:16:00PM GMT, Johan Jonker wrote:
>>> On 7/22/24 15:53, Shreeya Patel wrote:
>>>> On Saturday, July 20, 2024 16:14 IST, Johan Jonker <jbx6244@yandex.com> wrote:
>>>>> On 7/19/24 14:40, Shreeya Patel wrote:
>>>>>> Document bindings for the Synopsys DesignWare HDMI RX Controller.
>>>>>>
>>>
>>>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>
>>> Remove to trigger a new review.
>>
>> Rob and Dmitry both already reviewed the version with the fallback
>> compatible. I don't think the rename of hdmirx_cma to hdmi_receiver_cma
>> warrant a new review. Also FWIW:
>>
> 
>> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> Please have a look at the comments below before you tag.
> 

I have checked the (mostly wrong) comments before tagging.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Regards,
Angelo

>>
>>>>>> Signed-off-by: Shreeya Patel <shreeya.patel@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@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
>>>
>>>>>
> 
> Relevant compatible methods in use for Rockchip drivers:
> 
> ===================================================================================================
> 
> Compatible method #1:
> Probe is triggered by a SoC orientated string.
> 
> compatible = "rockchip,rk3588-hdmirx-ctrler";
> 
> If for example a new SoC rk3599 is released that has the same device properties
> then the old string can be used as fallback string.
> 
> compatible = ""rockchip,rk3599-hdmirx-ctrler" , "rockchip,rk3588-hdmirx-ctrler";
> 
> The driver structure:
> { .compatible = "rockchip,rk3588-hdmirx-ctrler" },
> 
> ===================================================================================================
> Compatible method #2:
> Probe is triggered by a IP orientated fallback string.
> 
> compatible = "rockchip,rk3588-hdmirx-ctrler" , "snps,dw-hdmi-rx";
> 
> If for example a new SoC rk3599 is released that has the same device properties
> then add the same fallback string.
> 
> compatible = ""rockchip,rk3599-hdmirx-ctrler" , "snps,dw-hdmi-rx";
> 
> The driver structure:
> { .compatible = "snps,dw-hdmi-rx" },
> 
> If for example a new SoC rk3599 is released that has NOT the same device properties
> then use method #1.
> 
> The driver structure:
> { .compatible = "rockchip,rk3599-hdmirx-ctrler" .data = &rk3599_ops },
> { .compatible = "snps,dw-hdmi-rx" },
> 
> ===================================================================================================
> 
> Compatible method #3:
> Probe is triggered by a vendor orientated fallback string.
> 
> Special case only useful if the driver is written long after all SoCs are released.
> The standalone IP has a version register and the driver can handle all the feature difference
> inside the IP depending on the version register.
> 
> compatible = "rockchip,sfc";
> 
> The driver structure:
> { .compatible = "rockchip,sfc"},
> 
> ===================================================================================================
> 
> The rules:
> 
> 1: Compatible strings must be SoC orientated.
> 2: In Linux there's no priority in which string will probed first.
> 3: There is a commitment that old DT's should still work with newer kernels.
> 
>>>>> 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" },
>>>>> +	{ },
>>>>> +};
>>>>>
> 
> The consequence of the third rule is that drivers must continue to support this string once added and
> can not be removed as suggested below.
> 
> If for example the fallback is added later it will trigger 2 probes and it breaks rule #2.
> Only one of string is allowed to trigger a probe in the driver.
> 
> This is wrong:
> compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
> 
> { .compatible = "rockchip,rk3588-hdmirx-ctrler" },
> { .compatible = "snps,dw-hdmi-rx" },
> 
> Ones a compatible method is chosen the driver must stick to it.
> 
> ===================================================================================================
> 
>>>>
>>>
>>>> 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.
> 
> This refers to compatible method #1.
> 
>>
>> Where did you learn that? Having non-SoC specific generic fallback
>> compatibles is pretty much standard throughout the kernel. See for
>> example these RK3588 DesignWare compatibles:
>>
>> Synopsys Serial Controller:
>>      Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
>>      compatible = "rockchip,rk3588-uart", "snps,dw-apb-uart";
> 
> Compatible method #2:
> 	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
> 
>>
>> Synopsys USB3 Controller:
>>      Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
>>      compatible = "rockchip,rk3588-dwc3", "snps,dwc3";
> 
> Compatible method #2:
> 	{
> 		.compatible = "snps,dwc3"
> 	},
> 
>>
>> Synopsys Ethernet Controller:
>>      Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>      compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
> 
> Compatible method #1:
> 	{ .compatible = "rockchip,rk3588-gmac", .data = &rk3588_ops },
> 
> 	    of_device_is_compatible(np, "snps,dwmac-4.20a") ||
> 
>>
>> Synsopsys SATA Controller:
>>      Documentation/devicetree/bindings/ata/rockchip,dwc-ahci.yaml
>>      compatible = "rockchip,rk3588-dwc-ahci", "snps,dwc-ahci"
> 
> Compatible method #2:
> 	{ .compatible = "snps,dwc-ahci", &ahci_dwc_plat },
> 
>>
>> It's also not specific to Synopsys (but RK3588 has a lot of Synopsys
>> design incl. the HDMI-RX IP currently worked on by Shreeya). Here
>> are some other examples:
>>
>> ARM Mali GPU:
>>      Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
>>      compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
> 
> Should be compatible method #2:
> 	{ .compatible = "rockchip,rk3588-mali" },
> 	{ .compatible = "arm,mali-valhall-csf" },
> 
> This is wrong!
> Each strings will trigger a probe.
> The string "rockchip,rk3588-mali" should be removed.
> 
> Review was done by Collabora people and without including the Rockchip mail list.
> https://patchwork.freedesktop.org/patch/msgid/20240229162230.2634044-12-boris.brezillon@collabora.com
> 
> Could someone look at this and test.
> 
>>
>> Generic EHCI:
>>      Documentation/devicetree/bindings/usb/generic-ehci.yaml
>>      compatible = "rockchip,rk3588-ehci", "generic-ehci";
> 
> compatible method #2:
> 	{ .compatible = "generic-ehci", },
> 
>>
>> As you can see almost everything in RK3588 has a non SoC specific
>> fallback :) It's also not a Rockchip/RK3588 specific thing, but
>> I think you should be able to find enough references yourself by
>> looking into the kernel's DTS files.
> 
> You are mixing up 2 compatible methods.
> The driver has compatible method #1 and the DT has method #2.
> 
>>
>>> Don't invent strings for devices that we don't know yet if it
>>> might or might not be compatible in the future.
>>
>> Right now it's a sensible assumption, that an operating system driver
>> for this hardware (i.e. not necessarily the one submitted by Shreeya
>> right now) can handle the Synopsys HDMI receiver hardware from different
>> SoCs just like it is the case for other Synopsys IP.
>>
>> Whatever is being done now is set in stone, since DT is considered
>> ABI. So without the fallback compatible being available in DT from
>> the beginning we need to carry the RK3588 specific compatible in the
> 
>> kernel driver forever. OTOH if we add the generic one now, the kernel
>> can switch to use the generic one at any point in time and ignore the
>> RK3588 specific one.
> 
> Ignoring breaks rule #3 as explained above.
> 
> For you the task to select a compatible method:
> 
> If the IP device registers are guaranteed remain the same then choose compatible method #2 and fix the driver.
> If in doubt choose compatible method #1 and fix the binding.
> 
> Johan
> 
>>
>> Greetings,
>>
>> -- Sebastian
>>
>>> 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@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>;
>>>>>> +    };
>>>>
>>> _______________________________________________
>>> Kernel mailing list -- kernel@mailman.collabora.com
>>> To unsubscribe send an email to kernel-leave@mailman.collabora.com
>>> This list is managed by https://mailman.collabora.com
> _______________________________________________
> Kernel mailing list -- kernel@mailman.collabora.com
> To unsubscribe send an email to kernel-leave@mailman.collabora.com
> This list is managed by https://mailman.collabora.com
  

On Saturday, July 20, 2024 03:40 IST, "Rob Herring (Arm)" <robh@kernel.org> wrote:

> 
> On Fri, 19 Jul 2024 18:10:30 +0530, Shreeya Patel wrote:
> > Document bindings for the Synopsys DesignWare HDMI RX Controller.
> > 
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Signed-off-by: Shreeya Patel <shreeya.patel@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
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.example.dts:53.38-39 syntax error
> FATAL ERROR: Unable to parse input tree
> make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.example.dtb] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
> make: *** [Makefile:240: __sub-make] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240719124032.26852-3-shreeya.patel@collabora.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 

My HDMI RX patches are based on the linux-next/master branch.
Since the bot tested the patches on top of rc1, it resulted in some errors
due to missing reset ID patches.

I think the above statement means I should explicitly mention in this
patch that it is based on linux-next/master (something to keep in mind
for future :)


> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 
> _______________________________________________
> Kernel mailing list -- kernel@mailman.collabora.com
> To unsubscribe send an email to kernel-leave@mailman.collabora.com
> This list is managed by https://mailman.collabora.com
  

Hi,

On Wed, Jul 24, 2024 at 03:20:28PM GMT, Johan Jonker wrote:
> 
> 
> On 7/23/24 19:28, Sebastian Reichel wrote:
> > Hi,
> >
> > On Tue, Jul 23, 2024 at 01:16:00PM GMT, Johan Jonker wrote:
> >> On 7/22/24 15:53, Shreeya Patel wrote:
> >>> On Saturday, July 20, 2024 16:14 IST, Johan Jonker <jbx6244@yandex.com> wrote:
> >>>> On 7/19/24 14:40, Shreeya Patel wrote:
> >>>>> Document bindings for the Synopsys DesignWare HDMI RX Controller.
> >>>>>
> >>
> >>>>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>
> >> Remove to trigger a new review.
> >
> > Rob and Dmitry both already reviewed the version with the fallback
> > compatible. I don't think the rename of hdmirx_cma to hdmi_receiver_cma
> > warrant a new review. Also FWIW:
> >
> 
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> Please have a look at the comments below before you tag.
> 
> >
> >>>>> Signed-off-by: Shreeya Patel <shreeya.patel@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@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
> >>
> >>>>
> 
> Relevant compatible methods in use for Rockchip drivers:

You are arguing with kernel drivers. Drivers can be changed at any
point in time, but DT bindings cannot, because they define an ABI.

> ===================================================================================================
> 
> Compatible method #1:
> Probe is triggered by a SoC orientated string.
> 
> compatible = "rockchip,rk3588-hdmirx-ctrler";
> 
> If for example a new SoC rk3599 is released that has the same device properties
> then the old string can be used as fallback string.
> 
> compatible = ""rockchip,rk3599-hdmirx-ctrler" , "rockchip,rk3588-hdmirx-ctrler";
> 
> The driver structure:
> { .compatible = "rockchip,rk3588-hdmirx-ctrler" },
> 
> ===================================================================================================
> Compatible method #2:
> Probe is triggered by a IP orientated fallback string.
> 
> compatible = "rockchip,rk3588-hdmirx-ctrler" , "snps,dw-hdmi-rx";
> 
> If for example a new SoC rk3599 is released that has the same device properties
> then add the same fallback string.
> 
> compatible = ""rockchip,rk3599-hdmirx-ctrler" , "snps,dw-hdmi-rx";
> 
> The driver structure:
> { .compatible = "snps,dw-hdmi-rx" },
> 
> If for example a new SoC rk3599 is released that has NOT the same device properties
> then use method #1.
> 
> The driver structure:
> { .compatible = "rockchip,rk3599-hdmirx-ctrler" .data = &rk3599_ops },
> { .compatible = "snps,dw-hdmi-rx" },

This is what is being used here. The only diference is, that the
driver currently uses the RK3588 specific compatible string instead
of the fallback string right now.

If another SoC vendor adds the same IP into their latest chip and
the driver has been proven to work with their hardware the driver
can be changed to bind against "snps,dw-hdmi-rx" instead of the
RK3588 specific compatible. Doing this change will keep
compatibility with existing DTs, if we add the fallback string now.
Until then we just carry it as an unused fallback.

> ===================================================================================================
> 
> Compatible method #3:
> Probe is triggered by a vendor orientated fallback string.
> 
> Special case only useful if the driver is written long after all SoCs are released.
> The standalone IP has a version register and the driver can handle all the feature difference
> inside the IP depending on the version register.
> 
> compatible = "rockchip,sfc";
> 
> The driver structure:
> { .compatible = "rockchip,sfc"},

FWIW I think _this_ is a bad example. It is missing the SoC specific
compatible making applying of quirks harder than necessary. Just
because no quirks are needed now, does not mean it will stay that
way. E.g. if an Errata gets released that SFC on RK3588 must not be
run at 1 MHz it would be super useful to have an "rockchip,rk3588-sfc"
to match against. That's the reason for the rule #1 in the following
list

> ===================================================================================================
> 
> The rules:
> 
> 1: Compatible strings must be SoC orientated.
> 2: In Linux there's no priority in which string will probed first.

I initially thought you mean the list in the driver, but after
reading your remaining mail - this is just wrong. There is a
priority. If DT specifies

compatible = "main", "fallback";

Linux will first try to bind against "main". If that does not
work it will try to bind against "fallback". To give you a
simple example from the subsystem I maintain:

DT binding: Documentation/devicetree/bindings/power/supply/sbs,sbs-battery.yaml
Linux Kernel Driver: drivers/power/supply/sbs-battery.c

Valid compatibles according to the DT binding:
 compatible = "ti,bq20z45", "sbs,sbs-battery";
 compatible = "ti,bq20z65", "sbs,sbs-battery";
 compatible = "ti,bq20z75", "sbs,sbs-battery";
 compatible = "sbs,sbs-battery";

The driver has these of_device_id entries:
 { .compatible = "sbs,sbs-battery" },
 { .compatible = "ti,bq20z65", QUIRKS },
 { .compatible = "ti,bq20z75", QUIRKS },

The driver will probe ONCE in all cases.

compatible = "ti,bq20z45", "sbs,sbs-battery"; => probe happens with fallback string
compatible = "ti,bq20z65", "sbs,sbs-battery"; => probe happens with main string (QUIRKS apply)
compatible = "ti,bq20z75", "sbs,sbs-battery"; => probe happens with main string (QUIRKS apply)
compatible = "sbs,sbs-battery"; => probe happens with main string

> 3: There is a commitment that old DT's should still work with newer kernels.

> >>>> 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" },
> >>>> +	{ },
> >>>> +};
> >>>>
> 
> The consequence of the third rule is that drivers must continue to
> support this string once added and can not be removed as suggested
> below.

That's wrong. We can remove the "rockchip,rk3588-hdmirx-ctrler" from
the kernel driver and use the fallback string at any point in time
__IF__ we make it mandatory that rockchip,rk3588-hdmirx-ctrler must
always be followed by the fallback compatible in DT. Because then we
will keep working with old DTs, since the old DT also has the
fallback compatible.

> If for example the fallback is added later it will trigger 2 probes and it breaks rule #2.
> Only one of string is allowed to trigger a probe in the driver.
> 
> This is wrong:
> compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
> 
> { .compatible = "rockchip,rk3588-hdmirx-ctrler" },
> { .compatible = "snps,dw-hdmi-rx" },
> 
> Ones a compatible method is chosen the driver must stick to it.

I don't know how you came to that conclusion, but it's simply wrong.
The above example will probe once using the "rockchip,rk3588-hdmirx-ctrler"
compatible. At that point the DT node is marked as processed, so
no other probe happens.

> ===================================================================================================
> 
> >>>
> >>
> >>> 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.
> 
> This refers to compatible method #1.

Yeah, which is used when the IP is from the SoC vendor itself (or
unknown) or if its not possible to use the generic IP compatible
anyways.

> [...]
> If the IP device registers are guaranteed remain the same then
> choose compatible method #2 and fix the driver.

Adapting the driver is the plan, but it cannot be done right now
because lack of information. This requires either another SoC with
this IP or at least the Synopsys documentation. We only have the
Rockchip documentation.

But the driver can be fixed in the future and the DT binding is
ABI. Thus the DT binding is prepared now to allow the driver looking
like your method #2 in the future. Until then the extra compatible
will just be ignored.

-- Sebastian
  

On 19/07/2024 14:40, Shreeya Patel wrote:
> Document bindings for the Synopsys DesignWare HDMI RX Controller.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>

If you are going to send a new version, then:

A nit, subject: drop second/last, redundant "Document bindings for". The
"dt-bindings" prefix is already stating that these are bindings and this
is documentation.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


"Add Synopsys HDMI RX Controller".


Best regards,
Krzysztof
  

Hi Shreeya, hi all,

First of all thank you very much for your efforts and patches!

I have got more of a general question that came up during my recent work
on a HDMI RX companion chip driver. It may or may not be out of scope of
this initial submission for the Synopsys HDMI RX IP core, but anyway I
feel like I should ask it now:

On 7/19/24 14:40, Shreeya Patel wrote:
> [...]
> +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@fdee0000 {
> +      compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
> +      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>;
> +    };

Should HDMI RX connectors be described in the device tree?

It seems that V4L2 features support for svidio and composite connectors,
but newer standards such as HDMI never made it to e.g,
drivers/media/v4l2-core/v4l2-fwnode.c. The name of the corresponding DT
binding
Documentation/devicetree/bindings/display/connector/analog-tv-connector.yaml
somewhat confirms that only analog connectors are in the scope there.
This means that some initial discussion and effort on the framework
would be required.

However, this would enable some nice (although not exactly killer)
features: describing label, orientation, and connector (sub)type on DT
level, thus eliminating the need to know the HW description in the
higher SW layers.

If the notion of HDMI RX (or, in general, digital video input)
connectors in DT sounds reasonable to you all, there may be the chance
to rework the submission at hand accordingly to avoid any compatibility
issues with older/newer device trees. This is why I am bringing this up
right now.

What do you all think?

Best regards,
Michael