[PATCH 0/2] media: qcom: camss: Use Glymur graph ports as CSIPHY IDs

[PATCH 0/2] media: qcom: camss: Use Glymur graph ports as CSIPHY IDs

[Nihal Kumar Gupta]

This follows up on the discussion around Glymur using standalone CSI2 PHY
nodes instead of the CAMSS CSIPHY resource table.

Glymur currently reuses the X1E80100 CAMSS resources. That works for
CSIPHY4 only because the X1E80100 resource table maps resource entry 3 to
CSIPHY id 4. After dropping the X1E80100 CSIPHY resource table, the
standalone PHY path uses the graph port number as the CSIPHY id, so
CSIPHY4 must be described as port@4.

Update the Glymur binding to allow ports 0, 1 and 4, then add
Glymur-specific CAMSS resources without reusing the X1E80100 CSIPHY
resource table.

Discussion:
https://lore.kernel.org/all/CAFEp6-23L+4+yzVxX815ZA2x1_tN8SpzL7ciNcb9b+gBT67AdQ@mail.gmail.com/

Depends-on: https://lore.kernel.org/all/20261026224957.1115370-1-vz@kernel.org/

Signed-off-by: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>
---
Nihal Kumar Gupta (2):
      dt-bindings: media: qcom,glymur-camss: Use CSIPHY indices for ports
      media: qcom: camss: Add Glymur specific resources

 .../devicetree/bindings/media/qcom,glymur-camss.yaml    |  6 +++---
 drivers/media/platform/qcom/camss/camss.c               | 17 ++++++++++++++++-
 2 files changed, 19 insertions(+), 4 deletions(-)
---
base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c
change-id: 20261102-glymur_camss-1cf83f4a447f

Best regards,
-- 
Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>

[PATCH 1/2] dt-bindings: media: qcom,glymur-camss: Use CSIPHY indices for ports

[Nihal Kumar Gupta]

Glymur describes CSI2 PHYs as standalone nodes, but the CAMSS binding
still models CSIPHY4 as port@3 from the X1E80100 csiphy_res[] layout,
where entry 3 carried CSIPHY id 4.

Describe the hardware connection directly by allowing ports 0, 1 and 4,
and using port@4 for CSIPHY4.

Signed-off-by: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/media/qcom,glymur-camss.yaml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,glymur-camss.yaml b/Documentation/devicetree/bindings/media/qcom,glymur-camss.yaml
index a0b77edc212319fad69113f3d55ca24f184d4b23..e23f43dfa4b51c1e44e57f31dc22b4312476365c 100644
--- a/Documentation/devicetree/bindings/media/qcom,glymur-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,glymur-camss.yaml
@@ -109,7 +109,7 @@ properties:
       CSI input ports.
 
     patternProperties:
-      "^port@[0-3]$":
+      "^port@[014]$":
         $ref: /schemas/graph.yaml#/$defs/port-base
         unevaluatedProperties: false
 
@@ -288,8 +288,8 @@ examples:
                     };
                 };
 
-                port@3 {
-                    reg = <3>;
+                port@4 {
+                    reg = <4>;
                     camss_csiphy4_ep: endpoint {
                         data-lanes = <1 2 3 4>;
                         remote-endpoint = <&csiphy4_out>;

-- 
2.34.1

[PATCH 2/2] media: qcom: camss: Add Glymur specific resources

[Nihal Kumar Gupta]

Glymur uses the X1E80100 CAMSS resources, but its CSI2 PHYs are
described as standalone PHY nodes. Reusing the X1E80100 resources also
reuses csiphy_res[], where port@3 maps to CSIPHY id 4.

Add Glymur resource data without reusing the X1E80100 CSIPHY resource
table, so the standalone PHY path uses the graph port number as the
CSIPHY id.

Signed-off-by: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>
---
 drivers/media/platform/qcom/camss/camss.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 1db66976e51b81edeb223cebb6bbe84995d102c2..4f705a056eedbc7b09d2630ee6284b8d768cc6d5 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -5800,6 +5800,21 @@ static void camss_remove(struct platform_device *pdev)
 	camss_genpd_cleanup(camss);
 }
 
+static const struct camss_resources glymur_resources = {
+	.version = CAMSS_X1E80100,
+	.pd_name = "top",
+	.tpg_res = tpg_res_x1e80100,
+	.csid_res = csid_res_x1e80100,
+	.vfe_res = vfe_res_x1e80100,
+	.csid_wrapper_res = &csid_wrapper_res_x1e80100,
+	.icc_res = icc_res_x1e80100,
+	.icc_path_num = ARRAY_SIZE(icc_res_x1e80100),
+	.csiphy_num = 5,
+	.tpg_num = ARRAY_SIZE(tpg_res_x1e80100),
+	.csid_num = ARRAY_SIZE(csid_res_x1e80100),
+	.vfe_num = ARRAY_SIZE(vfe_res_x1e80100),
+};
+
 static const struct camss_resources msm8916_resources = {
 	.version = CAMSS_8x16,
 	.csiphy_res = csiphy_res_8x16,
@@ -6049,6 +6064,7 @@ static const struct camss_resources x1e80100_resources = {
 };
 
 static const struct of_device_id camss_dt_match[] = {
+	{ .compatible = "qcom,glymur-camss", .data = &glymur_resources },
 	{ .compatible = "qcom,msm8916-camss", .data = &msm8916_resources },
 	{ .compatible = "qcom,msm8939-camss", .data = &msm8939_resources },
 	{ .compatible = "qcom,msm8953-camss", .data = &msm8953_resources },
@@ -6068,7 +6084,6 @@ static const struct of_device_id camss_dt_match[] = {
 	{ .compatible = "qcom,sm8550-camss", .data = &sm8550_resources },
 	{ .compatible = "qcom,sm8650-camss", .data = &sm8650_resources },
 	{ .compatible = "qcom,x1e80100-camss", .data = &x1e80100_resources },
-	{ .compatible = "qcom,glymur-camss", .data = &x1e80100_resources },
 	{ }
 };
 

-- 
2.34.1

Re: [PATCH 2/2] media: qcom: camss: Add Glymur specific resources

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: qcom: camss: NULL dereference from missing csiphy_res in glymur_resources
--

commit 8950cd65e1bb42630c5e3d1c122c6f357c2b384e
Author: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>

media: qcom: camss: Add Glymur specific resources

This commit adds a dedicated camss_resources table for Glymur so that its
standalone CSI2 PHYs do not reuse the X1E80100 CSIPHY resource table.

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index c54185c93a8a9..72541ce3a632e 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -5576,6 +5576,21 @@ static void camss_remove(struct platform_device *pdev)
>  	camss_genpd_cleanup(camss);
>  }
>  
> +static const struct camss_resources glymur_resources = {
> +	.version = CAMSS_X1E80100,
> +	.pd_name = "top",
> +	.tpg_res = tpg_res_x1e80100,
> +	.csid_res = csid_res_x1e80100,
> +	.vfe_res = vfe_res_x1e80100,
> +	.csid_wrapper_res = &csid_wrapper_res_x1e80100,
> +	.icc_res = icc_res_x1e80100,
> +	.icc_path_num = ARRAY_SIZE(icc_res_x1e80100),
> +	.csiphy_num = 5,
> +	.tpg_num = ARRAY_SIZE(tpg_res_x1e80100),
> +	.csid_num = ARRAY_SIZE(csid_res_x1e80100),
> +	.vfe_num = ARRAY_SIZE(vfe_res_x1e80100),
> +};

[Severity: High]
Does leaving csiphy_res as NULL while setting csiphy_num = 5 cause a NULL
pointer dereference when probing qcom,glymur-camss?

In the standalone PHY path, camss_probe() -> camss_init_subdevices() calls
msm_csiphy_subdev_init(camss, i) for each port, which indexes
camss->res->csiphy_res and dereferences res->csiphy.id:

drivers/media/platform/qcom/camss/camss-csiphy.c:msm_csiphy_subdev_init() {
	const struct camss_subdev_resources *res = &camss->res->csiphy_res[port];
	...
	csiphy->camss = camss;
	csiphy->id = res->csiphy.id;
	csiphy->res = &res->csiphy;
}

Even if msm_csiphy_subdev_init() failed early (for example with
-EPROBE_DEFER from devm_phy_get_by_of_node()), or if no remote PHY
endpoints are enabled and camss_detect_legacy_phy() leaves
camss->legacy_phy as true, camss_init_subdevices() also dereferences
res->csiphy_res[i].csiphy.id:

drivers/media/platform/qcom/camss/camss.c:camss_init_subdevices() {
		if (camss->legacy_phy)
			ret = msm_csiphy_subdev_init_legacy(camss, &camss->csiphy[i],
							    &res->csiphy_res[i],
							    res->csiphy_res[i].csiphy.id);
		else
			ret = msm_csiphy_subdev_init(camss, i);
		if (ret < 0) {
			dev_err(camss->dev, "csiphy %d init fail\n",
				res->csiphy_res[i].csiphy.id);
			return ret;
		}
}

Furthermore, the standalone PHY subdev operations in
drivers/media/platform/qcom/camss/camss-csiphy.c (via
msm_csiphy_register_entity() -> csiphy_init_formats() ->
csiphy_set_format() -> csiphy_try_format(), as well as
csiphy_enum_mbus_code() and csiphy_stream_on()) unconditionally dereference
csiphy->res->formats:

drivers/media/platform/qcom/camss/camss-csiphy.c:csiphy_stream_on() {
	u8 bpp = csiphy_get_bpp(csiphy->res->formats->formats, csiphy->res->formats->nformats,
				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
}

Should glymur_resources provide a csiphy_res table (or should the CSIPHY
init and format paths handle a NULL csiphy_res) so this doesn't crash at
probe or runtime?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-glymur_camss-v1-0-fcb619fc7319@oss.qualcomm.com?part=2