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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9489391.T7Z3S40VBb@diego>
Date: Thu, 06 Mar 2025 14:06:40 +0100
From: Heiko Stübner <heiko@...ech.de>
To: Shreeya Patel <shreeya.patel@...labora.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Hans Verkuil <hverkuil@...all.nl>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 jose.abreu@...opsys.com, nelson.costa@...opsys.com, shawn.wen@...k-chips.com,
 nicolas.dufresne@...labora.com,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Dmitry Osipenko <dmitry.osipenko@...labora.com>
Cc: kernel@...labora.com, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, Tim Surber <me@...surber.de>,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>,
 Diederik de Haas <didi.debian@...ow.org>
Subject:
 Re: [PATCH v14 1/3] arm64: dts: rockchip: Add device tree support for HDMI RX
 Controller

Hi Dmitry,

Am Donnerstag, 6. März 2025, 08:28:40 MEZ schrieb Dmitry Osipenko:
> From: Shreeya Patel <shreeya.patel@...labora.com>
> 
> Add device tree support for Synopsys DesignWare HDMI RX
> Controller.
> 
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
> Tested-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
> Co-developed-by: Dingxian Wen <shawn.wen@...k-chips.com>
> Signed-off-by: Dingxian Wen <shawn.wen@...k-chips.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@...labora.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@...labora.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@...00000 {
>  		status = "disabled";
>  	};
>  
> +	hdmi_receiver: hdmi_receiver@...e0000 {
> +		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