Re: [PATCH 3/3] arm64: dts: qcom: kodiak: add CSI2 D-PHY common-phy providers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] arm64: dts: qcom: kodiak: duplicate CSIPHY MMIO and IRQ with camss node
- [Medium] arm64: dts: qcom: kodiak: undocumented qcom,sc7280-csi2-phy compatible
- [Medium] arm64: dts: qcom: kodiak: missing #phy-cells in csiphy0..4 provider nodes
- [Low] arm64: dts: qcom: kodiak: opp-table-csiphy placed inside simple-bus soc@0
--

commit 8059004601c54f69c5b842a91e910a3594ccac37
Author: Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>

arm64: dts: qcom: kodiak: add CSI2 D-PHY common-phy providers

This commit adds five standalone qcom,sc7280-csi2-phy device tree nodes
(csiphy0 through csiphy4) and a shared opp-table-csiphy operating points
table to arch/arm64/boot/dts/qcom/kodiak.dtsi, disabled by default.

> diff --git a/arch/arm64/boot/dts/qcom/kodiak.dtsi b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> index f2da3706d5c88..bdac6b149aee8 100644
> --- a/arch/arm64/boot/dts/qcom/kodiak.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> @@ -5151,6 +5151,178 @@ cci1_i2c1: i2c-bus@1 {
>  			};
>  		};
>  
> +		csiphy0: phy@ace0000 {
> +			compatible = "qcom,sc7280-csi2-phy";
> +			reg = <0x0 0x0ace0000 0x0 0x2000>;

[Severity: Medium]
Is there a Devicetree binding schema or driver series that this patch depends
on?

Neither Documentation/devicetree/bindings/ nor drivers/ in the tree contains
the compatible string qcom,sc7280-csi2-phy (repeated on csiphy0 through
csiphy4), and camss_detect_legacy_phy() referenced in the commit message does
not exist in drivers/media/platform/qcom/camss/. Running dtbs_check fails on
the undocumented compatible string, and enabling any of csiphy0..csiphy4 at
runtime leaves the platform device unbound.

> +
> +			clocks = <&camcc CAM_CC_CSIPHY0_CLK>,
> +				 <&camcc CAM_CC_CSI0PHYTIMER_CLK>;
> +			clock-names = "core",
> +				      "timer";
> +
> +			interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>;

[Severity: High]
Will enabling any of csiphy0 through csiphy4 alongside camss cause a resource
conflict (-EBUSY) during probe?

The standalone csiphy0..csiphy4 nodes declare the same MMIO ranges
(0x0ace0000, 0x0ace2000, 0x0ace4000, 0x0ace6000, and 0x0ace8000) and GIC_SPI
interrupts (477, 478, 479, 448, and 122) that remain listed in the sibling
camss node (isp@acb3000):

arch/arm64/boot/dts/qcom/kodiak.dtsi:
		camss: isp@acb3000 {
			compatible = "qcom,sc7280-camss";

			reg = <0x0 0x0acb3000 0x0 0x1000>,
			      <0x0 0x0acba000 0x0 0x1000>,
			      <0x0 0x0acc1000 0x0 0x1000>,
			      <0x0 0x0acc8000 0x0 0x1000>,
			      <0x0 0x0accf000 0x0 0x1000>,
			      <0x0 0x0ace0000 0x0 0x2000>,
			      <0x0 0x0ace2000 0x0 0x2000>,
			      <0x0 0x0ace4000 0x0 0x2000>,
			      <0x0 0x0ace6000 0x0 0x2000>,
			      <0x0 0x0ace8000 0x0 0x2000>,

During camss_probe() -> camss_init_subdevices(), the CAMSS driver
unconditionally iterates over all camss->res->csiphy_num entries:

drivers/media/platform/qcom/camss/camss.c:camss_init_subdevices() {
	for (i = 0; i < camss->res->csiphy_num; i++) {
		ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
					     &res->csiphy_res[i],
					     res->csiphy_res[i].csiphy.id);
}

And msm_csiphy_subdev_init() claims exclusive ownership of each memory region
and non-shared interrupt line:

drivers/media/platform/qcom/camss/camss-csiphy.c:msm_csiphy_subdev_init() {
	csiphy->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
	if (IS_ERR(csiphy->base))
		return PTR_ERR(csiphy->base);
	...
	ret = devm_request_irq(dev, csiphy->irq, csiphy->res->hw_ops->isr,
			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
			       csiphy->irq_name, csiphy);
}

> +
> +			operating-points-v2 = <&csiphy_opp_table>;
> +
> +			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +			power-domain-names = "top";
> +
> +			status = "disabled";

[Severity: Medium]
Should csiphy0 through csiphy4 include #phy-cells = <0>?

Because these nodes are named phy@ace0000 through phy@ace8000 and act as
generic-PHY providers, phy-provider.yaml requires #phy-cells during
dtbs_check, and _of_phy_get() in drivers/phy/phy-core.c requires #phy-cells
when resolving a PHY phandle:

drivers/phy/phy-core.c:_of_phy_get() {
	ret = of_parse_phandle_with_args(np, "phys", "#phy-cells",
		index, &args);
	if (ret)
		return ERR_PTR(-ENODEV);
}

[ ... ]

> +		csiphy_opp_table: opp-table-csiphy {
> +			compatible = "operating-points-v2";
> +
> +			opp-300000000 {
> +				opp-hz = /bits/ 64 <300000000>;
> +			};
> +
> +			opp-400000000 {
> +				opp-hz = /bits/ 64 <400000000>;
> +			};
> +		};
> +
>  		camss: isp@acb3000 {
>  			compatible = "qcom,sc7280-camss";
>  

[Severity: Low]
Should csiphy_opp_table: opp-table-csiphy be placed under the root node
(alongside qspi_opp_table and qup_opp_table) rather than inside soc: soc@0,
and should phy@ace0000..phy@ace8000 be ordered after isp@acb3000?

Because soc: soc@0 has compatible = "simple-bus", placing opp-table-csiphy
without a reg or ranges property inside it triggers dtc simple_bus_reg
warnings (under W=1) and simple-bus.yaml schema validation errors in
dtbs_check. Also, unit addresses 0x0ace0000..0x0ace8000 sort after
isp@acb3000.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017001116.2318647-1-anusha.nandi@oss.qualcomm.com?part=3