Re: [PATCH v2 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 3 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: kodiak: undocumented qcom,sc7280-csi2-phy compatible
- [Medium] arm64: dts: qcom: kodiak: single power-domain on csiphy0..4 breaks probe
- [Low] arm64: dts: qcom: kodiak: opp-table-csiphy placed inside simple-bus soc@0
--

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

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

This commit adds standalone qcom,sc7280-csi2-phy generic-PHY provider nodes
(csiphy0..csiphy4) and a shared csiphy_opp_table in kodiak.dtsi so boards can
wire sensors through the non-legacy CAMSS PHY path.

> diff --git a/arch/arm64/boot/dts/qcom/kodiak.dtsi b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> index 8c7523602d271..3f3a41cfaa001 100644
> --- a/arch/arm64/boot/dts/qcom/kodiak.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> @@ -5164,6 +5164,178 @@ cci1_i2c1: i2c-bus@1 {
>  			};
>  		};
>  
> +		csiphy0: phy@ace0000 {
> +			compatible = "qcom,sc7280-csi2-phy";

[Severity: Medium]
As reported by Sashiko on v1 [1], is there a companion DT binding and PHY
driver series for "qcom,sc7280-csi2-phy"?

In the current tree, Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml
only documents "qcom,x1e80100-csi2-phy" and "qcom,glymur-csi2-phy", and
phy_qcom_mipi_csi2_of_match_table in drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
only matches "qcom,x1e80100-csi2-phy":

static const struct of_device_id phy_qcom_mipi_csi2_of_match_table[] = {
	{ .compatible	= "qcom,x1e80100-csi2-phy", .data = &mipi_csi2_dphy_4nm_x1e },
	{ }
};

Without a binding and driver match for "qcom,sc7280-csi2-phy" (used across
csiphy0..csiphy4), dtbs_check fails schema validation, and enabling any of
these nodes on a board or overlay causes camss_detect_legacy_phy() ->
camss_init_subdevices() -> msm_csiphy_subdev_init() ->
devm_phy_get_by_of_node() to fail with permanent -EPROBE_DEFER because no PHY
provider driver binds to the remote node.

> +			reg = <0x0 0x0ace0000 0x0 0x2000>;
> +
> +			clocks = <&camcc CAM_CC_CSIPHY0_CLK>,
> +				 <&camcc CAM_CC_CSI0PHYTIMER_CLK>;
> +			clock-names = "core",
> +				      "timer";
> +
> +			interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>;
> +
> +			operating-points-v2 = <&csiphy_opp_table>;
> +
> +			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +			power-domain-names = "top";

[Severity: Medium]
Will specifying only a single power-domains entry on csiphy0..csiphy4 cause
phy_qcom_mipi_csi2_probe() to fail with -EEXIST when a board enables one of
these PHYs?

When a platform device has a single power-domains entry, platform_probe() ->
dev_pm_domain_attach() -> genpd_dev_pm_attach() attaches it directly to
dev->pm_domain before invoking the driver's probe callback:

drivers/pmdomain/core.c:genpd_dev_pm_attach()
	if (of_count_phandle_with_args(dev->of_node, "power-domains",
				       "#power-domain-cells") != 1)
		return 0;

	return __genpd_dev_pm_attach(dev, dev, 0, 1, true);

When phy_qcom_mipi_csi2_probe() then calls
phy_qcom_mipi_csi2_attach_pm_domains() -> devm_pm_domain_attach_list() ->
dev_pm_domain_attach_list(), it sees dev->pm_domain is already populated and
returns -EEXIST:

drivers/base/power/common.c:dev_pm_domain_attach_list()
	if (dev->pm_domain)
		return -EEXIST;

drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c:phy_qcom_mipi_csi2_probe()
	ret = phy_qcom_mipi_csi2_attach_pm_domains(csi2phy);
	if (ret < 0 || csi2phy->pd_list == NULL) {
		if (ret == 0)
			ret = -ENODEV;
		return dev_err_probe(dev, ret, "Failed to attach power-domain list\n");
	}

Should csiphy0..csiphy4 also include the RPMHPD voltage rail power domains
(and corresponding required-opps in csiphy_opp_table) expected by the
Qualcomm CSI2 PHY binding and phy_qcom_mipi_csi2_set_clock_rates()?

[ ... ]

> +		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 {

[Severity: Low]
As reported by Sashiko on v1 [2], should csiphy_opp_table be moved out of
soc: soc@0 to the root node (alongside qspi_opp_table and qup_opp_table), and
should csiphy0..csiphy4 (phy@ace0000..phy@ace8000) be placed after
camss: isp@acb3000 to maintain unit-address ordering?

Because soc: soc@0 is a "simple-bus", placing opp-table-csiphy directly inside
it without a reg or ranges property triggers a simple_bus_reg warning in
check_simple_bus_reg() in scripts/dtc/checks.c during W=1 DTB builds as well
as a simple-bus.yaml schema validation error in dtbs_check.

[1] https://lore.kernel.org/r/20261017001116.2318647-4-anusha.nandi@oss.qualcomm.com
[2] https://lore.kernel.org/r/20261017001116.2318647-4-anusha.nandi@oss.qualcomm.com

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy