Re: [PATCH v4 03/11] arm64: dts: qcom: x1e80100: Add CAMSS block definition

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Phantom standalone PHY child nodes inside the `camss` node violate the CAMSS driver architecture and trap essential power domains, risking system hangs.
- [Medium] The commit message claims '2 x CSID', but the patch actually maps and exposes 3 CSID blocks.
--

--- Patch [3]: [PATCH v4 03/11] arm64: dts: qcom: x1e80100: Add CAMSS block definition ---
commit 9011a44664267ff0299bfa7f7504bc4b9b3c798b
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

arm64: dts: qcom: x1e80100: Add CAMSS block definition

This commit adds the device tree node to describe the x1e80100 CAMSS block,
including CSIPHY, TPG, CSID, and IFE components.

> 2 x CSID

[Severity: Medium]
Is this description accurate?

The commit message states there are 2 x CSID blocks, but the device tree
addition maps 3 CSID instances (csid0, csid1, and csid2). Should the commit
message be updated to reflect the 3 instances?

> diff --git a/arch/arm64/boot/dts/qcom/hamoa.dtsi b/arch/arm64/boot/dts/qcom/hamoa.dtsi
> index 6a14d7c9a7ec4..e043095f2da74 100644
> --- a/arch/arm64/boot/dts/qcom/hamoa.dtsi
> +++ b/arch/arm64/boot/dts/qcom/hamoa.dtsi
[ ... ]
> @@ -5629,6 +5630,368 @@ cci1_i2c1: i2c-bus@1 {
[ ... ]
> +		camss: isp@acb7000 {
> +			compatible = "qcom,x1e80100-camss";
> +
> +			reg = <0 0x0acb7000 0 0x2000>,
> +			      <0 0x0acb9000 0 0x2000>,
> +			      <0 0x0acbb000 0 0x2000>,
> +			      <0 0x0acc6000 0 0x1000>,
> +			      <0 0x0acca000 0 0x1000>,
> +			      <0 0x0acb6000 0 0x1000>,
> +			      <0 0x0ace4000 0 0x1000>,
> +			      <0 0x0ace6000 0 0x1000>,
> +			      <0 0x0ace8000 0 0x1000>,
> +			      <0 0x0acec000 0 0x4000>,
[ ... ]
> +			phys = <&csiphy0 PHY_TYPE_DPHY>,
> +			       <&csiphy1 PHY_TYPE_DPHY>,
> +			       <&csiphy2 PHY_TYPE_DPHY>,
> +			       <&csiphy4 PHY_TYPE_DPHY>;
> +			phy-names = "csiphy0",
> +				    "csiphy1",
> +				    "csiphy2",
> +				    "csiphy4";
[ ... ]
> +			csiphy0: phy@ace4000 {
> +				compatible = "qcom,x1e80100-csi2-phy";
> +				reg = <0 0x0ace4000 0 0x2000>;
> +
> +				clocks = <&camcc CAM_CC_CSIPHY0_CLK>,
> +					 <&camcc CAM_CC_CSI0PHYTIMER_CLK>,
> +					 <&camcc CAM_CC_CORE_AHB_CLK>;
> +				clock-names = "core",
> +					      "timer",
> +					      "ahb";
> +
> +				operating-points-v2 = <&csiphy_mxc_opp_table>;
> +
> +				interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>;
> +
> +				power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>,
> +						<&rpmhpd RPMHPD_MMCX>,
> +						<&rpmhpd RPMHPD_MXC>;
> +				power-domain-names = "top",
> +						     "mmcx",
> +						     "mx";

[Severity: High]
Will this cause a system hang when attempting to access the CSIPHY hardware?

It looks like the upstream CAMSS driver directly maps these PHY registers in
the parent camss node's reg block, and does not use the generic PHY framework
defined by the phys and phy-names properties.

Since no driver binds to the qcom,x1e80100-csi2-phy compatible string, these
child nodes appear to be dead code that trap the RPMHPD_MMCX and RPMHPD_MXC
power domains. 

If these power domains are never enabled by a PHY driver, could probing the
CAMSS driver and accessing the CSIPHY registers result in unpowered hardware
and a bus hang?

Also, does the reg property here (0x0ace4000) improperly overlap with the
parent node's reg claim?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-x1e-camss-csi2-phy-dtsi-v4-0-572348ad1b2a@linaro.org?part=3