diff mbox series

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

Message ID 20240216094922.257674-3-shreeya.patel@collabora.com
State New
Headers
Series Add Synopsys DesignWare HDMI RX Controller |

Commit Message

Shreeya Patel Feb. 16, 2024, 9:49 a.m. UTC 
  Document bindings for the Synopsys DesignWare HDMI RX Controller.

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 .../bindings/media/snps,dw-hdmi-rx.yaml       | 128 ++++++++++++++++++
 1 file changed, 128 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml

Comments

Krzysztof Kozlowski Feb. 16, 2024, 10:01 a.m. UTC | #1 
On 16/02/2024 10:49, Shreeya Patel wrote:
> Document bindings for the Synopsys DesignWare HDMI RX Controller.
> 
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>

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

> ---
>  .../bindings/media/snps,dw-hdmi-rx.yaml       | 128 ++++++++++++++++++
>  1 file changed, 128 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..a70d96b548ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> @@ -0,0 +1,128 @@
> +# SPDX-License-Identifier: (GPL-3.0 OR BSD-2-Clause)

Use license checkpatch tells you.

> +# Device Tree bindings for Synopsys DesignWare HDMI RX Controller
> +
> +---
> +$id: http://devicetree.org/schemas/media/snps,dw-hdmi-rx.yaml#

Why this is a media, not display? Does RX means input? Lack of hardware
description does not help?


> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys DesignWare HDMI RX Controller
> +
> +maintainers:
> +  - Shreeya Patel <shreeya.patel@collabora.com>
> +

description:

> +properties:
> +  compatible:
> +    items:
> +      - const: rockchip,rk3588-hdmirx-ctrler
> +      - const: snps,dw-hdmi-rx
> +
> +  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: rst_a
> +      - const: rst_p
> +      - const: rst_ref
> +      - const: rst_biu

Drop rest_ prefix

> +
> +  pinctrl-names:
> +    const: default

Drop

> +
> +  memory-region:
> +    maxItems: 1
> +
> +  hdmirx-5v-detection-gpios:
> +    description: GPIO specifier for 5V detection.

Detection of what? Isn't this HPD?

> +    maxItems: 1
> +
> +  rockchip,grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      The phandle of the syscon node for the GRF register.

Instead describe what for. Basically 80% of your description is
redundant and only "GRF register" brings some information.


> +
> +  rockchip,vo1_grf:

No underscores.

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      The phandle of the syscon node for the VO1 GRF register.

Same problem.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - resets
> +  - pinctrl-0
> +  - pinctrl-names

Why? Drop.

> +  - hdmirx-5v-detection-gpios
> +
> +additionalProperties: 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>
> +    hdmirx_ctrler: hdmirx-controller@fdee0000 {

What is hdmirx-controller? Isn't this just hdmi@?

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +      compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
> +      reg = <0x0 0xfdee0000 0x0 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>;



Best regards,
Krzysztof
Rob Herring Feb. 16, 2024, 11:30 a.m. UTC | #2 
On Fri, 16 Feb 2024 15:19:20 +0530, Shreeya Patel wrote:
> Document bindings for the Synopsys DesignWare HDMI RX Controller.
> 
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>  .../bindings/media/snps,dw-hdmi-rx.yaml       | 128 ++++++++++++++++++
>  1 file changed, 128 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.example.dts:57.47-48 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: 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:1428: 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/20240216094922.257674-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.
Shreeya Patel Feb. 21, 2024, 8:55 p.m. UTC | #3 
On Friday, February 16, 2024 15:31 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 16/02/2024 10:49, Shreeya Patel wrote:
> > Document bindings for the Synopsys DesignWare HDMI RX Controller.
> > 
> > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> 
> A nit, subject: drop second/last, redundant "bindings for". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> > ---
> >  .../bindings/media/snps,dw-hdmi-rx.yaml       | 128 ++++++++++++++++++
> >  1 file changed, 128 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..a70d96b548ee
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > @@ -0,0 +1,128 @@
> > +# SPDX-License-Identifier: (GPL-3.0 OR BSD-2-Clause)
> 
> Use license checkpatch tells you.
> 
> > +# Device Tree bindings for Synopsys DesignWare HDMI RX Controller
> > +
> > +---
> > +$id: http://devicetree.org/schemas/media/snps,dw-hdmi-rx.yaml#
> 
> Why this is a media, not display? Does RX means input? Lack of hardware
> description does not help?
> 

Yes, RX means input and this binding doc is for the HDMI INPUT controller.
I'll add some description in v2

> 
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Synopsys DesignWare HDMI RX Controller
> > +
> > +maintainers:
> > +  - Shreeya Patel <shreeya.patel@collabora.com>
> > +
> 
> description:
> 
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: rockchip,rk3588-hdmirx-ctrler
> > +      - const: snps,dw-hdmi-rx
> > +
> > +  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: rst_a
> > +      - const: rst_p
> > +      - const: rst_ref
> > +      - const: rst_biu
> 
> Drop rest_ prefix
> 
> > +
> > +  pinctrl-names:
> > +    const: default
> 
> Drop
> 
> > +
> > +  memory-region:
> > +    maxItems: 1
> > +
> > +  hdmirx-5v-detection-gpios:
> > +    description: GPIO specifier for 5V detection.
> 
> Detection of what? Isn't this HPD?
> 
> > +    maxItems: 1
> > +
> > +  rockchip,grf:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      The phandle of the syscon node for the GRF register.
> 
> Instead describe what for. Basically 80% of your description is
> redundant and only "GRF register" brings some information.
> 
> 
> > +
> > +  rockchip,vo1_grf:
> 
> No underscores.
> 
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      The phandle of the syscon node for the VO1 GRF register.
> 
> Same problem.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-names
> > +  - clocks
> > +  - clock-names
> > +  - power-domains
> > +  - resets
> > +  - pinctrl-0
> > +  - pinctrl-names
> 
> Why? Drop.
> 
> > +  - hdmirx-5v-detection-gpios
> > +
> > +additionalProperties: 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>
> > +    hdmirx_ctrler: hdmirx-controller@fdee0000 {
> 
> What is hdmirx-controller? Isn't this just hdmi@?
> 

Writing just hdmi would imply hdmi output I think so that name
will not be appropriate here.

> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 

This documentation doesn't have any generic name for HDMI INPUT
but maybe we can use the name hdmi-receiver like some other existing
binding has it here :-
Documentation/devicetree/bindings/media/i2c/tda1997x.txt

Thanks,
Shreeya Patel
> 
> > +      compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
> > +      reg = <0x0 0xfdee0000 0x0 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>;
> 
> 
> 
> Best regards,
> Krzysztof
> 
> _______________________________________________
> 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
Krzysztof Kozlowski Feb. 22, 2024, 8:30 a.m. UTC | #4 
On 21/02/2024 21:55, Shreeya Patel wrote:
>>> +  - hdmirx-5v-detection-gpios
>>> +
>>> +additionalProperties: 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>
>>> +    hdmirx_ctrler: hdmirx-controller@fdee0000 {
>>
>> What is hdmirx-controller? Isn't this just hdmi@?
>>
> 
> Writing just hdmi would imply hdmi output I think so that name
> will not be appropriate here.
> 
>> Node names should be generic. See also an explanation and list of
>> examples (not exhaustive) in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>
> 
> This documentation doesn't have any generic name for HDMI INPUT
> but maybe we can use the name hdmi-receiver like some other existing
> binding has it here :-
> Documentation/devicetree/bindings/media/i2c/tda1997x.txt

Yes, it is fine. You did not respond to other comments, so I assume you
agree with them.

Best regards,
Krzysztof
diff mbox series

Patch

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..a70d96b548ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
@@ -0,0 +1,128 @@ 
+# SPDX-License-Identifier: (GPL-3.0 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>
+
+properties:
+  compatible:
+    items:
+      - const: rockchip,rk3588-hdmirx-ctrler
+      - const: snps,dw-hdmi-rx
+
+  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: rst_a
+      - const: rst_p
+      - const: rst_ref
+      - const: rst_biu
+
+  pinctrl-names:
+    const: default
+
+  memory-region:
+    maxItems: 1
+
+  hdmirx-5v-detection-gpios:
+    description: GPIO specifier for 5V detection.
+    maxItems: 1
+
+  rockchip,grf:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      The phandle of the syscon node for the GRF register.
+
+  rockchip,vo1_grf:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      The phandle of the syscon node for the VO1 GRF register.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+  - power-domains
+  - resets
+  - pinctrl-0
+  - pinctrl-names
+  - hdmirx-5v-detection-gpios
+
+additionalProperties: 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>
+    hdmirx_ctrler: hdmirx-controller@fdee0000 {
+      compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
+      reg = <0x0 0xfdee0000 0x0 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 = "rst_a", "rst_p", "rst_ref", "rst_biu";
+      pinctrl-0 = <&hdmim1_rx_cec &hdmim1_rx_hpdin &hdmim1_rx_scl &hdmim1_rx_sda &hdmirx_5v_detection>;
+      pinctrl-names = "default";
+      memory-region = <&hdmirx_cma>;
+      hdmirx-5v-detection-gpios = <&gpio1 RK_PC6 GPIO_ACTIVE_LOW>;
+    };