Re: [PATCH v14 1/3] arm64: dts: rockchip: Add device tree support for HDMI RX Controller

["Heiko Stübner" <heiko@sntech.de>]

Hi Dmitry,

Am Donnerstag, 6. März 2025, 08:28:40 MEZ schrieb Dmitry Osipenko:
> From: Shreeya Patel <shreeya.patel@collabora.com>
> 
> Add device tree support for Synopsys DesignWare HDMI RX
> Controller.
> 
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Co-developed-by: Dingxian Wen <shawn.wen@rock-chips.com>
> Signed-off-by: Dingxian Wen <shawn.wen@rock-chips.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  .../dts/rockchip/rk3588-base-pinctrl.dtsi     | 14 +++++
>  .../arm64/boot/dts/rockchip/rk3588-extra.dtsi | 57 +++++++++++++++++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi
> index 7f874c77410c..2d4b9986a177 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi
> @@ -594,6 +594,20 @@ hdmim0_tx1_hpd: hdmim0-tx1-hpd {
>  				/* hdmim0_tx1_hpd */
>  				<1 RK_PA6 5 &pcfg_pull_none>;
>  		};
> +
> +		/omit-if-no-ref/
> +		hdmim1_rx: hdmim1-rx {
> +			rockchip,pins =
> +				/* hdmim1_rx_cec */
> +				<3 RK_PD1 5 &pcfg_pull_none>,
> +				/* hdmim1_rx_scl */
> +				<3 RK_PD2 5 &pcfg_pull_none_smt>,
> +				/* hdmim1_rx_sda */
> +				<3 RK_PD3 5 &pcfg_pull_none_smt>,
> +				/* hdmim1_rx_hpdin */
> +				<3 RK_PD4 5 &pcfg_pull_none>;
> +		};
> +

what's the reason for duplicating these pinctrl entries?

The base-pinctrl already contains a separate set of pins (and also a
variant for the m0 set of pins), so why not check and use the already
existing ones:

                hdmim1_rx_cec: hdmim1-rx-cec {
                        rockchip,pins =
                                /* hdmim1_rx_cec */
                                <3 RK_PD1 5 &pcfg_pull_none>;
                };

                hdmim1_rx_hpdin: hdmim1-rx-hpdin {
                        rockchip,pins =
                                /* hdmim1_rx_hpdin */
                                <3 RK_PD4 5 &pcfg_pull_none>;
                };

                hdmim1_rx_scl: hdmim1-rx-scl {
                        rockchip,pins =
                                /* hdmim1_rx_scl */
                                <3 RK_PD2 5 &pcfg_pull_none>;
                };

                hdmim1_rx_sda: hdmim1-rx-sda {
                        rockchip,pins =
                                /* hdmim1_rx_sda */
                                <3 RK_PD3 5 &pcfg_pull_none>;
                };


Having multiple sets (even with differeing settings) will cause confusion
later on.


>  		/omit-if-no-ref/
>  		hdmim1_rx_cec: hdmim1-rx-cec {
>  			rockchip,pins =
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
> index 4a950907ea6f..b7d06f93c8ce 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi
> @@ -135,6 +159,39 @@ i2s10_8ch: i2s@fde00000 {
>  		status = "disabled";
>  	};
>  
> +	hdmi_receiver: hdmi_receiver@fdee0000 {
> +		compatible = "rockchip,rk3588-hdmirx-ctrler", "snps,dw-hdmi-rx";
> +		reg = <0x0 0xfdee0000 0x0 0x6000>;
> +		power-domains = <&power RK3588_PD_VO1>;
> +		rockchip,grf = <&sys_grf>;
> +		rockchip,vo1-grf = <&vo1_grf>;
> +		interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 178 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";
> +		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>;
> +		pinctrl-names = "default";

hmm, this might be better living in the board dts?

Heiko