Re: [PATCH v3 7/7] arm64: dts: qcom: sm8150: Add CAMSS and CCI nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] arm64: dts: qcom: sm8150: camss endpoints omit required
  data-lanes
- [Low] arm64: dts: qcom: sm8150: camss placed before csiphy0..3 out of
  address order
--

commit c52d60a765d4e993fcd753e18db2ef2055f389e5
Author: Andrew Gigena <mail@andrewgigena.dev>

arm64: dts: qcom: sm8150: Add CAMSS and CCI nodes

This commit adds Devicetree nodes for the SM8150 CAMSS ISP, four CSI2
PHYs, and two CCI I2C controllers along with their TLMM pin
configurations.

> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index 293fcbacff346..008f787235b86 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
[ ... ]
> @@ -3782,6 +3862,414 @@ camnoc_virt: interconnect@ac00000 {
[ ... ]
> +		camss: isp@acaf000 {
> +			compatible = "qcom,sm8150-camss";
> +			status = "disabled";
[ ... ]
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					camss_csi0_in: endpoint {
> +						remote-endpoint = <&csiphy0_out>;
> +					};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +
> +					camss_csi1_in: endpoint {
> +						remote-endpoint = <&csiphy1_out>;
> +					};
> +				};
> +
> +				port@2 {
> +					reg = <2>;
> +
> +					camss_csi2_in: endpoint {
> +						remote-endpoint = <&csiphy2_out>;
> +					};
> +				};
> +
> +				port@3 {
> +					reg = <3>;
> +
> +					camss_csi3_in: endpoint {
> +						remote-endpoint = <&csiphy3_out>;
> +					};
> +				};
> +			};
> +		};

[Severity: Medium]
Should port@0 through port@3 in camss omit the pre-populated
camss_csi0_in..camss_csi3_in endpoint nodes here so that board DTS files
instantiate only the active endpoints with their data-lanes?

Documentation/devicetree/bindings/media/qcom,sm8150-camss.yaml requires
data-lanes on every endpoint under port@[0-3], so enabling camss on a
board that uses fewer than all four CSI2 PHYs leaves the unused
endpoints without data-lanes and fails dtbs_check.  At runtime, if a
board DTS only sets data-lanes on the sensor-facing csiphyN_in endpoint,
camss_parse_ports() -> camss_parse_endpoint_node() parses camss_csiN_in
without data-lanes, does not select V4L2_MBUS_CSI2_DPHY in
v4l2_fwnode_endpoint_parse(), and fails probe with -EINVAL:

drivers/media/platform/qcom/camss/camss.c:camss_parse_endpoint_node() {
    ...
	v4l2_fwnode_endpoint_parse(of_fwnode_handle(node), &vep);
	if (vep.bus_type != V4L2_MBUS_CSI2_DPHY) {
		dev_err(dev, "Unsupported bus type %d\n", vep.bus_type);
		return -EINVAL;
	}
    ...
}

> +
> +		csiphy0: phy@ac65000 {
> +			compatible = "qcom,sm8150-csi2-phy";
> +			reg = <0 0xac65000 0 0x1000>;
> +			status = "disabled";

[Severity: Low]
Should csiphy0..csiphy3 (phy@ac65000..phy@ac68000) be placed before
camss (isp@acaf000) to keep /soc@0 child nodes in ascending unit-address
order?

Because of_platform_populate() registers devices in node order, placing
isp@acaf000 before phy@ac65000..phy@ac68000 also causes camss_probe() ->
camss_init_subdevices() -> msm_csiphy_subdev_init() to run before the
PHY providers are registered when built-in, hitting -EPROBE_DEFER from
devm_phy_get_by_of_node() and logging a spurious dev_err("csiphy %d init
fail\n").  In addition, status = "disabled" in camss and
csiphy0..csiphy3 should sit after the standard device properties (before
child nodes), and the reg addresses in these nodes should be zero-padded
to 8 hex digits (such as 0x0ac4a000) to match surrounding /soc@0 nodes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-sm8150-v7.4-rc1-camss-v3-0-0103e8a0bbcd@andrewgigena.dev?part=7

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