[PATCH 0/9] media: qcom: camss: a number of cleanups and fixes

[PATCH 0/9] media: qcom: camss: a number of cleanups and fixes

[Vladimir Zapolskiy]

The patchset prepares the ground for a CSIPHY rework in CAMSS ISP driver.

For simplicity of continuing my reviews of CAMSS paches add myself as
a reviewer, as well it will simplify the work of syncing and avoiding
patch conflicts between work oin CAMSS done by others and myself.

Vladimir Zapolskiy (9):
  media: qcom: camss: cleanup media device allocated resource on error path
  media: qcom: camss: remove duplicated csiphy_formats_sc7280 data
  media: qcom: camss: remove .link_entities
  media: qcom: camss: register camss media device before subdevices
  media: qcom: camss: unconditionally set async notifier of subdevices
  media: qcom: camss: simplify camss_subdev_notifier_complete() function
  media: qcom: camss: change internals of endpoint parsing to fwnode handling
  media: qcom: camss: use a handy v4l2_async_nf_add_fwnode_remote() function
  MAINTAINERS: add myself as a CAMSS patch reviewer

 MAINTAINERS                                   |   1 +
 .../media/platform/qcom/camss/camss-csiphy.c  |   5 -
 .../media/platform/qcom/camss/camss-csiphy.h  |   1 -
 drivers/media/platform/qcom/camss/camss.c     | 182 +++++++-----------
 drivers/media/platform/qcom/camss/camss.h     |   1 -
 5 files changed, 71 insertions(+), 119 deletions(-)

-- 
2.45.2

[PATCH 1/9] media: qcom: camss: cleanup media device allocated resource on error path

[Vladimir Zapolskiy]

A call to media_device_init() requires media_device_cleanup() counterpart
to complete cleanup and release any allocated resources.

This has been done in the driver .remove() right from the beginning, but
error paths on .probe() shall also be fixed.

Fixes: a1d7c116fcf7 ("media: camms: Add core files")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 06f42875702f..f76773dbd296 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -3625,7 +3625,7 @@ static int camss_probe(struct platform_device *pdev)
 	ret = v4l2_device_register(camss->dev, &camss->v4l2_dev);
 	if (ret < 0) {
 		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
-		goto err_genpd_cleanup;
+		goto err_media_device_cleanup;
 	}
 
 	v4l2_async_nf_init(&camss->notifier, &camss->v4l2_dev);
@@ -3680,6 +3680,8 @@ static int camss_probe(struct platform_device *pdev)
 	v4l2_device_unregister(&camss->v4l2_dev);
 	v4l2_async_nf_cleanup(&camss->notifier);
 	pm_runtime_disable(dev);
+err_media_device_cleanup:
+	media_device_cleanup(&camss->media_dev);
 err_genpd_cleanup:
 	camss_genpd_cleanup(camss);
 
-- 
2.45.2

Re: [PATCH 1/9] media: qcom: camss: cleanup media device allocated resource on error path

[Bryan O'Donoghue]

On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> A call to media_device_init() requires media_device_cleanup() counterpart
> to complete cleanup and release any allocated resources.
> 
> This has been done in the driver .remove() right from the beginning, but
> error paths on .probe() shall also be fixed.
> 
> Fixes: a1d7c116fcf7 ("media: camms: Add core files")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 06f42875702f..f76773dbd296 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -3625,7 +3625,7 @@ static int camss_probe(struct platform_device *pdev)
>   	ret = v4l2_device_register(camss->dev, &camss->v4l2_dev);
>   	if (ret < 0) {
>   		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
> -		goto err_genpd_cleanup;
> +		goto err_media_device_cleanup;
>   	}
> 
>   	v4l2_async_nf_init(&camss->notifier, &camss->v4l2_dev);
> @@ -3680,6 +3680,8 @@ static int camss_probe(struct platform_device *pdev)
>   	v4l2_device_unregister(&camss->v4l2_dev);
>   	v4l2_async_nf_cleanup(&camss->notifier);
>   	pm_runtime_disable(dev);
> +err_media_device_cleanup:
> +	media_device_cleanup(&camss->media_dev);
>   err_genpd_cleanup:
>   	camss_genpd_cleanup(camss);
> 
> --
> 2.45.2
> 
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

[PATCH 2/9] media: qcom: camss: remove duplicated csiphy_formats_sc7280 data

[Vladimir Zapolskiy]

It's sufficient to have just one previously set csiphy_formats_sdm845 data.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-csiphy.c |  5 -----
 drivers/media/platform/qcom/camss/camss-csiphy.h |  1 -
 drivers/media/platform/qcom/camss/camss.c        | 10 +++++-----
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index c622efcc92ff..2de97f58f9ae 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -103,11 +103,6 @@ const struct csiphy_formats csiphy_formats_8x96 = {
 	.formats = formats_8x96
 };
 
-const struct csiphy_formats csiphy_formats_sc7280 = {
-	.nformats = ARRAY_SIZE(formats_sdm845),
-	.formats = formats_sdm845
-};
-
 const struct csiphy_formats csiphy_formats_sdm845 = {
 	.nformats = ARRAY_SIZE(formats_sdm845),
 	.formats = formats_sdm845
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
index ab91273303b9..895f80003c44 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.h
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
@@ -126,7 +126,6 @@ void msm_csiphy_unregister_entity(struct csiphy_device *csiphy);
 
 extern const struct csiphy_formats csiphy_formats_8x16;
 extern const struct csiphy_formats csiphy_formats_8x96;
-extern const struct csiphy_formats csiphy_formats_sc7280;
 extern const struct csiphy_formats csiphy_formats_sdm845;
 
 extern const struct csiphy_hw_ops csiphy_ops_2ph_1_0;
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index f76773dbd296..8c844ebf9cb6 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1481,7 +1481,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
 		.csiphy = {
 			.id = 0,
 			.hw_ops = &csiphy_ops_3ph_1_0,
-			.formats = &csiphy_formats_sc7280
+			.formats = &csiphy_formats_sdm845,
 		}
 	},
 	/* CSIPHY1 */
@@ -1496,7 +1496,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
 		.csiphy = {
 			.id = 1,
 			.hw_ops = &csiphy_ops_3ph_1_0,
-			.formats = &csiphy_formats_sc7280
+			.formats = &csiphy_formats_sdm845,
 		}
 	},
 	/* CSIPHY2 */
@@ -1511,7 +1511,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
 		.csiphy = {
 			.id = 2,
 			.hw_ops = &csiphy_ops_3ph_1_0,
-			.formats = &csiphy_formats_sc7280
+			.formats = &csiphy_formats_sdm845,
 		}
 	},
 	/* CSIPHY3 */
@@ -1526,7 +1526,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
 		.csiphy = {
 			.id = 3,
 			.hw_ops = &csiphy_ops_3ph_1_0,
-			.formats = &csiphy_formats_sc7280
+			.formats = &csiphy_formats_sdm845,
 		}
 	},
 	/* CSIPHY4 */
@@ -1541,7 +1541,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
 		.csiphy = {
 			.id = 4,
 			.hw_ops = &csiphy_ops_3ph_1_0,
-			.formats = &csiphy_formats_sc7280
+			.formats = &csiphy_formats_sdm845,
 		}
 	},
 };
-- 
2.45.2

Re: [PATCH 2/9] media: qcom: camss: remove duplicated csiphy_formats_sc7280 data

[Bryan O'Donoghue]

On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> It's sufficient to have just one previously set csiphy_formats_sdm845 data.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss-csiphy.c |  5 -----
>   drivers/media/platform/qcom/camss/camss-csiphy.h |  1 -
>   drivers/media/platform/qcom/camss/camss.c        | 10 +++++-----
>   3 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index c622efcc92ff..2de97f58f9ae 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -103,11 +103,6 @@ const struct csiphy_formats csiphy_formats_8x96 = {
>   	.formats = formats_8x96
>   };
>   
> -const struct csiphy_formats csiphy_formats_sc7280 = {
> -	.nformats = ARRAY_SIZE(formats_sdm845),
> -	.formats = formats_sdm845
> -};
> -
>   const struct csiphy_formats csiphy_formats_sdm845 = {
>   	.nformats = ARRAY_SIZE(formats_sdm845),
>   	.formats = formats_sdm845
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
> index ab91273303b9..895f80003c44 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
> @@ -126,7 +126,6 @@ void msm_csiphy_unregister_entity(struct csiphy_device *csiphy);
>   
>   extern const struct csiphy_formats csiphy_formats_8x16;
>   extern const struct csiphy_formats csiphy_formats_8x96;
> -extern const struct csiphy_formats csiphy_formats_sc7280;
>   extern const struct csiphy_formats csiphy_formats_sdm845;
>   
>   extern const struct csiphy_hw_ops csiphy_ops_2ph_1_0;
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index f76773dbd296..8c844ebf9cb6 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1481,7 +1481,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
>   		.csiphy = {
>   			.id = 0,
>   			.hw_ops = &csiphy_ops_3ph_1_0,
> -			.formats = &csiphy_formats_sc7280
> +			.formats = &csiphy_formats_sdm845,
>   		}
>   	},
>   	/* CSIPHY1 */
> @@ -1496,7 +1496,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
>   		.csiphy = {
>   			.id = 1,
>   			.hw_ops = &csiphy_ops_3ph_1_0,
> -			.formats = &csiphy_formats_sc7280
> +			.formats = &csiphy_formats_sdm845,
>   		}
>   	},
>   	/* CSIPHY2 */
> @@ -1511,7 +1511,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
>   		.csiphy = {
>   			.id = 2,
>   			.hw_ops = &csiphy_ops_3ph_1_0,
> -			.formats = &csiphy_formats_sc7280
> +			.formats = &csiphy_formats_sdm845,
>   		}
>   	},
>   	/* CSIPHY3 */
> @@ -1526,7 +1526,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
>   		.csiphy = {
>   			.id = 3,
>   			.hw_ops = &csiphy_ops_3ph_1_0,
> -			.formats = &csiphy_formats_sc7280
> +			.formats = &csiphy_formats_sdm845,
>   		}
>   	},
>   	/* CSIPHY4 */
> @@ -1541,7 +1541,7 @@ static const struct camss_subdev_resources csiphy_res_7280[] = {
>   		.csiphy = {
>   			.id = 4,
>   			.hw_ops = &csiphy_ops_3ph_1_0,
> -			.formats = &csiphy_formats_sc7280
> +			.formats = &csiphy_formats_sdm845,
>   		}
>   	},
>   };

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

[PATCH 3/9] media: qcom: camss: remove .link_entities

[Vladimir Zapolskiy]

There is no potential for a custom .link_entities callback,
remove it by replacing with a common camss_link_entities().

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 14 +-------------
 drivers/media/platform/qcom/camss/camss.h |  1 -
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 8c844ebf9cb6..2977aeaf27e1 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -3143,7 +3143,6 @@ static int camss_init_subdevices(struct camss *camss)
 }
 
 /*
- * camss_link_entities - Register subdev nodes and create links
  * camss_link_err - print error in case link creation fails
  * @src_name: name for source of the link
  * @sink_name: name for sink of the link
@@ -3642,7 +3641,7 @@ static int camss_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_v4l2_device_unregister;
 
-	ret = camss->res->link_entities(camss);
+	ret = camss_link_entities(camss);
 	if (ret < 0)
 		goto err_register_subdevs;
 
@@ -3726,7 +3725,6 @@ static const struct camss_resources msm8916_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_8x16),
 	.csid_num = ARRAY_SIZE(csid_res_8x16),
 	.vfe_num = ARRAY_SIZE(vfe_res_8x16),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources msm8953_resources = {
@@ -3740,7 +3738,6 @@ static const struct camss_resources msm8953_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_8x96),
 	.csid_num = ARRAY_SIZE(csid_res_8x53),
 	.vfe_num = ARRAY_SIZE(vfe_res_8x53),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources msm8996_resources = {
@@ -3752,7 +3749,6 @@ static const struct camss_resources msm8996_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_8x96),
 	.csid_num = ARRAY_SIZE(csid_res_8x96),
 	.vfe_num = ARRAY_SIZE(vfe_res_8x96),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources sdm660_resources = {
@@ -3764,7 +3760,6 @@ static const struct camss_resources sdm660_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_660),
 	.csid_num = ARRAY_SIZE(csid_res_660),
 	.vfe_num = ARRAY_SIZE(vfe_res_660),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources sdm670_resources = {
@@ -3775,7 +3770,6 @@ static const struct camss_resources sdm670_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_670),
 	.csid_num = ARRAY_SIZE(csid_res_670),
 	.vfe_num = ARRAY_SIZE(vfe_res_670),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources sdm845_resources = {
@@ -3787,7 +3781,6 @@ static const struct camss_resources sdm845_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_845),
 	.csid_num = ARRAY_SIZE(csid_res_845),
 	.vfe_num = ARRAY_SIZE(vfe_res_845),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources sm8250_resources = {
@@ -3801,7 +3794,6 @@ static const struct camss_resources sm8250_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_8250),
 	.csid_num = ARRAY_SIZE(csid_res_8250),
 	.vfe_num = ARRAY_SIZE(vfe_res_8250),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources sc8280xp_resources = {
@@ -3816,7 +3808,6 @@ static const struct camss_resources sc8280xp_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_sc8280xp),
 	.csid_num = ARRAY_SIZE(csid_res_sc8280xp),
 	.vfe_num = ARRAY_SIZE(vfe_res_sc8280xp),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources sc7280_resources = {
@@ -3830,7 +3821,6 @@ static const struct camss_resources sc7280_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_7280),
 	.csid_num = ARRAY_SIZE(csid_res_7280),
 	.vfe_num = ARRAY_SIZE(vfe_res_7280),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources sm8550_resources = {
@@ -3845,7 +3835,6 @@ static const struct camss_resources sm8550_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_8550),
 	.csid_num = ARRAY_SIZE(csid_res_8550),
 	.vfe_num = ARRAY_SIZE(vfe_res_8550),
-	.link_entities = camss_link_entities
 };
 
 static const struct camss_resources x1e80100_resources = {
@@ -3860,7 +3849,6 @@ static const struct camss_resources x1e80100_resources = {
 	.csiphy_num = ARRAY_SIZE(csiphy_res_x1e80100),
 	.csid_num = ARRAY_SIZE(csid_res_x1e80100),
 	.vfe_num = ARRAY_SIZE(vfe_res_x1e80100),
-	.link_entities = camss_link_entities
 };
 
 static const struct of_device_id camss_dt_match[] = {
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index 63c0afee154a..1d0f83e4a2c9 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -107,7 +107,6 @@ struct camss_resources {
 	const unsigned int csiphy_num;
 	const unsigned int csid_num;
 	const unsigned int vfe_num;
-	int (*link_entities)(struct camss *camss);
 };
 
 struct camss {
-- 
2.45.2

Re: [PATCH 3/9] media: qcom: camss: remove .link_entities

[Bryan O'Donoghue]

On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> There is no potential for a custom .link_entities callback,
> remove it by replacing with a common camss_link_entities().
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 14 +-------------
>   drivers/media/platform/qcom/camss/camss.h |  1 -
>   2 files changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 8c844ebf9cb6..2977aeaf27e1 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -3143,7 +3143,6 @@ static int camss_init_subdevices(struct camss *camss)
>   }
>   
>   /*
> - * camss_link_entities - Register subdev nodes and create links
>    * camss_link_err - print error in case link creation fails
>    * @src_name: name for source of the link
>    * @sink_name: name for sink of the link
> @@ -3642,7 +3641,7 @@ static int camss_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		goto err_v4l2_device_unregister;
>   
> -	ret = camss->res->link_entities(camss);
> +	ret = camss_link_entities(camss);
>   	if (ret < 0)
>   		goto err_register_subdevs;
>   
> @@ -3726,7 +3725,6 @@ static const struct camss_resources msm8916_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_8x16),
>   	.csid_num = ARRAY_SIZE(csid_res_8x16),
>   	.vfe_num = ARRAY_SIZE(vfe_res_8x16),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources msm8953_resources = {
> @@ -3740,7 +3738,6 @@ static const struct camss_resources msm8953_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_8x96),
>   	.csid_num = ARRAY_SIZE(csid_res_8x53),
>   	.vfe_num = ARRAY_SIZE(vfe_res_8x53),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources msm8996_resources = {
> @@ -3752,7 +3749,6 @@ static const struct camss_resources msm8996_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_8x96),
>   	.csid_num = ARRAY_SIZE(csid_res_8x96),
>   	.vfe_num = ARRAY_SIZE(vfe_res_8x96),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources sdm660_resources = {
> @@ -3764,7 +3760,6 @@ static const struct camss_resources sdm660_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_660),
>   	.csid_num = ARRAY_SIZE(csid_res_660),
>   	.vfe_num = ARRAY_SIZE(vfe_res_660),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources sdm670_resources = {
> @@ -3775,7 +3770,6 @@ static const struct camss_resources sdm670_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_670),
>   	.csid_num = ARRAY_SIZE(csid_res_670),
>   	.vfe_num = ARRAY_SIZE(vfe_res_670),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources sdm845_resources = {
> @@ -3787,7 +3781,6 @@ static const struct camss_resources sdm845_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_845),
>   	.csid_num = ARRAY_SIZE(csid_res_845),
>   	.vfe_num = ARRAY_SIZE(vfe_res_845),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources sm8250_resources = {
> @@ -3801,7 +3794,6 @@ static const struct camss_resources sm8250_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_8250),
>   	.csid_num = ARRAY_SIZE(csid_res_8250),
>   	.vfe_num = ARRAY_SIZE(vfe_res_8250),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources sc8280xp_resources = {
> @@ -3816,7 +3808,6 @@ static const struct camss_resources sc8280xp_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_sc8280xp),
>   	.csid_num = ARRAY_SIZE(csid_res_sc8280xp),
>   	.vfe_num = ARRAY_SIZE(vfe_res_sc8280xp),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources sc7280_resources = {
> @@ -3830,7 +3821,6 @@ static const struct camss_resources sc7280_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_7280),
>   	.csid_num = ARRAY_SIZE(csid_res_7280),
>   	.vfe_num = ARRAY_SIZE(vfe_res_7280),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources sm8550_resources = {
> @@ -3845,7 +3835,6 @@ static const struct camss_resources sm8550_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_8550),
>   	.csid_num = ARRAY_SIZE(csid_res_8550),
>   	.vfe_num = ARRAY_SIZE(vfe_res_8550),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct camss_resources x1e80100_resources = {
> @@ -3860,7 +3849,6 @@ static const struct camss_resources x1e80100_resources = {
>   	.csiphy_num = ARRAY_SIZE(csiphy_res_x1e80100),
>   	.csid_num = ARRAY_SIZE(csid_res_x1e80100),
>   	.vfe_num = ARRAY_SIZE(vfe_res_x1e80100),
> -	.link_entities = camss_link_entities
>   };
>   
>   static const struct of_device_id camss_dt_match[] = {
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 63c0afee154a..1d0f83e4a2c9 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -107,7 +107,6 @@ struct camss_resources {
>   	const unsigned int csiphy_num;
>   	const unsigned int csid_num;
>   	const unsigned int vfe_num;
> -	int (*link_entities)(struct camss *camss);
>   };
>   
>   struct camss {

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

[PATCH 4/9] media: qcom: camss: register camss media device before subdevices

[Vladimir Zapolskiy]

A media device can and at least for sake of simplicity should be registered
before V4L2 devices including the ones added on async completion.

The change removes the second and out of camss_probe() media device
registration path, and it allows to get a working ISP media device
independently from connected or not sensor devices.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 25 ++++++++++-------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 2977aeaf27e1..976b70cc6d6a 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -3417,11 +3417,7 @@ static int camss_subdev_notifier_complete(struct v4l2_async_notifier *async)
 		}
 	}
 
-	ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
-	if (ret < 0)
-		return ret;
-
-	return media_device_register(&camss->media_dev);
+	return v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
 }
 
 static const struct v4l2_async_notifier_operations camss_subdev_notifier_ops = {
@@ -3645,6 +3641,12 @@ static int camss_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_register_subdevs;
 
+	ret = media_device_register(&camss->media_dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register media device: %d\n", ret);
+		goto err_register_subdevs;
+	}
+
 	if (num_subdevs) {
 		camss->notifier.ops = &camss_subdev_notifier_ops;
 
@@ -3653,26 +3655,21 @@ static int camss_probe(struct platform_device *pdev)
 			dev_err(dev,
 				"Failed to register async subdev nodes: %d\n",
 				ret);
-			goto err_register_subdevs;
+			goto err_media_device_unregister;
 		}
 	} else {
 		ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
 		if (ret < 0) {
 			dev_err(dev, "Failed to register subdev nodes: %d\n",
 				ret);
-			goto err_register_subdevs;
-		}
-
-		ret = media_device_register(&camss->media_dev);
-		if (ret < 0) {
-			dev_err(dev, "Failed to register media device: %d\n",
-				ret);
-			goto err_register_subdevs;
+			goto err_media_device_unregister;
 		}
 	}
 
 	return 0;
 
+err_media_device_unregister:
+	media_device_unregister(&camss->media_dev);
 err_register_subdevs:
 	camss_unregister_entities(camss);
 err_v4l2_device_unregister:
-- 
2.45.2

Re: [PATCH 4/9] media: qcom: camss: register camss media device before subdevices

[Bryan O'Donoghue]

On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> A media device can and at least for sake of simplicity should be registered
> before V4L2 devices including the ones added on async completion.
> 
> The change removes the second and out of camss_probe() media device
> registration path, and it allows to get a working ISP media device
> independently from connected or not sensor devices.

So are you proposing this change to simply the code or for this 
secondary reason ?

---
bod

Re: [PATCH 4/9] media: qcom: camss: register camss media device before subdevices

[Vladimir Zapolskiy]

Hello Bryan.

On 6/13/25 12:06, Bryan O'Donoghue wrote:
> On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
>> A media device can and at least for sake of simplicity should be registered
>> before V4L2 devices including the ones added on async completion.
>>
>> The change removes the second and out of camss_probe() media device
>> registration path, and it allows to get a working ISP media device
>> independently from connected or not sensor devices.
> 
> So are you proposing this change to simply the code or for this
> secondary reason ?
> 

This change halves the execution branches with media_device_register()
usage, this is a nice simplification by itself, isn't it?

Please let me know your worries about the change, if there are any.

--
Best wishes,
Vladimir

Re: [PATCH 4/9] media: qcom: camss: register camss media device before subdevices

[neil.armstrong]

On 17/06/2025 18:14, Vladimir Zapolskiy wrote:
> Hello Bryan.
> 
> On 6/13/25 12:06, Bryan O'Donoghue wrote:
>> On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
>>> A media device can and at least for sake of simplicity should be registered
>>> before V4L2 devices including the ones added on async completion.
>>>
>>> The change removes the second and out of camss_probe() media device
>>> registration path, and it allows to get a working ISP media device
>>> independently from connected or not sensor devices.
>>
>> So are you proposing this change to simply the code or for this
>> secondary reason ?
>>
> 
> This change halves the execution branches with media_device_register()
> usage, this is a nice simplification by itself, isn't it?
> 
> Please let me know your worries about the change, if there are any.

The change is pretty logical and well explained in the commit message,
it make the CAMSS media device functional even without any sub devices.

Which is great for unitary validating the CAMSS on platforms.

I'll add my:
Acked-by: Neil Armstrong <neil.armstrong@linaro.org>

Thanks,
Neil

> 
> -- 
> Best wishes,
> Vladimir
> 

[PATCH 5/9] media: qcom: camss: unconditionally set async notifier of subdevices

[Vladimir Zapolskiy]

For sake of simplicity it makes sense to register async notifier
for all type of subdevices, both CAMSS components and sensors.

The case of sensors not connected to CAMSS is extraordinary and
degenerate, it does not deserve any specific optimization.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 30 ++++++-----------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 976b70cc6d6a..4e91e4b6ef52 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct camss *camss;
-	int num_subdevs;
 	int ret;
 
 	camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL);
@@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
-	num_subdevs = camss_of_parse_ports(camss);
-	if (num_subdevs < 0) {
-		ret = num_subdevs;
+	ret = camss_of_parse_ports(camss);
+	if (ret < 0)
 		goto err_v4l2_device_unregister;
-	}
 
 	ret = camss_register_entities(camss);
 	if (ret < 0)
@@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev)
 		goto err_register_subdevs;
 	}
 
-	if (num_subdevs) {
-		camss->notifier.ops = &camss_subdev_notifier_ops;
-
-		ret = v4l2_async_nf_register(&camss->notifier);
-		if (ret) {
-			dev_err(dev,
-				"Failed to register async subdev nodes: %d\n",
-				ret);
-			goto err_media_device_unregister;
-		}
-	} else {
-		ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
-		if (ret < 0) {
-			dev_err(dev, "Failed to register subdev nodes: %d\n",
-				ret);
-			goto err_media_device_unregister;
-		}
+	camss->notifier.ops = &camss_subdev_notifier_ops;
+	ret = v4l2_async_nf_register(&camss->notifier);
+	if (ret) {
+		dev_err(dev,
+			"Failed to register async subdev nodes: %d\n", ret);
+		goto err_media_device_unregister;
 	}
 
 	return 0;
-- 
2.45.2

Re: [PATCH 5/9] media: qcom: camss: unconditionally set async notifier of subdevices

[Bryan O'Donoghue]

On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> For sake of simplicity it makes sense to register async notifier
> for all type of subdevices, both CAMSS components and sensors.
> 
> The case of sensors not connected to CAMSS is extraordinary and
> degenerate, it does not deserve any specific optimization.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 30 ++++++-----------------
>   1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 976b70cc6d6a..4e91e4b6ef52 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct camss *camss;
> -	int num_subdevs;
>   	int ret;
>   
>   	camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL);
> @@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev)
>   
>   	pm_runtime_enable(dev);
>   
> -	num_subdevs = camss_of_parse_ports(camss);
> -	if (num_subdevs < 0) {
> -		ret = num_subdevs;
> +	ret = camss_of_parse_ports(camss);
> +	if (ret < 0)
>   		goto err_v4l2_device_unregister;
> -	}
>   
>   	ret = camss_register_entities(camss);
>   	if (ret < 0)
> @@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev)
>   		goto err_register_subdevs;
>   	}
>   
> -	if (num_subdevs) {
> -		camss->notifier.ops = &camss_subdev_notifier_ops;
> -
> -		ret = v4l2_async_nf_register(&camss->notifier);
> -		if (ret) {
> -			dev_err(dev,
> -				"Failed to register async subdev nodes: %d\n",
> -				ret);
> -			goto err_media_device_unregister;
> -		}
> -	} else {
> -		ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
> -		if (ret < 0) {
> -			dev_err(dev, "Failed to register subdev nodes: %d\n",
> -				ret);
> -			goto err_media_device_unregister;
> -		}
> +	camss->notifier.ops = &camss_subdev_notifier_ops;
> +	ret = v4l2_async_nf_register(&camss->notifier);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to register async subdev nodes: %d\n", ret);
> +		goto err_media_device_unregister;
>   	}
>   
>   	return 0;

If I've understood the intent here, don't think this is right.

For cases where we want to run CSID TPG or standalone TPG we would not 
necessarily have a sensor connected.

---
bod

Re: [PATCH 5/9] media: qcom: camss: unconditionally set async notifier of subdevices

[Vladimir Zapolskiy]

Hi Bryan.

On 5/13/25 18:46, Bryan O'Donoghue wrote:
> On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
>> For sake of simplicity it makes sense to register async notifier
>> for all type of subdevices, both CAMSS components and sensors.
>>
>> The case of sensors not connected to CAMSS is extraordinary and
>> degenerate, it does not deserve any specific optimization.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> ---
>>    drivers/media/platform/qcom/camss/camss.c | 30 ++++++-----------------
>>    1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 976b70cc6d6a..4e91e4b6ef52 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev)
>>    {
>>    	struct device *dev = &pdev->dev;
>>    	struct camss *camss;
>> -	int num_subdevs;
>>    	int ret;
>>    
>>    	camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL);
>> @@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev)
>>    
>>    	pm_runtime_enable(dev);
>>    
>> -	num_subdevs = camss_of_parse_ports(camss);
>> -	if (num_subdevs < 0) {
>> -		ret = num_subdevs;
>> +	ret = camss_of_parse_ports(camss);
>> +	if (ret < 0)
>>    		goto err_v4l2_device_unregister;
>> -	}
>>    
>>    	ret = camss_register_entities(camss);
>>    	if (ret < 0)
>> @@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev)
>>    		goto err_register_subdevs;
>>    	}
>>    
>> -	if (num_subdevs) {
>> -		camss->notifier.ops = &camss_subdev_notifier_ops;
>> -
>> -		ret = v4l2_async_nf_register(&camss->notifier);
>> -		if (ret) {
>> -			dev_err(dev,
>> -				"Failed to register async subdev nodes: %d\n",
>> -				ret);
>> -			goto err_media_device_unregister;
>> -		}
>> -	} else {
>> -		ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
>> -		if (ret < 0) {
>> -			dev_err(dev, "Failed to register subdev nodes: %d\n",
>> -				ret);
>> -			goto err_media_device_unregister;
>> -		}
>> +	camss->notifier.ops = &camss_subdev_notifier_ops;
>> +	ret = v4l2_async_nf_register(&camss->notifier);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to register async subdev nodes: %d\n", ret);
>> +		goto err_media_device_unregister;
>>    	}
>>    
>>    	return 0;
> 
> If I've understood the intent here, don't think this is right.

Please elaborate, so far it's not completely clear.

> For cases where we want to run CSID TPG or standalone TPG we would not
> necessarily have a sensor connected.
> 

That's correct, and this is not broken by any means.

As you mention it there might be so many usecases, but fortunately all of them
are covered by the code, which is noticeably simpler than the original one, and
which opens the path for even further code optimization and simplification, as
you find it in this changeset.

--
Best wishes,
Vladimir

Re: [PATCH 5/9] media: qcom: camss: unconditionally set async notifier of subdevices

[Loic Poulain]

On Tue, May 13, 2025 at 5:47 PM Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> > For sake of simplicity it makes sense to register async notifier
> > for all type of subdevices, both CAMSS components and sensors.
> >
> > The case of sensors not connected to CAMSS is extraordinary and
> > degenerate, it does not deserve any specific optimization.
> >
> > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> > ---
> >   drivers/media/platform/qcom/camss/camss.c | 30 ++++++-----------------
> >   1 file changed, 8 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> > index 976b70cc6d6a..4e91e4b6ef52 100644
> > --- a/drivers/media/platform/qcom/camss/camss.c
> > +++ b/drivers/media/platform/qcom/camss/camss.c
> > @@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev)
> >   {
> >       struct device *dev = &pdev->dev;
> >       struct camss *camss;
> > -     int num_subdevs;
> >       int ret;
> >
> >       camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL);
> > @@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev)
> >
> >       pm_runtime_enable(dev);
> >
> > -     num_subdevs = camss_of_parse_ports(camss);
> > -     if (num_subdevs < 0) {
> > -             ret = num_subdevs;
> > +     ret = camss_of_parse_ports(camss);
> > +     if (ret < 0)
> >               goto err_v4l2_device_unregister;
> > -     }
> >
> >       ret = camss_register_entities(camss);
> >       if (ret < 0)
> > @@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev)
> >               goto err_register_subdevs;
> >       }
> >
> > -     if (num_subdevs) {
> > -             camss->notifier.ops = &camss_subdev_notifier_ops;
> > -
> > -             ret = v4l2_async_nf_register(&camss->notifier);
> > -             if (ret) {
> > -                     dev_err(dev,
> > -                             "Failed to register async subdev nodes: %d\n",
> > -                             ret);
> > -                     goto err_media_device_unregister;
> > -             }
> > -     } else {
> > -             ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
> > -             if (ret < 0) {
> > -                     dev_err(dev, "Failed to register subdev nodes: %d\n",
> > -                             ret);
> > -                     goto err_media_device_unregister;
> > -             }
> > +     camss->notifier.ops = &camss_subdev_notifier_ops;
> > +     ret = v4l2_async_nf_register(&camss->notifier);
> > +     if (ret) {
> > +             dev_err(dev,
> > +                     "Failed to register async subdev nodes: %d\n", ret);
> > +             goto err_media_device_unregister;
> >       }
> >
> >       return 0;
>
> If I've understood the intent here, don't think this is right.
>
> For cases where we want to run CSID TPG or standalone TPG we would not
> necessarily have a sensor connected.

I understand it will work because Vladimir moved the media device
registering earlier in the probe, so the media pipeline will be ready,
even if no subdev sensor has been registered.

Regards,
Loic

Re: [PATCH 5/9] media: qcom: camss: unconditionally set async notifier of subdevices

[Bryan O'Donoghue]

On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> For sake of simplicity it makes sense to register async notifier
> for all type of subdevices, both CAMSS components and sensors.
> 
> The case of sensors not connected to CAMSS is extraordinary and
> degenerate, it does not deserve any specific optimization.

Degenerate is an odd word to use.
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 30 ++++++-----------------
>   1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 976b70cc6d6a..4e91e4b6ef52 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct camss *camss;
> -	int num_subdevs;
>   	int ret;
> 
>   	camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL);
> @@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev)
> 
>   	pm_runtime_enable(dev);
> 
> -	num_subdevs = camss_of_parse_ports(camss);
> -	if (num_subdevs < 0) {
> -		ret = num_subdevs;
> +	ret = camss_of_parse_ports(camss);
> +	if (ret < 0)
>   		goto err_v4l2_device_unregister;
> -	}
> 
>   	ret = camss_register_entities(camss);
>   	if (ret < 0)
> @@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev)
>   		goto err_register_subdevs;
>   	}
> 
> -	if (num_subdevs) {
> -		camss->notifier.ops = &camss_subdev_notifier_ops;
> -
> -		ret = v4l2_async_nf_register(&camss->notifier);
> -		if (ret) {
> -			dev_err(dev,
> -				"Failed to register async subdev nodes: %d\n",
> -				ret);
> -			goto err_media_device_unregister;
> -		}
> -	} else {
> -		ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
> -		if (ret < 0) {
> -			dev_err(dev, "Failed to register subdev nodes: %d\n",
> -				ret);
> -			goto err_media_device_unregister;
> -		}
> +	camss->notifier.ops = &camss_subdev_notifier_ops;
> +	ret = v4l2_async_nf_register(&camss->notifier);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to register async subdev nodes: %d\n", ret);
> +		goto err_media_device_unregister;
>   	}
I'm a little concerned about changing the current flow. Have you tested 
this out without sensors attached, the TPG on rb5 for example ?

---
bod

Re: [PATCH 5/9] media: qcom: camss: unconditionally set async notifier of subdevices

[Vladimir Zapolskiy]

Hi Bryan.

On 6/13/25 12:11, Bryan O'Donoghue wrote:
> On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
>> For sake of simplicity it makes sense to register async notifier
>> for all type of subdevices, both CAMSS components and sensors.
>>
>> The case of sensors not connected to CAMSS is extraordinary and
>> degenerate, it does not deserve any specific optimization.
> 
> Degenerate is an odd word to use.

Well, here the wording "degenerate case" is a direct borrowing from a
mathematical term "degenerate case" with no intended change of meaning:

https://en.wikipedia.org/wiki/Degeneracy_(mathematics)

>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> ---
>>    drivers/media/platform/qcom/camss/camss.c | 30 ++++++-----------------
>>    1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 976b70cc6d6a..4e91e4b6ef52 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev)
>>    {
>>    	struct device *dev = &pdev->dev;
>>    	struct camss *camss;
>> -	int num_subdevs;
>>    	int ret;
>>
>>    	camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL);
>> @@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev)
>>
>>    	pm_runtime_enable(dev);
>>
>> -	num_subdevs = camss_of_parse_ports(camss);
>> -	if (num_subdevs < 0) {
>> -		ret = num_subdevs;
>> +	ret = camss_of_parse_ports(camss);
>> +	if (ret < 0)
>>    		goto err_v4l2_device_unregister;
>> -	}
>>
>>    	ret = camss_register_entities(camss);
>>    	if (ret < 0)
>> @@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev)
>>    		goto err_register_subdevs;
>>    	}
>>
>> -	if (num_subdevs) {
>> -		camss->notifier.ops = &camss_subdev_notifier_ops;
>> -
>> -		ret = v4l2_async_nf_register(&camss->notifier);
>> -		if (ret) {
>> -			dev_err(dev,
>> -				"Failed to register async subdev nodes: %d\n",
>> -				ret);
>> -			goto err_media_device_unregister;
>> -		}
>> -	} else {
>> -		ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
>> -		if (ret < 0) {
>> -			dev_err(dev, "Failed to register subdev nodes: %d\n",
>> -				ret);
>> -			goto err_media_device_unregister;
>> -		}
>> +	camss->notifier.ops = &camss_subdev_notifier_ops;
>> +	ret = v4l2_async_nf_register(&camss->notifier);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to register async subdev nodes: %d\n", ret);
>> +		goto err_media_device_unregister;
>>    	}
> I'm a little concerned about changing the current flow. Have you tested
> this out without sensors attached, the TPG on rb5 for example ?

Yes, I have tested this out with the TPG on RB5 by removing a sensor
description from the RB5 vision mezzanine, I don't find any regression.

Please let me know, if you find any issues with the change.

--
Best wishes,
Vladimir

Re: [PATCH 5/9] media: qcom: camss: unconditionally set async notifier of subdevices

[neil.armstrong]

On 13/05/2025 16:23, Vladimir Zapolskiy wrote:
> For sake of simplicity it makes sense to register async notifier
> for all type of subdevices, both CAMSS components and sensors.
> 
> The case of sensors not connected to CAMSS is extraordinary and
> degenerate, it does not deserve any specific optimization.

Could you describe the difference on behavior in this case ? and how
it's still valid ?

Neil

> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 30 ++++++-----------------
>   1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 976b70cc6d6a..4e91e4b6ef52 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct camss *camss;
> -	int num_subdevs;
>   	int ret;
>   
>   	camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL);
> @@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev)
>   
>   	pm_runtime_enable(dev);
>   
> -	num_subdevs = camss_of_parse_ports(camss);
> -	if (num_subdevs < 0) {
> -		ret = num_subdevs;
> +	ret = camss_of_parse_ports(camss);
> +	if (ret < 0)
>   		goto err_v4l2_device_unregister;
> -	}
>   
>   	ret = camss_register_entities(camss);
>   	if (ret < 0)
> @@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev)
>   		goto err_register_subdevs;
>   	}
>   
> -	if (num_subdevs) {
> -		camss->notifier.ops = &camss_subdev_notifier_ops;
> -
> -		ret = v4l2_async_nf_register(&camss->notifier);
> -		if (ret) {
> -			dev_err(dev,
> -				"Failed to register async subdev nodes: %d\n",
> -				ret);
> -			goto err_media_device_unregister;
> -		}
> -	} else {
> -		ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev);
> -		if (ret < 0) {
> -			dev_err(dev, "Failed to register subdev nodes: %d\n",
> -				ret);
> -			goto err_media_device_unregister;
> -		}
> +	camss->notifier.ops = &camss_subdev_notifier_ops;
> +	ret = v4l2_async_nf_register(&camss->notifier);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to register async subdev nodes: %d\n", ret);
> +		goto err_media_device_unregister;
>   	}
>   
>   	return 0;

[PATCH 6/9] media: qcom: camss: simplify camss_subdev_notifier_complete() function

[Vladimir Zapolskiy]

For sake of code simplicity and readability reduce the function code by
one level of indentation, the change is non-functional.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 50 +++++++++++------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 4e91e4b6ef52..39c5472f4552 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -3385,35 +3385,35 @@ static int camss_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	struct camss *camss = container_of(async, struct camss, notifier);
 	struct v4l2_device *v4l2_dev = &camss->v4l2_dev;
 	struct v4l2_subdev *sd;
-	int ret;
 
 	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		if (sd->host_priv) {
-			struct media_entity *sensor = &sd->entity;
-			struct csiphy_device *csiphy =
-					(struct csiphy_device *) sd->host_priv;
-			struct media_entity *input = &csiphy->subdev.entity;
-			unsigned int i;
-
-			for (i = 0; i < sensor->num_pads; i++) {
-				if (sensor->pads[i].flags & MEDIA_PAD_FL_SOURCE)
-					break;
-			}
-			if (i == sensor->num_pads) {
-				dev_err(camss->dev,
-					"No source pad in external entity\n");
-				return -EINVAL;
-			}
+		struct csiphy_device *csiphy = sd->host_priv;
+		struct media_entity *input, *sensor;
+		unsigned int i;
+		int ret;
+
+		if (!csiphy)
+			continue;
+
+		input = &csiphy->subdev.entity;
+		sensor = &sd->entity;
+
+		for (i = 0; i < sensor->num_pads; i++) {
+			if (sensor->pads[i].flags & MEDIA_PAD_FL_SOURCE)
+				break;
+		}
+		if (i == sensor->num_pads) {
+			dev_err(camss->dev,
+				"No source pad in external entity\n");
+			return -EINVAL;
+		}
 
-			ret = media_create_pad_link(sensor, i,
-				input, MSM_CSIPHY_PAD_SINK,
+		ret = media_create_pad_link(sensor, i, input,
+				MSM_CSIPHY_PAD_SINK,
 				MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
-			if (ret < 0) {
-				camss_link_err(camss, sensor->name,
-					       input->name,
-					       ret);
-				return ret;
-			}
+		if (ret < 0) {
+			camss_link_err(camss, sensor->name, input->name, ret);
+			return ret;
 		}
 	}
 
-- 
2.45.2

Re: [PATCH 6/9] media: qcom: camss: simplify camss_subdev_notifier_complete() function

[Bryan O'Donoghue]

On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> For sake of code simplicity and readability reduce the function code by
> one level of indentation, the change is non-functional.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 50 +++++++++++------------
>   1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 4e91e4b6ef52..39c5472f4552 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -3385,35 +3385,35 @@ static int camss_subdev_notifier_complete(struct v4l2_async_notifier *async)
>   	struct camss *camss = container_of(async, struct camss, notifier);
>   	struct v4l2_device *v4l2_dev = &camss->v4l2_dev;
>   	struct v4l2_subdev *sd;
> -	int ret;
> 
>   	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> -		if (sd->host_priv) {
> -			struct media_entity *sensor = &sd->entity;
> -			struct csiphy_device *csiphy =
> -					(struct csiphy_device *) sd->host_priv;
> -			struct media_entity *input = &csiphy->subdev.entity;
> -			unsigned int i;
> -
> -			for (i = 0; i < sensor->num_pads; i++) {
> -				if (sensor->pads[i].flags & MEDIA_PAD_FL_SOURCE)
> -					break;
> -			}
> -			if (i == sensor->num_pads) {
> -				dev_err(camss->dev,
> -					"No source pad in external entity\n");
> -				return -EINVAL;
> -			}
> +		struct csiphy_device *csiphy = sd->host_priv;
> +		struct media_entity *input, *sensor;
> +		unsigned int i;
> +		int ret;
> +
> +		if (!csiphy)
> +			continue;
> +
> +		input = &csiphy->subdev.entity;
> +		sensor = &sd->entity;
> +
> +		for (i = 0; i < sensor->num_pads; i++) {
> +			if (sensor->pads[i].flags & MEDIA_PAD_FL_SOURCE)
> +				break;
> +		}
> +		if (i == sensor->num_pads) {
> +			dev_err(camss->dev,
> +				"No source pad in external entity\n");
> +			return -EINVAL;
> +		}
> 
> -			ret = media_create_pad_link(sensor, i,
> -				input, MSM_CSIPHY_PAD_SINK,
> +		ret = media_create_pad_link(sensor, i, input,
> +				MSM_CSIPHY_PAD_SINK,
>   				MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> -			if (ret < 0) {
> -				camss_link_err(camss, sensor->name,
> -					       input->name,
> -					       ret);
> -				return ret;
> -			}
> +		if (ret < 0) {
> +			camss_link_err(camss, sensor->name, input->name, ret);
> +			return ret;
>   		}
>   	}
> 
> --
> 2.45.2
> 
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

[PATCH 7/9] media: qcom: camss: change internals of endpoint parsing to fwnode handling

[Vladimir Zapolskiy]

Since a few called V4L2 functions operate with fwnode arguments the change
from OF device nodes to fwnodes brings a simplification to the code.

Because camss_probe() as the single caller of camss_of_parse_endpoint_node()
has no need to know a number of async registered remote devices, it makes
sense to remove the related computation from it. In addition there is no
reason to check for a OF device availability on CAMSS side, the check is
useless as the always passed one.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 52 ++++++++++-------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 39c5472f4552..d4745fb21152 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -2973,16 +2973,16 @@ static const struct parent_dev_ops vfe_parent_dev_ops = {
 };
 
 /*
- * camss_of_parse_endpoint_node - Parse port endpoint node
- * @dev: Device
- * @node: Device node to be parsed
+ * camss_parse_endpoint_node - Parse port endpoint node
+ * @dev: CAMSS device
+ * @ep: Device endpoint to be parsed
  * @csd: Parsed data from port endpoint node
  *
  * Return 0 on success or a negative error code on failure
  */
-static int camss_of_parse_endpoint_node(struct device *dev,
-					struct device_node *node,
-					struct camss_async_subdev *csd)
+static int camss_parse_endpoint_node(struct device *dev,
+				     struct fwnode_handle *ep,
+				     struct camss_async_subdev *csd)
 {
 	struct csiphy_lanes_cfg *lncfg = &csd->interface.csi2.lane_cfg;
 	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2;
@@ -2990,7 +2990,7 @@ static int camss_of_parse_endpoint_node(struct device *dev,
 	unsigned int i;
 	int ret;
 
-	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(node), &vep);
+	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
 	if (ret)
 		return ret;
 
@@ -3025,52 +3025,46 @@ static int camss_of_parse_endpoint_node(struct device *dev,
 }
 
 /*
- * camss_of_parse_ports - Parse ports node
- * @dev: Device
- * @notifier: v4l2_device notifier data
+ * camss_parse_ports - Parse ports node
+ * @dev: CAMSS device
  *
- * Return number of "port" nodes found in "ports" node
+ * Return 0 on success or a negative error code on failure
  */
-static int camss_of_parse_ports(struct camss *camss)
+static int camss_parse_ports(struct camss *camss)
 {
 	struct device *dev = camss->dev;
-	struct device_node *node = NULL;
-	struct device_node *remote = NULL;
-	int ret, num_subdevs = 0;
+	struct fwnode_handle *fwnode = dev_fwnode(dev), *ep;
+	int ret;
 
-	for_each_endpoint_of_node(dev->of_node, node) {
+	fwnode_graph_for_each_endpoint(fwnode, ep) {
 		struct camss_async_subdev *csd;
+		struct fwnode_handle *remote;
 
-		if (!of_device_is_available(node))
-			continue;
-
-		remote = of_graph_get_remote_port_parent(node);
+		remote = fwnode_graph_get_remote_port_parent(ep);
 		if (!remote) {
 			dev_err(dev, "Cannot get remote parent\n");
 			ret = -EINVAL;
 			goto err_cleanup;
 		}
 
-		csd = v4l2_async_nf_add_fwnode(&camss->notifier,
-					       of_fwnode_handle(remote),
+		csd = v4l2_async_nf_add_fwnode(&camss->notifier, remote,
 					       struct camss_async_subdev);
-		of_node_put(remote);
+		fwnode_handle_put(remote);
 		if (IS_ERR(csd)) {
 			ret = PTR_ERR(csd);
 			goto err_cleanup;
 		}
 
-		ret = camss_of_parse_endpoint_node(dev, node, csd);
+		ret = camss_parse_endpoint_node(dev, ep, csd);
 		if (ret < 0)
 			goto err_cleanup;
-
-		num_subdevs++;
 	}
 
-	return num_subdevs;
+	return 0;
 
 err_cleanup:
-	of_node_put(node);
+	fwnode_handle_put(ep);
+
 	return ret;
 }
 
@@ -3626,7 +3620,7 @@ static int camss_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
-	ret = camss_of_parse_ports(camss);
+	ret = camss_parse_ports(camss);
 	if (ret < 0)
 		goto err_v4l2_device_unregister;
 
-- 
2.45.2

Re: [PATCH 7/9] media: qcom: camss: change internals of endpoint parsing to fwnode handling

[Bryan O'Donoghue]

On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> +	fwnode_graph_for_each_endpoint(fwnode, ep) {
>   		struct camss_async_subdev *csd;
> +		struct fwnode_handle *remote;
> 
> -		if (!of_device_is_available(node))
> -			continue;

The change to fwnode seems fine I think but, either leave the 
of_device_is_available() check as-is or move its removal to a separate 
patch.

Changes should be as granular as possible.

---
bod

Re: [PATCH 7/9] media: qcom: camss: change internals of endpoint parsing to fwnode handling

[Vladimir Zapolskiy]

Hi Bryan.

On 6/13/25 12:30, Bryan O'Donoghue wrote:
> On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
>> +	fwnode_graph_for_each_endpoint(fwnode, ep) {
>>    		struct camss_async_subdev *csd;
>> +		struct fwnode_handle *remote;
>>
>> -		if (!of_device_is_available(node))
>> -			continue;
> 
> The change to fwnode seems fine I think but, either leave the
> of_device_is_available() check as-is or move its removal to a separate
> patch.
> 
> Changes should be as granular as possible.
> 

The change from OF handling to fwnode handling assumes that there is
no more room left for of_device_is_available() calls, it passes away.

of_device_is_available() here is useless, CAMSS ports are not devices
and they are not disabled in board dts.

--
Best wishes,
Vladimir

Re: [PATCH 7/9] media: qcom: camss: change internals of endpoint parsing to fwnode handling

[neil.armstrong]

On 13/05/2025 16:23, Vladimir Zapolskiy wrote:
> Since a few called V4L2 functions operate with fwnode arguments the change
> from OF device nodes to fwnodes brings a simplification to the code.
> 
> Because camss_probe() as the single caller of camss_of_parse_endpoint_node()
> has no need to know a number of async registered remote devices, it makes
> sense to remove the related computation from it. In addition there is no
> reason to check for a OF device availability on CAMSS side, the check is
> useless as the always passed one.

I think you should explain why it's useless, TBH I'm not even sure why it's
not necessary. What if we set the remote endpoint as disabled, this is
a regression, no ?

Why not replace it with  fwnode_device_is_available() and remove it in another
patch really explaining why it's useless ?

Neil

> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 52 ++++++++++-------------
>   1 file changed, 23 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 39c5472f4552..d4745fb21152 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -2973,16 +2973,16 @@ static const struct parent_dev_ops vfe_parent_dev_ops = {
>   };
>   
>   /*
> - * camss_of_parse_endpoint_node - Parse port endpoint node
> - * @dev: Device
> - * @node: Device node to be parsed
> + * camss_parse_endpoint_node - Parse port endpoint node
> + * @dev: CAMSS device
> + * @ep: Device endpoint to be parsed
>    * @csd: Parsed data from port endpoint node
>    *
>    * Return 0 on success or a negative error code on failure
>    */
> -static int camss_of_parse_endpoint_node(struct device *dev,
> -					struct device_node *node,
> -					struct camss_async_subdev *csd)
> +static int camss_parse_endpoint_node(struct device *dev,
> +				     struct fwnode_handle *ep,
> +				     struct camss_async_subdev *csd)
>   {
>   	struct csiphy_lanes_cfg *lncfg = &csd->interface.csi2.lane_cfg;
>   	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2;
> @@ -2990,7 +2990,7 @@ static int camss_of_parse_endpoint_node(struct device *dev,
>   	unsigned int i;
>   	int ret;
>   
> -	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(node), &vep);
> +	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
>   	if (ret)
>   		return ret;
>   
> @@ -3025,52 +3025,46 @@ static int camss_of_parse_endpoint_node(struct device *dev,
>   }
>   
>   /*
> - * camss_of_parse_ports - Parse ports node
> - * @dev: Device
> - * @notifier: v4l2_device notifier data
> + * camss_parse_ports - Parse ports node
> + * @dev: CAMSS device
>    *
> - * Return number of "port" nodes found in "ports" node
> + * Return 0 on success or a negative error code on failure
>    */
> -static int camss_of_parse_ports(struct camss *camss)
> +static int camss_parse_ports(struct camss *camss)
>   {
>   	struct device *dev = camss->dev;
> -	struct device_node *node = NULL;
> -	struct device_node *remote = NULL;
> -	int ret, num_subdevs = 0;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev), *ep;
> +	int ret;
>   
> -	for_each_endpoint_of_node(dev->of_node, node) {
> +	fwnode_graph_for_each_endpoint(fwnode, ep) {
>   		struct camss_async_subdev *csd;
> +		struct fwnode_handle *remote;
>   
> -		if (!of_device_is_available(node))
> -			continue;
> -
> -		remote = of_graph_get_remote_port_parent(node);
> +		remote = fwnode_graph_get_remote_port_parent(ep);
>   		if (!remote) {
>   			dev_err(dev, "Cannot get remote parent\n");
>   			ret = -EINVAL;
>   			goto err_cleanup;
>   		}
>   
> -		csd = v4l2_async_nf_add_fwnode(&camss->notifier,
> -					       of_fwnode_handle(remote),
> +		csd = v4l2_async_nf_add_fwnode(&camss->notifier, remote,
>   					       struct camss_async_subdev);
> -		of_node_put(remote);
> +		fwnode_handle_put(remote);
>   		if (IS_ERR(csd)) {
>   			ret = PTR_ERR(csd);
>   			goto err_cleanup;
>   		}
>   
> -		ret = camss_of_parse_endpoint_node(dev, node, csd);
> +		ret = camss_parse_endpoint_node(dev, ep, csd);
>   		if (ret < 0)
>   			goto err_cleanup;
> -
> -		num_subdevs++;
>   	}
>   
> -	return num_subdevs;
> +	return 0;
>   
>   err_cleanup:
> -	of_node_put(node);
> +	fwnode_handle_put(ep);
> +
>   	return ret;
>   }
>   
> @@ -3626,7 +3620,7 @@ static int camss_probe(struct platform_device *pdev)
>   
>   	pm_runtime_enable(dev);
>   
> -	ret = camss_of_parse_ports(camss);
> +	ret = camss_parse_ports(camss);
>   	if (ret < 0)
>   		goto err_v4l2_device_unregister;
>   

Re: [PATCH 7/9] media: qcom: camss: change internals of endpoint parsing to fwnode handling

[Vladimir Zapolskiy]

On 6/23/25 12:29, neil.armstrong@linaro.org wrote:
> On 13/05/2025 16:23, Vladimir Zapolskiy wrote:
>> Since a few called V4L2 functions operate with fwnode arguments the change
>> from OF device nodes to fwnodes brings a simplification to the code.
>>
>> Because camss_probe() as the single caller of camss_of_parse_endpoint_node()
>> has no need to know a number of async registered remote devices, it makes
>> sense to remove the related computation from it. In addition there is no
>> reason to check for a OF device availability on CAMSS side, the check is
>> useless as the always passed one.
> 
> I think you should explain why it's useless, TBH I'm not even sure why it's
> not necessary. What if we set the remote endpoint as disabled, this is
> a regression, no ?

Here the check of_device_is_available(node) verifies CAMSS endpoint "subdevice"
node availability, it's not about remote endpoints, and there is no such
usecases.

> Why not replace it with  fwnode_device_is_available() and remove it in another
> patch really explaining why it's useless ?
> 

Probably the commit message could be improved anyway, thank you.

Best wishes,
Vladimir

>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> ---
>>    drivers/media/platform/qcom/camss/camss.c | 52 ++++++++++-------------
>>    1 file changed, 23 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 39c5472f4552..d4745fb21152 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -2973,16 +2973,16 @@ static const struct parent_dev_ops vfe_parent_dev_ops = {
>>    };
>>    
>>    /*
>> - * camss_of_parse_endpoint_node - Parse port endpoint node
>> - * @dev: Device
>> - * @node: Device node to be parsed
>> + * camss_parse_endpoint_node - Parse port endpoint node
>> + * @dev: CAMSS device
>> + * @ep: Device endpoint to be parsed
>>     * @csd: Parsed data from port endpoint node
>>     *
>>     * Return 0 on success or a negative error code on failure
>>     */
>> -static int camss_of_parse_endpoint_node(struct device *dev,
>> -					struct device_node *node,
>> -					struct camss_async_subdev *csd)
>> +static int camss_parse_endpoint_node(struct device *dev,
>> +				     struct fwnode_handle *ep,
>> +				     struct camss_async_subdev *csd)
>>    {
>>    	struct csiphy_lanes_cfg *lncfg = &csd->interface.csi2.lane_cfg;
>>    	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2;
>> @@ -2990,7 +2990,7 @@ static int camss_of_parse_endpoint_node(struct device *dev,
>>    	unsigned int i;
>>    	int ret;
>>    
>> -	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(node), &vep);
>> +	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
>>    	if (ret)
>>    		return ret;
>>    
>> @@ -3025,52 +3025,46 @@ static int camss_of_parse_endpoint_node(struct device *dev,
>>    }
>>    
>>    /*
>> - * camss_of_parse_ports - Parse ports node
>> - * @dev: Device
>> - * @notifier: v4l2_device notifier data
>> + * camss_parse_ports - Parse ports node
>> + * @dev: CAMSS device
>>     *
>> - * Return number of "port" nodes found in "ports" node
>> + * Return 0 on success or a negative error code on failure
>>     */
>> -static int camss_of_parse_ports(struct camss *camss)
>> +static int camss_parse_ports(struct camss *camss)
>>    {
>>    	struct device *dev = camss->dev;
>> -	struct device_node *node = NULL;
>> -	struct device_node *remote = NULL;
>> -	int ret, num_subdevs = 0;
>> +	struct fwnode_handle *fwnode = dev_fwnode(dev), *ep;
>> +	int ret;
>>    
>> -	for_each_endpoint_of_node(dev->of_node, node) {
>> +	fwnode_graph_for_each_endpoint(fwnode, ep) {
>>    		struct camss_async_subdev *csd;
>> +		struct fwnode_handle *remote;
>>    
>> -		if (!of_device_is_available(node))
>> -			continue;
>> -
>> -		remote = of_graph_get_remote_port_parent(node);
>> +		remote = fwnode_graph_get_remote_port_parent(ep);
>>    		if (!remote) {
>>    			dev_err(dev, "Cannot get remote parent\n");
>>    			ret = -EINVAL;
>>    			goto err_cleanup;
>>    		}
>>    
>> -		csd = v4l2_async_nf_add_fwnode(&camss->notifier,
>> -					       of_fwnode_handle(remote),
>> +		csd = v4l2_async_nf_add_fwnode(&camss->notifier, remote,
>>    					       struct camss_async_subdev);
>> -		of_node_put(remote);
>> +		fwnode_handle_put(remote);
>>    		if (IS_ERR(csd)) {
>>    			ret = PTR_ERR(csd);
>>    			goto err_cleanup;
>>    		}
>>    
>> -		ret = camss_of_parse_endpoint_node(dev, node, csd);
>> +		ret = camss_parse_endpoint_node(dev, ep, csd);
>>    		if (ret < 0)
>>    			goto err_cleanup;
>> -
>> -		num_subdevs++;
>>    	}
>>    
>> -	return num_subdevs;
>> +	return 0;
>>    
>>    err_cleanup:
>> -	of_node_put(node);
>> +	fwnode_handle_put(ep);
>> +
>>    	return ret;
>>    }
>>    
>> @@ -3626,7 +3620,7 @@ static int camss_probe(struct platform_device *pdev)
>>    
>>    	pm_runtime_enable(dev);
>>    
>> -	ret = camss_of_parse_ports(camss);
>> +	ret = camss_parse_ports(camss);
>>    	if (ret < 0)
>>    		goto err_v4l2_device_unregister;
>>    
> 

[PATCH 8/9] media: qcom: camss: use a handy v4l2_async_nf_add_fwnode_remote() function

[Vladimir Zapolskiy]

One more code simplification makes parsing of remote endpoints easy.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index d4745fb21152..0d05f52a6e92 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -3038,18 +3038,9 @@ static int camss_parse_ports(struct camss *camss)
 
 	fwnode_graph_for_each_endpoint(fwnode, ep) {
 		struct camss_async_subdev *csd;
-		struct fwnode_handle *remote;
 
-		remote = fwnode_graph_get_remote_port_parent(ep);
-		if (!remote) {
-			dev_err(dev, "Cannot get remote parent\n");
-			ret = -EINVAL;
-			goto err_cleanup;
-		}
-
-		csd = v4l2_async_nf_add_fwnode(&camss->notifier, remote,
-					       struct camss_async_subdev);
-		fwnode_handle_put(remote);
+		csd = v4l2_async_nf_add_fwnode_remote(&camss->notifier, ep,
+						struct camss_async_subdev);
 		if (IS_ERR(csd)) {
 			ret = PTR_ERR(csd);
 			goto err_cleanup;
-- 
2.45.2

[PATCH 9/9] MAINTAINERS: add myself as a CAMSS patch reviewer

[Vladimir Zapolskiy]

Add myself as a review of Qualcomm CAMSS subsystem patches.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6dbdf02d6b0c..9b973c0128fa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20135,6 +20135,7 @@ QUALCOMM CAMERA SUBSYSTEM DRIVER
 M:	Robert Foss <rfoss@kernel.org>
 M:	Todor Tomov <todor.too@gmail.com>
 M:	Bryan O'Donoghue <bryan.odonoghue@linaro.org>
+R:	Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	Documentation/admin-guide/media/qcom_camss.rst
-- 
2.45.2

Re: [PATCH 9/9] MAINTAINERS: add myself as a CAMSS patch reviewer

[Bryan O'Donoghue]

On 13/05/2025 15:23, Vladimir Zapolskiy wrote:
> Add myself as a review of Qualcomm CAMSS subsystem patches.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   MAINTAINERS | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6dbdf02d6b0c..9b973c0128fa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20135,6 +20135,7 @@ QUALCOMM CAMERA SUBSYSTEM DRIVER
>   M:	Robert Foss <rfoss@kernel.org>
>   M:	Todor Tomov <todor.too@gmail.com>
>   M:	Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> +R:	Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>   L:	linux-media@vger.kernel.org
>   S:	Maintained
>   F:	Documentation/admin-guide/media/qcom_camss.rst

Acked-by: Bryan O'Donoghue <bod@kernel.org>

Re: [PATCH 0/9] media: qcom: camss: a number of cleanups and fixes

[Vladimir Zapolskiy]

Hello Bryan, Robert, Todor.

On 5/13/25 17:23, Vladimir Zapolskiy wrote:
> The patchset prepares the ground for a CSIPHY rework in CAMSS ISP driver.
> 
> For simplicity of continuing my reviews of CAMSS paches add myself as
> a reviewer, as well it will simplify the work of syncing and avoiding
> patch conflicts between work oin CAMSS done by others and myself.
> 
> Vladimir Zapolskiy (9):
>    media: qcom: camss: cleanup media device allocated resource on error path
>    media: qcom: camss: remove duplicated csiphy_formats_sc7280 data
>    media: qcom: camss: remove .link_entities
>    media: qcom: camss: register camss media device before subdevices
>    media: qcom: camss: unconditionally set async notifier of subdevices
>    media: qcom: camss: simplify camss_subdev_notifier_complete() function
>    media: qcom: camss: change internals of endpoint parsing to fwnode handling
>    media: qcom: camss: use a handy v4l2_async_nf_add_fwnode_remote() function
>    MAINTAINERS: add myself as a CAMSS patch reviewer
> 
>   MAINTAINERS                                   |   1 +
>   .../media/platform/qcom/camss/camss-csiphy.c  |   5 -
>   .../media/platform/qcom/camss/camss-csiphy.h  |   1 -
>   drivers/media/platform/qcom/camss/camss.c     | 182 +++++++-----------
>   drivers/media/platform/qcom/camss/camss.h     |   1 -
>   5 files changed, 71 insertions(+), 119 deletions(-)
> 

Let me kindly remind about this unreviewed patchset, the support of new
platforms like MSM8939 in CAMSS comes later and now with merge conflicts.

--
Best wishes,
Vladimir

Re: [PATCH 0/9] media: qcom: camss: a number of cleanups and fixes

[Vladimir Zapolskiy]

On 5/13/25 17:23, Vladimir Zapolskiy wrote:
> The patchset prepares the ground for a CSIPHY rework in CAMSS ISP driver.
> 
> For simplicity of continuing my reviews of CAMSS paches add myself as
> a reviewer, as well it will simplify the work of syncing and avoiding
> patch conflicts between work oin CAMSS done by others and myself.
> 
> Vladimir Zapolskiy (9):
>    media: qcom: camss: cleanup media device allocated resource on error path
>    media: qcom: camss: remove duplicated csiphy_formats_sc7280 data
>    media: qcom: camss: remove .link_entities
>    media: qcom: camss: register camss media device before subdevices
>    media: qcom: camss: unconditionally set async notifier of subdevices
>    media: qcom: camss: simplify camss_subdev_notifier_complete() function
>    media: qcom: camss: change internals of endpoint parsing to fwnode handling
>    media: qcom: camss: use a handy v4l2_async_nf_add_fwnode_remote() function
>    MAINTAINERS: add myself as a CAMSS patch reviewer
> 
>   MAINTAINERS                                   |   1 +
>   .../media/platform/qcom/camss/camss-csiphy.c  |   5 -
>   .../media/platform/qcom/camss/camss-csiphy.h  |   1 -
>   drivers/media/platform/qcom/camss/camss.c     | 182 +++++++-----------
>   drivers/media/platform/qcom/camss/camss.h     |   1 -
>   5 files changed, 71 insertions(+), 119 deletions(-)
> 

The changeset is left unreviewed for 4 weeks...

May I ask linux-media maintainers to take care of the patchset?..

--
Best wishes,
Vladimir

Re: [PATCH 0/9] media: qcom: camss: a number of cleanups and fixes

[Bryan O'Donoghue]

On 10/06/2025 18:26, Vladimir Zapolskiy wrote:
> On 5/13/25 17:23, Vladimir Zapolskiy wrote:
>> The patchset prepares the ground for a CSIPHY rework in CAMSS ISP driver.
>>
>> For simplicity of continuing my reviews of CAMSS paches add myself as
>> a reviewer, as well it will simplify the work of syncing and avoiding
>> patch conflicts between work oin CAMSS done by others and myself.
>>
>> Vladimir Zapolskiy (9):
>>    media: qcom: camss: cleanup media device allocated resource on 
>> error path
>>    media: qcom: camss: remove duplicated csiphy_formats_sc7280 data
>>    media: qcom: camss: remove .link_entities
>>    media: qcom: camss: register camss media device before subdevices
>>    media: qcom: camss: unconditionally set async notifier of subdevices
>>    media: qcom: camss: simplify camss_subdev_notifier_complete() function
>>    media: qcom: camss: change internals of endpoint parsing to fwnode 
>> handling
>>    media: qcom: camss: use a handy v4l2_async_nf_add_fwnode_remote() 
>> function
>>    MAINTAINERS: add myself as a CAMSS patch reviewer
>>
>>   MAINTAINERS                                   |   1 +
>>   .../media/platform/qcom/camss/camss-csiphy.c  |   5 -
>>   .../media/platform/qcom/camss/camss-csiphy.h  |   1 -
>>   drivers/media/platform/qcom/camss/camss.c     | 182 +++++++-----------
>>   drivers/media/platform/qcom/camss/camss.h     |   1 -
>>   5 files changed, 71 insertions(+), 119 deletions(-)
>>
> 
> The changeset is left unreviewed for 4 weeks...
> 
> May I ask linux-media maintainers to take care of the patchset?..
> 
> -- 
> Best wishes,
> Vladimir

I haven't forgotten about your series, thanks for the ping.

---
bod

Re: [PATCH 0/9] media: qcom: camss: a number of cleanups and fixes

[Vladimir Zapolskiy]

Hi Bryan.

On 6/10/25 21:53, Bryan O'Donoghue wrote:
> On 10/06/2025 18:26, Vladimir Zapolskiy wrote:
>> On 5/13/25 17:23, Vladimir Zapolskiy wrote:
>>> The patchset prepares the ground for a CSIPHY rework in CAMSS ISP driver.
>>>
>>> For simplicity of continuing my reviews of CAMSS paches add myself as
>>> a reviewer, as well it will simplify the work of syncing and avoiding
>>> patch conflicts between work oin CAMSS done by others and myself.
>>>
>>> Vladimir Zapolskiy (9):
>>>     media: qcom: camss: cleanup media device allocated resource on
>>> error path
>>>     media: qcom: camss: remove duplicated csiphy_formats_sc7280 data
>>>     media: qcom: camss: remove .link_entities
>>>     media: qcom: camss: register camss media device before subdevices
>>>     media: qcom: camss: unconditionally set async notifier of subdevices
>>>     media: qcom: camss: simplify camss_subdev_notifier_complete() function
>>>     media: qcom: camss: change internals of endpoint parsing to fwnode
>>> handling
>>>     media: qcom: camss: use a handy v4l2_async_nf_add_fwnode_remote()
>>> function
>>>     MAINTAINERS: add myself as a CAMSS patch reviewer
>>>
>>>    MAINTAINERS                                   |   1 +
>>>    .../media/platform/qcom/camss/camss-csiphy.c  |   5 -
>>>    .../media/platform/qcom/camss/camss-csiphy.h  |   1 -
>>>    drivers/media/platform/qcom/camss/camss.c     | 182 +++++++-----------
>>>    drivers/media/platform/qcom/camss/camss.h     |   1 -
>>>    5 files changed, 71 insertions(+), 119 deletions(-)
>>>
>>
>> The changeset is left unreviewed for 4 weeks...
>>
>> May I ask linux-media maintainers to take care of the patchset?..
>>
>> -- 
>> Best wishes,
>> Vladimir
> 
> I haven't forgotten about your series, thanks for the ping.
> 

Thank you in advance, it would be nice to get your review/testing
and get the changeset landed into the linux-next.

--
Best wishes,
Vladimir