Re: [RFC PATCH] arm64: dts: rockchip: add mipi csi-2 receiver nodes for the dcphys on rk3588

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Increasing `#phy-cells` to 2 breaks the existing PHY driver and violates the device tree binding schema.
- [Low] Undeclared macros `PHY_TYPE_CSI` and `PHY_TYPE_DSI` cause Device Tree Compiler (DTC) syntax errors and build failures.
--

commit 743d097de341070ad41811abb0840d431932cb63
Author: Jason Yang <jason98166@gmail.com>

arm64: dts: rockchip: add mipi csi-2 receiver nodes for the dcphys on rk3588
    
This is test patch, contains the following 3 changes:
  - raise #phy-cells to 2
  - add PHY_TYPE_DSI to the dsi nodes
  - add mipi csi0 and csi1

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> index 376ad04e07869..bcb2704037c86 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi

[ ... ]

> @@ -1568,6 +1576,66 @@ vicap_mmu: iommu@fdce0800 {
>  		status = "disabled";
>  	};
>  
> +	csi0: csi@fdd10000 {
> +		compatible = "rockchip,rk3588-mipi-csi2", "rockchip,rk3568-mipi-csi2";
> +		reg = <0x0 0xfdd10000 0x0 0x10000>;
> +		interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH 0>;
> +		interrupt-names = "err1", "err2";
> +		clocks = <&cru PCLK_CSI_HOST_0>;
> +		phys = <&mipidcphy0 PHY_TYPE_DPHY PHY_TYPE_CSI>;

[Severity: Low]
Will this compile correctly? PHY_TYPE_CSI does not appear to be defined
in include/dt-bindings/phy/phy.h or any included header, which could
cause device tree compiler syntax errors and build failures due to
raw string tokens left by the C preprocessor.

> +		power-domains = <&power RK3588_PD_VI>;
> +		resets = <&cru SRST_P_CSI_HOST_0>;
> +		status = "disabled";

[ ... ]

> @@ -1788,7 +1856,7 @@ dsi0: dsi@fde20000 {
>  		resets = <&cru SRST_P_DSIHOST0>;
>  		reset-names = "apb";
>  		power-domains = <&power RK3588_PD_VOP>;
> -		phys = <&mipidcphy0 PHY_TYPE_DPHY>;
> +		phys = <&mipidcphy0 PHY_TYPE_DPHY PHY_TYPE_DSI>;

[Severity: Low]
Is PHY_TYPE_DSI defined? Similar to PHY_TYPE_CSI, this undeclared macro
can lead to syntax errors when compiling the arm64 dtbs.

>  		phy-names = "dcphy";
>  		rockchip,grf = <&vop_grf>;
>  		status = "disabled";

[ ... ]

> @@ -3429,7 +3497,7 @@ mipidcphy0: phy@feda0000 {
>  			 <&cru SRST_P_MIPI_DCPHY0_GRF>,
>  			 <&cru SRST_S_MIPI_DCPHY0>;
>  		reset-names = "m_phy", "apb", "grf", "s_phy";
> -		#phy-cells = <1>;
> +		#phy-cells = <2>;

[Severity: High]
Does this change break the existing PHY driver?

The rockchip samsung dcphy binding schema currently restricts #phy-cells
to 1 (Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml).

Additionally, the driver explicitly rejects any request with more than 1
argument:

drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c:samsung_mipi_dcphy_xlate() {
	if (args->args_count != 1) {
		dev_err(dev, "invalid number of arguments\n");
		return ERR_PTR(-EINVAL);
	}
}

Will increasing this to 2 cause the driver to return -EINVAL during probe,
breaking display and camera initialization on this platform?

>  		status = "disabled";
>  	};
>  
> @@ -3445,7 +3513,7 @@ mipidcphy1: phy@fedb0000 {
>  			 <&cru SRST_P_MIPI_DCPHY1_GRF>,
>  			 <&cru SRST_S_MIPI_DCPHY1>;
>  		reset-names = "m_phy", "apb", "grf", "s_phy";
> -		#phy-cells = <1>;
> +		#phy-cells = <2>;

[Severity: High]
Can this also cause the same probe failure for the second PHY instance
due to the driver's strict args_count check?

>  		status = "disabled";
>  	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904075534.574037-1-jason98166@gmail.com?part=1