[PATCH v3 00/15] media: qcom: camss: Add parameter passing to remove several outstanding bugs

[PATCH v3 00/15] media: qcom: camss: Add parameter passing to remove several outstanding bugs

[Bryan O'Donoghue]

V3:
- Adds RB/AB as indicated - Konrad
- Replaces >= SDM845 with helper function per discussion - bod/Konrad
- Leaves out constraining VFE clock names sizes. A full pass for resource strings will happen later. - bod
- Clarifies commit log resulting in updated patch title also
  "Add support for setting CSIPHY clock name csiphyX"
  ->
  "Fix support for setting CSIPHY clock name csiphyX"
- Adds patch to remove dead integer return type in vfe_disable()
- Adds patch to comment CSID dt_id meanining which I personally find non-obvious right now - bod

Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/commits/09e7805a733b488c5dc19b301eb3b77cb0fad3d6

V2:
- Replaces &camss->res with pointer to res - Konrad
- Squashes patch for NULL removal - Konrad
- Left suggestion on ICC initialisation points alone, doesn't seem to fit Konrad/bod

Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-08-07-db410c-rb3-camss-dts-v3+maintenance-bugfixes-v2

V1:
- I forgot to include patch # 14 in V0 of this series.
  This patch leverages previous changes to unwind the fixed polling of
  RDI[0..2] allowing driver data to articulate on a per-VFE basis how many
  RDIs to poll.

Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-08-07-db410c-rb3-camss-dts-v3+maintenance-bugfixes-v1

V0:
This second series of bugfixes stacks ontop of the Fixes series sent earlier.

Link: https://lore.kernel.org/linux-arm-msm/20230814141007.3721197-1-bryan.odonoghue@linaro.org/T/#t

Rather than send both series as one giant series, I opted to send a pure
Fixes series above, with this second series a non-backport series i.e. no
Fixes tags in this series.

The existing CAMSS code relies on some hard-coded parameters buried inside
of the driver, instead of passed via compat .data as arguably ought to be
the case.

This brittle model is an extending morass of spaghetti code. More than that
in CAMSS Video Front Ends (VFEs) and the number of Raw Data Interfaces
(RDIs) per VFE can vary from SoC to SoC. Indeed sm8250 has VFE and VFE Lite
blocks which have a different number of RDIs per block.

The use of defines as opposed to per-compat parameters inside of ISRs leads
to either under-polling or over-polling the number of RDIs.

On top of all of that we have some hard-coded statements for clock names
which breaks easily.

We can solve the under/over polling loop problem by transitioning loop
controls from macros to parameters passed via probe().

Similarly and unsurprisingly we can also solve the hard-coded clock problem
by adding some string processing routines that take passed arguments.

There is still some additional maintenance work to be done in this driver
but before adding one more SoC the code needs to be made more extensible
and less brittle.

Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/commits/dc346c7f46c0680bcfb84fded6db97497fffe49a

Bryan O'Donoghue (15):
  media: qcom: camss: Amalgamate struct resource with struct
    resource_ispif
  media: qcom: camss: Start to move to module compat matched resources
  media: qcom: camss: Pass icc bandwidth table as a platform parameter
  media: qcom: camss: Pass remainder of variables as resources
  media: qcom: camss: Pass line_num from compat resources
  media: qcom: camss: Assign the correct number of RDIs per VFE
  media: qcom: camss: Capture VFE CSID dependency in a helper function
  media: qcom: camss: Untangle if/else spaghetti in camss
  media: qcom: camss: Improve error printout on icc_get fail
  media: qcom: camss: Allow clocks vfeN vfe_liteN or vfe_lite
  media: qcom: camss: Functionally decompose CSIPHY clock lookups
  media: qcom: camss: Fix support for setting CSIPHY clock name csiphyX
  media: qcom: camss: Support RDI3 for VFE 17x
  media: qcom: camss: Convert vfe_disable() from int to void
  media: qcom: camss: Comment CSID dt_id field

 .../platform/qcom/camss/camss-csid-gen2.c     |   5 +
 .../media/platform/qcom/camss/camss-csid.c    |  40 ++-
 .../qcom/camss/camss-csiphy-3ph-1-0.c         |   8 +-
 .../media/platform/qcom/camss/camss-csiphy.c  |  67 ++--
 .../media/platform/qcom/camss/camss-ispif.c   |  32 +-
 .../media/platform/qcom/camss/camss-ispif.h   |   4 +-
 .../media/platform/qcom/camss/camss-vfe-170.c |  22 +-
 .../media/platform/qcom/camss/camss-vfe-4-1.c |   2 -
 .../media/platform/qcom/camss/camss-vfe-4-7.c |   2 -
 .../media/platform/qcom/camss/camss-vfe-4-8.c |   2 -
 .../media/platform/qcom/camss/camss-vfe-480.c |  10 +-
 .../platform/qcom/camss/camss-vfe-gen1.c      |   5 +-
 .../platform/qcom/camss/camss-vfe-gen1.h      |   3 +-
 drivers/media/platform/qcom/camss/camss-vfe.c |  83 +++--
 drivers/media/platform/qcom/camss/camss-vfe.h |   2 +-
 .../media/platform/qcom/camss/camss-video.c   |  16 +-
 drivers/media/platform/qcom/camss/camss.c     | 293 +++++++++---------
 drivers/media/platform/qcom/camss/camss.h     |  31 +-
 18 files changed, 352 insertions(+), 275 deletions(-)

-- 
2.41.0

[PATCH v3 01/15] media: qcom: camss: Amalgamate struct resource with struct resource_ispif

[Bryan O'Donoghue]

There is no good reason to differentiate the two resource structures
here. As part of a general tidyup of the declaration and passing of
resources within in the CAMSS driver it will be advantageous to have
one unified resource structure.

The two structures are very similar anyway thus leading more credence
still to the argument there should be only one.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-ispif.c |  4 ++--
 drivers/media/platform/qcom/camss/camss-ispif.h |  4 ++--
 drivers/media/platform/qcom/camss/camss.c       | 14 +++++++-------
 drivers/media/platform/qcom/camss/camss.h       |  8 +-------
 4 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c
index 1c7e4b1b49401..61765b874b9a2 100644
--- a/drivers/media/platform/qcom/camss/camss-ispif.c
+++ b/drivers/media/platform/qcom/camss/camss-ispif.c
@@ -1095,7 +1095,7 @@ static int ispif_init_formats(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
  * Return 0 on success or a negative error code otherwise
  */
 int msm_ispif_subdev_init(struct camss *camss,
-			  const struct resources_ispif *res)
+			  const struct resources *res)
 {
 	struct device *dev = camss->dev;
 	struct ispif_device *ispif = camss->ispif;
@@ -1152,7 +1152,7 @@ int msm_ispif_subdev_init(struct camss *camss,
 
 	/* Interrupt */
 
-	ret = platform_get_irq_byname(pdev, res->interrupt);
+	ret = platform_get_irq_byname(pdev, res->interrupt[0]);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/media/platform/qcom/camss/camss-ispif.h b/drivers/media/platform/qcom/camss/camss-ispif.h
index fdf28e68cc7d8..c7c41f7afcaad 100644
--- a/drivers/media/platform/qcom/camss/camss-ispif.h
+++ b/drivers/media/platform/qcom/camss/camss-ispif.h
@@ -66,10 +66,10 @@ struct ispif_device {
 	struct camss *camss;
 };
 
-struct resources_ispif;
+struct resources;
 
 int msm_ispif_subdev_init(struct camss *camss,
-			  const struct resources_ispif *res);
+			  const struct resources *res);
 
 int msm_ispif_register_entities(struct ispif_device *ispif,
 				struct v4l2_device *v4l2_dev);
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index f4948bdf3f8f9..de39dc987444f 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -93,14 +93,14 @@ static const struct resources csid_res_8x16[] = {
 	},
 };
 
-static const struct resources_ispif ispif_res_8x16 = {
+static const struct resources ispif_res_8x16 = {
 	/* ISPIF */
 	.clock = { "top_ahb", "ahb", "ispif_ahb",
 		   "csi0", "csi0_pix", "csi0_rdi",
 		   "csi1", "csi1_pix", "csi1_rdi" },
 	.clock_for_reset = { "vfe0", "csi_vfe0" },
 	.reg = { "ispif", "csi_clk_mux" },
-	.interrupt = "ispif"
+	.interrupt = { "ispif" }
 
 };
 
@@ -234,7 +234,7 @@ static const struct resources csid_res_8x96[] = {
 	}
 };
 
-static const struct resources_ispif ispif_res_8x96 = {
+static const struct resources ispif_res_8x96 = {
 	/* ISPIF */
 	.clock = { "top_ahb", "ahb", "ispif_ahb",
 		   "csi0", "csi0_pix", "csi0_rdi",
@@ -243,7 +243,7 @@ static const struct resources_ispif ispif_res_8x96 = {
 		   "csi3", "csi3_pix", "csi3_rdi" },
 	.clock_for_reset = { "vfe0", "csi_vfe0", "vfe1", "csi_vfe1" },
 	.reg = { "ispif", "csi_clk_mux" },
-	.interrupt = "ispif"
+	.interrupt = { "ispif" }
 };
 
 static const struct resources vfe_res_8x96[] = {
@@ -410,7 +410,7 @@ static const struct resources csid_res_660[] = {
 	}
 };
 
-static const struct resources_ispif ispif_res_660 = {
+static const struct resources ispif_res_660 = {
 	/* ISPIF */
 	.clock = { "top_ahb", "ahb", "ispif_ahb",
 		   "csi0", "csi0_pix", "csi0_rdi",
@@ -419,7 +419,7 @@ static const struct resources_ispif ispif_res_660 = {
 		   "csi3", "csi3_pix", "csi3_rdi" },
 	.clock_for_reset = { "vfe0", "csi_vfe0", "vfe1", "csi_vfe1" },
 	.reg = { "ispif", "csi_clk_mux" },
-	.interrupt = "ispif"
+	.interrupt = { "ispif" }
 };
 
 static const struct resources vfe_res_660[] = {
@@ -1122,7 +1122,7 @@ static int camss_init_subdevices(struct camss *camss)
 {
 	const struct resources *csiphy_res;
 	const struct resources *csid_res;
-	const struct resources_ispif *ispif_res;
+	const struct resources *ispif_res;
 	const struct resources *vfe_res;
 	unsigned int i;
 	int ret;
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index f6c326cb853b8..e95211cdb1fd6 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -44,18 +44,12 @@
 struct resources {
 	char *regulators[CAMSS_RES_MAX];
 	char *clock[CAMSS_RES_MAX];
+	char *clock_for_reset[CAMSS_RES_MAX];
 	u32 clock_rate[CAMSS_RES_MAX][CAMSS_RES_MAX];
 	char *reg[CAMSS_RES_MAX];
 	char *interrupt[CAMSS_RES_MAX];
 };
 
-struct resources_ispif {
-	char *clock[CAMSS_RES_MAX];
-	char *clock_for_reset[CAMSS_RES_MAX];
-	char *reg[CAMSS_RES_MAX];
-	char *interrupt;
-};
-
 struct icc_bw_tbl {
 	u32 avg;
 	u32 peak;
-- 
2.41.0

[PATCH v3 02/15] media: qcom: camss: Start to move to module compat matched resources

[Bryan O'Donoghue]

There is a lot of unnecessary if/elsing in this code that arguably
should never have made it upstream when adding a second let alone
subsequent SoC.

I'm guilty of not fixing the mess myself when adding in the sm8250.
Before adding in any new SoCs or resources lets take the time to cleanup
the resource passing.

First step is to pass the generic struct camss_resources as a parameter
per the compatible list.

Subsequent patches will address the other somewhat dispirate strutures
which we are also doing if/else on and assigning statically.

Squashed down a commit to drop useless NULL assignment for ispif resources.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 92 ++++++++++++-----------
 drivers/media/platform/qcom/camss/camss.h |  8 ++
 2 files changed, 56 insertions(+), 44 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index de39dc987444f..82e679c8ca011 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_graph.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_domain.h>
@@ -1120,47 +1121,13 @@ static int camss_of_parse_ports(struct camss *camss)
  */
 static int camss_init_subdevices(struct camss *camss)
 {
-	const struct resources *csiphy_res;
-	const struct resources *csid_res;
-	const struct resources *ispif_res;
-	const struct resources *vfe_res;
+	const struct camss_resources *res = camss->res;
 	unsigned int i;
 	int ret;
 
-	if (camss->version == CAMSS_8x16) {
-		csiphy_res = csiphy_res_8x16;
-		csid_res = csid_res_8x16;
-		ispif_res = &ispif_res_8x16;
-		vfe_res = vfe_res_8x16;
-	} else if (camss->version == CAMSS_8x96) {
-		csiphy_res = csiphy_res_8x96;
-		csid_res = csid_res_8x96;
-		ispif_res = &ispif_res_8x96;
-		vfe_res = vfe_res_8x96;
-	} else if (camss->version == CAMSS_660) {
-		csiphy_res = csiphy_res_660;
-		csid_res = csid_res_660;
-		ispif_res = &ispif_res_660;
-		vfe_res = vfe_res_660;
-	}  else if (camss->version == CAMSS_845) {
-		csiphy_res = csiphy_res_845;
-		csid_res = csid_res_845;
-		/* Titan VFEs don't have an ISPIF  */
-		ispif_res = NULL;
-		vfe_res = vfe_res_845;
-	} else if (camss->version == CAMSS_8250) {
-		csiphy_res = csiphy_res_8250;
-		csid_res = csid_res_8250;
-		/* Titan VFEs don't have an ISPIF  */
-		ispif_res = NULL;
-		vfe_res = vfe_res_8250;
-	} else {
-		return -EINVAL;
-	}
-
 	for (i = 0; i < camss->csiphy_num; i++) {
 		ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
-					     &csiphy_res[i], i);
+					     &res->csiphy_res[i], i);
 		if (ret < 0) {
 			dev_err(camss->dev,
 				"Failed to init csiphy%d sub-device: %d\n",
@@ -1172,7 +1139,7 @@ static int camss_init_subdevices(struct camss *camss)
 	/* note: SM8250 requires VFE to be initialized before CSID */
 	for (i = 0; i < camss->vfe_num + camss->vfe_lite_num; i++) {
 		ret = msm_vfe_subdev_init(camss, &camss->vfe[i],
-					  &vfe_res[i], i);
+					  &res->vfe_res[i], i);
 		if (ret < 0) {
 			dev_err(camss->dev,
 				"Fail to init vfe%d sub-device: %d\n", i, ret);
@@ -1182,7 +1149,7 @@ static int camss_init_subdevices(struct camss *camss)
 
 	for (i = 0; i < camss->csid_num; i++) {
 		ret = msm_csid_subdev_init(camss, &camss->csid[i],
-					   &csid_res[i], i);
+					   &res->csid_res[i], i);
 		if (ret < 0) {
 			dev_err(camss->dev,
 				"Failed to init csid%d sub-device: %d\n",
@@ -1191,7 +1158,7 @@ static int camss_init_subdevices(struct camss *camss)
 		}
 	}
 
-	ret = msm_ispif_subdev_init(camss, ispif_res);
+	ret = msm_ispif_subdev_init(camss, res->ispif_res);
 	if (ret < 0) {
 		dev_err(camss->dev, "Failed to init ispif sub-device: %d\n",
 		ret);
@@ -1554,6 +1521,10 @@ static int camss_probe(struct platform_device *pdev)
 	if (!camss)
 		return -ENOMEM;
 
+	camss->res = of_device_get_match_data(dev);
+	if (!camss->res)
+		return -ENODEV;
+
 	atomic_set(&camss->ref_count, 0);
 	camss->dev = dev;
 	platform_set_drvdata(pdev, camss);
@@ -1735,12 +1706,45 @@ static void camss_remove(struct platform_device *pdev)
 		camss_delete(camss);
 }
 
+static const struct camss_resources msm8916_resources = {
+	.csiphy_res = csiphy_res_8x16,
+	.csid_res = csid_res_8x16,
+	.ispif_res = &ispif_res_8x16,
+	.vfe_res = vfe_res_8x16,
+};
+
+static const struct camss_resources msm8996_resources = {
+	.csiphy_res = csiphy_res_8x96,
+	.csid_res = csid_res_8x96,
+	.ispif_res = &ispif_res_8x96,
+	.vfe_res = vfe_res_8x96,
+};
+
+static const struct camss_resources sdm660_resources = {
+	.csiphy_res = csiphy_res_660,
+	.csid_res = csid_res_660,
+	.ispif_res = &ispif_res_660,
+	.vfe_res = vfe_res_660,
+};
+
+static const struct camss_resources sdm845_resources = {
+	.csiphy_res = csiphy_res_845,
+	.csid_res = csid_res_845,
+	.vfe_res = vfe_res_845,
+};
+
+static const struct camss_resources sm8250_resources = {
+	.csiphy_res = csiphy_res_8250,
+	.csid_res = csid_res_8250,
+	.vfe_res = vfe_res_8250,
+};
+
 static const struct of_device_id camss_dt_match[] = {
-	{ .compatible = "qcom,msm8916-camss" },
-	{ .compatible = "qcom,msm8996-camss" },
-	{ .compatible = "qcom,sdm660-camss" },
-	{ .compatible = "qcom,sdm845-camss" },
-	{ .compatible = "qcom,sm8250-camss" },
+	{ .compatible = "qcom,msm8916-camss", .data = &msm8916_resources },
+	{ .compatible = "qcom,msm8996-camss", .data = &msm8996_resources },
+	{ .compatible = "qcom,sdm660-camss", .data = &sdm660_resources },
+	{ .compatible = "qcom,sdm845-camss", .data = &sdm845_resources },
+	{ .compatible = "qcom,sm8250-camss", .data = &sm8250_resources },
 	{ }
 };
 
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index e95211cdb1fd6..f632ee49ad83e 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -79,6 +79,13 @@ enum icc_count {
 	ICC_SM8250_COUNT = 4,
 };
 
+struct camss_resources {
+	const struct resources *csiphy_res;
+	const struct resources *csid_res;
+	const struct resources *ispif_res;
+	const struct resources *vfe_res;
+};
+
 struct camss {
 	enum camss_version version;
 	struct v4l2_device v4l2_dev;
@@ -99,6 +106,7 @@ struct camss {
 	struct device_link **genpd_link;
 	struct icc_path *icc_path[ICC_SM8250_COUNT];
 	struct icc_bw_tbl icc_bw_tbl[ICC_SM8250_COUNT];
+	const struct camss_resources *res;
 };
 
 struct camss_camera_interface {
-- 
2.41.0

[PATCH v3 03/15] media: qcom: camss: Pass icc bandwidth table as a platform parameter

[Bryan O'Donoghue]

Pass the bandwidth table as a platform parameter not if/else derived
pointer to the static table.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 29 +++++++----------------
 drivers/media/platform/qcom/camss/camss.h |  3 ++-
 2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 82e679c8ca011..9a15f2253aa20 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1485,21 +1485,15 @@ static int camss_configure_pd(struct camss *camss)
 static int camss_icc_get(struct camss *camss)
 {
 	const struct resources_icc *icc_res;
-	int nbr_icc_paths = 0;
 	int i;
 
-	if (camss->version == CAMSS_8250) {
-		icc_res = &icc_res_sm8250[0];
-		nbr_icc_paths =	ICC_SM8250_COUNT;
-	}
+	icc_res = camss->res->icc_res;
 
-	for (i = 0; i < nbr_icc_paths; i++) {
+	for (i = 0; i < camss->res->icc_path_num; i++) {
 		camss->icc_path[i] = devm_of_icc_get(camss->dev,
 						     icc_res[i].name);
 		if (IS_ERR(camss->icc_path[i]))
 			return PTR_ERR(camss->icc_path[i]);
-
-		camss->icc_bw_tbl[i] = icc_res[i].icc_bw_tbl;
 	}
 
 	return 0;
@@ -1737,6 +1731,8 @@ static const struct camss_resources sm8250_resources = {
 	.csiphy_res = csiphy_res_8250,
 	.csid_res = csid_res_8250,
 	.vfe_res = vfe_res_8250,
+	.icc_res = icc_res_sm8250,
+	.icc_path_num = ARRAY_SIZE(icc_res_sm8250),
 };
 
 static const struct of_device_id camss_dt_match[] = {
@@ -1753,14 +1749,10 @@ MODULE_DEVICE_TABLE(of, camss_dt_match);
 static int __maybe_unused camss_runtime_suspend(struct device *dev)
 {
 	struct camss *camss = dev_get_drvdata(dev);
-	int nbr_icc_paths = 0;
 	int i;
 	int ret;
 
-	if (camss->version == CAMSS_8250)
-		nbr_icc_paths =	ICC_SM8250_COUNT;
-
-	for (i = 0; i < nbr_icc_paths; i++) {
+	for (i = 0; i < camss->res->icc_path_num; i++) {
 		ret = icc_set_bw(camss->icc_path[i], 0, 0);
 		if (ret)
 			return ret;
@@ -1772,17 +1764,14 @@ static int __maybe_unused camss_runtime_suspend(struct device *dev)
 static int __maybe_unused camss_runtime_resume(struct device *dev)
 {
 	struct camss *camss = dev_get_drvdata(dev);
-	int nbr_icc_paths = 0;
+	const struct resources_icc *icc_res = camss->res->icc_res;
 	int i;
 	int ret;
 
-	if (camss->version == CAMSS_8250)
-		nbr_icc_paths =	ICC_SM8250_COUNT;
-
-	for (i = 0; i < nbr_icc_paths; i++) {
+	for (i = 0; i < camss->res->icc_path_num; i++) {
 		ret = icc_set_bw(camss->icc_path[i],
-				 camss->icc_bw_tbl[i].avg,
-				 camss->icc_bw_tbl[i].peak);
+				 icc_res[i].icc_bw_tbl.avg,
+				 icc_res[i].icc_bw_tbl.peak);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index f632ee49ad83e..988c313b63551 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -84,6 +84,8 @@ struct camss_resources {
 	const struct resources *csid_res;
 	const struct resources *ispif_res;
 	const struct resources *vfe_res;
+	const struct resources_icc *icc_res;
+	const int icc_path_num;
 };
 
 struct camss {
@@ -105,7 +107,6 @@ struct camss {
 	struct device **genpd;
 	struct device_link **genpd_link;
 	struct icc_path *icc_path[ICC_SM8250_COUNT];
-	struct icc_bw_tbl icc_bw_tbl[ICC_SM8250_COUNT];
 	const struct camss_resources *res;
 };
 
-- 
2.41.0

[PATCH v3 04/15] media: qcom: camss: Pass remainder of variables as resources

[Bryan O'Donoghue]

The following variables are being assigned statically based on
compatible strings in the probe path.

* enum camss_version version;
* int csiphy_num;
* int csid_num;
* int vfe_num;
* int vfe_lite_num;
* int vfe_total_num;

Migrate those variables to resource parameters passed in on platform
probe arguments. The one caveat is for VFE it has been necessary to
intoduce a new variable vfe_total_num to capture the aggregate value of
vfe_num + vfe_lite_num.

All the rest of the changes are rote camss->variable to
camss->res->variable with the parameter tables now populating the listed
variables.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../media/platform/qcom/camss/camss-csid.c    |  14 +--
 .../qcom/camss/camss-csiphy-3ph-1-0.c         |   8 +-
 .../media/platform/qcom/camss/camss-csiphy.c  |  16 +--
 .../media/platform/qcom/camss/camss-ispif.c   |  28 ++---
 .../media/platform/qcom/camss/camss-vfe-170.c |   4 +-
 .../media/platform/qcom/camss/camss-vfe-480.c |   4 +-
 drivers/media/platform/qcom/camss/camss-vfe.c |  26 ++--
 .../media/platform/qcom/camss/camss-video.c   |  10 +-
 drivers/media/platform/qcom/camss/camss.c     | 119 +++++++++---------
 drivers/media/platform/qcom/camss/camss.h     |  11 +-
 10 files changed, 117 insertions(+), 123 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index 9d7fb1adebb98..08991b070bd61 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -159,7 +159,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
 	struct camss *camss = csid->camss;
 	struct device *dev = camss->dev;
 	struct vfe_device *vfe = &camss->vfe[csid->id];
-	u32 version = camss->version;
+	u32 version = camss->res->version;
 	int ret = 0;
 
 	if (on) {
@@ -576,13 +576,13 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
 	csid->camss = camss;
 	csid->id = id;
 
-	if (camss->version == CAMSS_8x16) {
+	if (camss->res->version == CAMSS_8x16) {
 		csid->ops = &csid_ops_4_1;
-	} else if (camss->version == CAMSS_8x96 ||
-		   camss->version == CAMSS_660) {
+	} else if (camss->res->version == CAMSS_8x96 ||
+		   camss->res->version == CAMSS_660) {
 		csid->ops = &csid_ops_4_7;
-	} else if (camss->version == CAMSS_845 ||
-		   camss->version == CAMSS_8250) {
+	} else if (camss->res->version == CAMSS_845 ||
+		   camss->res->version == CAMSS_8250) {
 		csid->ops = &csid_ops_gen2;
 	} else {
 		return -EINVAL;
@@ -591,7 +591,7 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
 
 	/* Memory */
 
-	if (camss->version == CAMSS_8250) {
+	if (camss->res->version == CAMSS_8250) {
 		/* for titan 480, CSID registers are inside the VFE region,
 		 * between the VFE "top" and "bus" registers. this requires
 		 * VFE to be initialized before CSID
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
index 4dba61b8d3f2a..f50e2235c37fc 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
@@ -402,7 +402,7 @@ static void csiphy_gen1_config_lanes(struct csiphy_device *csiphy,
 	val = CSIPHY_3PH_LNn_CFG1_SWI_REC_DLY_PRG;
 	writel_relaxed(val, csiphy->base + CSIPHY_3PH_LNn_CFG1(l));
 
-	if (csiphy->camss->version == CAMSS_660)
+	if (csiphy->camss->res->version == CAMSS_660)
 		val = CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS_660;
 	else
 		val = CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS;
@@ -419,7 +419,7 @@ static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
 	int i, l, array_size;
 	u32 val;
 
-	switch (csiphy->camss->version) {
+	switch (csiphy->camss->res->version) {
 	case CAMSS_845:
 		r = &lane_regs_sdm845[0][0];
 		array_size = ARRAY_SIZE(lane_regs_sdm845[0]);
@@ -468,8 +468,8 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,
 				s64 link_freq, u8 lane_mask)
 {
 	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
-	bool is_gen2 = (csiphy->camss->version == CAMSS_845 ||
-			csiphy->camss->version == CAMSS_8250);
+	bool is_gen2 = (csiphy->camss->res->version == CAMSS_845 ||
+			csiphy->camss->res->version == CAMSS_8250);
 	u8 settle_cnt;
 	u8 val;
 	int i;
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index c5ae9c61ee0b1..593aec5c97bc2 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -557,17 +557,17 @@ int msm_csiphy_subdev_init(struct camss *camss,
 	csiphy->id = id;
 	csiphy->cfg.combo_mode = 0;
 
-	if (camss->version == CAMSS_8x16) {
+	if (camss->res->version == CAMSS_8x16) {
 		csiphy->ops = &csiphy_ops_2ph_1_0;
 		csiphy->formats = csiphy_formats_8x16;
 		csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x16);
-	} else if (camss->version == CAMSS_8x96 ||
-		   camss->version == CAMSS_660) {
+	} else if (camss->res->version == CAMSS_8x96 ||
+		   camss->res->version == CAMSS_660) {
 		csiphy->ops = &csiphy_ops_3ph_1_0;
 		csiphy->formats = csiphy_formats_8x96;
 		csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x96);
-	} else if (camss->version == CAMSS_845 ||
-		   camss->version == CAMSS_8250) {
+	} else if (camss->res->version == CAMSS_845 ||
+		   camss->res->version == CAMSS_8250) {
 		csiphy->ops = &csiphy_ops_3ph_1_0;
 		csiphy->formats = csiphy_formats_sdm845;
 		csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845);
@@ -581,8 +581,8 @@ int msm_csiphy_subdev_init(struct camss *camss,
 	if (IS_ERR(csiphy->base))
 		return PTR_ERR(csiphy->base);
 
-	if (camss->version == CAMSS_8x16 ||
-	    camss->version == CAMSS_8x96) {
+	if (camss->res->version == CAMSS_8x16 ||
+	    camss->res->version == CAMSS_8x96) {
 		csiphy->base_clk_mux =
 			devm_platform_ioremap_resource_byname(pdev, res->reg[1]);
 		if (IS_ERR(csiphy->base_clk_mux))
@@ -664,7 +664,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
 		    !strcmp(clock->name, "csiphy5_timer"))
 			csiphy->rate_set[i] = true;
 
-		if (camss->version == CAMSS_660 &&
+		if (camss->res->version == CAMSS_660 &&
 		    (!strcmp(clock->name, "csi0_phy") ||
 		     !strcmp(clock->name, "csi1_phy") ||
 		     !strcmp(clock->name, "csi2_phy")))
diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c
index 61765b874b9a2..0a58d2cf45007 100644
--- a/drivers/media/platform/qcom/camss/camss-ispif.c
+++ b/drivers/media/platform/qcom/camss/camss-ispif.c
@@ -270,7 +270,7 @@ static int ispif_vfe_reset(struct ispif_device *ispif, u8 vfe_id)
 	unsigned long time;
 	u32 val;
 
-	if (vfe_id > (camss->vfe_num - 1)) {
+	if (vfe_id > (camss->res->vfe_num - 1)) {
 		dev_err(camss->dev,
 			"Error: asked reset for invalid VFE%d\n", vfe_id);
 		return -ENOENT;
@@ -829,8 +829,8 @@ static int ispif_set_stream(struct v4l2_subdev *sd, int enable)
 		ispif_select_csid(ispif, intf, csid, vfe, 1);
 		ispif_select_cid(ispif, intf, cid, vfe, 1);
 		ispif_config_irq(ispif, intf, vfe, 1);
-		if (camss->version == CAMSS_8x96 ||
-		    camss->version == CAMSS_660)
+		if (camss->res->version == CAMSS_8x96 ||
+		    camss->res->version == CAMSS_660)
 			ispif_config_pack(ispif,
 					  line->fmt[MSM_ISPIF_PAD_SINK].code,
 					  intf, cid, vfe, 1);
@@ -847,8 +847,8 @@ static int ispif_set_stream(struct v4l2_subdev *sd, int enable)
 			return ret;
 
 		mutex_lock(&ispif->config_lock);
-		if (camss->version == CAMSS_8x96 ||
-		    camss->version == CAMSS_660)
+		if (camss->res->version == CAMSS_8x96 ||
+		    camss->res->version == CAMSS_660)
 			ispif_config_pack(ispif,
 					  line->fmt[MSM_ISPIF_PAD_SINK].code,
 					  intf, cid, vfe, 0);
@@ -1109,10 +1109,10 @@ int msm_ispif_subdev_init(struct camss *camss,
 	ispif->camss = camss;
 
 	/* Number of ISPIF lines - same as number of CSID hardware modules */
-	if (camss->version == CAMSS_8x16)
+	if (camss->res->version == CAMSS_8x16)
 		ispif->line_num = 2;
-	else if (camss->version == CAMSS_8x96 ||
-		 camss->version == CAMSS_660)
+	else if (camss->res->version == CAMSS_8x96 ||
+		 camss->res->version == CAMSS_660)
 		ispif->line_num = 4;
 	else
 		return -EINVAL;
@@ -1126,12 +1126,12 @@ int msm_ispif_subdev_init(struct camss *camss,
 		ispif->line[i].ispif = ispif;
 		ispif->line[i].id = i;
 
-		if (camss->version == CAMSS_8x16) {
+		if (camss->res->version == CAMSS_8x16) {
 			ispif->line[i].formats = ispif_formats_8x16;
 			ispif->line[i].nformats =
 					ARRAY_SIZE(ispif_formats_8x16);
-		} else if (camss->version == CAMSS_8x96 ||
-			   camss->version == CAMSS_660) {
+		} else if (camss->res->version == CAMSS_8x96 ||
+			   camss->res->version == CAMSS_660) {
 			ispif->line[i].formats = ispif_formats_8x96;
 			ispif->line[i].nformats =
 					ARRAY_SIZE(ispif_formats_8x96);
@@ -1159,11 +1159,11 @@ int msm_ispif_subdev_init(struct camss *camss,
 	ispif->irq = ret;
 	snprintf(ispif->irq_name, sizeof(ispif->irq_name), "%s_%s",
 		 dev_name(dev), MSM_ISPIF_NAME);
-	if (camss->version == CAMSS_8x16)
+	if (camss->res->version == CAMSS_8x16)
 		ret = devm_request_irq(dev, ispif->irq, ispif_isr_8x16,
 			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
-	else if (camss->version == CAMSS_8x96 ||
-		 camss->version == CAMSS_660)
+	else if (camss->res->version == CAMSS_8x96 ||
+		 camss->res->version == CAMSS_660)
 		ret = devm_request_irq(dev, ispif->irq, ispif_isr_8x96,
 			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
 	else
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
index ae9137633c301..9905bb06b3823 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
@@ -676,7 +676,7 @@ static void vfe_pm_domain_off(struct vfe_device *vfe)
 {
 	struct camss *camss = vfe->camss;
 
-	if (vfe->id >= camss->vfe_num)
+	if (vfe->id >= camss->res->vfe_num)
 		return;
 
 	device_link_del(camss->genpd_link[vfe->id]);
@@ -691,7 +691,7 @@ static int vfe_pm_domain_on(struct vfe_device *vfe)
 	struct camss *camss = vfe->camss;
 	enum vfe_line_id id = vfe->id;
 
-	if (id >= camss->vfe_num)
+	if (id >= camss->res->vfe_num)
 		return 0;
 
 	camss->genpd_link[id] = device_link_add(camss->dev, camss->genpd[id],
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
index a64d660abc538..80338efceb9e1 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
@@ -502,7 +502,7 @@ static void vfe_pm_domain_off(struct vfe_device *vfe)
 {
 	struct camss *camss = vfe->camss;
 
-	if (vfe->id >= camss->vfe_num)
+	if (vfe->id >= camss->res->vfe_num)
 		return;
 
 	device_link_del(camss->genpd_link[vfe->id]);
@@ -517,7 +517,7 @@ static int vfe_pm_domain_on(struct vfe_device *vfe)
 	struct camss *camss = vfe->camss;
 	enum vfe_line_id id = vfe->id;
 
-	if (id >= camss->vfe_num)
+	if (id >= camss->res->vfe_num)
 		return 0;
 
 	camss->genpd_link[id] = device_link_add(camss->dev, camss->genpd[id],
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index b021f81cef123..526dd4ab343fe 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -170,7 +170,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
 {
 	struct vfe_device *vfe = to_vfe(line);
 
-	if (vfe->camss->version == CAMSS_8x16)
+	if (vfe->camss->res->version == CAMSS_8x16)
 		switch (sink_code) {
 		case MEDIA_BUS_FMT_YUYV8_1X16:
 		{
@@ -218,10 +218,10 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
 
 			return sink_code;
 		}
-	else if (vfe->camss->version == CAMSS_8x96 ||
-		 vfe->camss->version == CAMSS_660 ||
-		 vfe->camss->version == CAMSS_845 ||
-		 vfe->camss->version == CAMSS_8250)
+	else if (vfe->camss->res->version == CAMSS_8x96 ||
+		 vfe->camss->res->version == CAMSS_660 ||
+		 vfe->camss->res->version == CAMSS_845 ||
+		 vfe->camss->res->version == CAMSS_8250)
 		switch (sink_code) {
 		case MEDIA_BUS_FMT_YUYV8_1X16:
 		{
@@ -1286,7 +1286,7 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
 	int i, j;
 	int ret;
 
-	switch (camss->version) {
+	switch (camss->res->version) {
 	case CAMSS_8x16:
 		vfe->ops = &vfe_ops_4_1;
 		break;
@@ -1392,7 +1392,7 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
 		init_completion(&l->output.sof);
 		init_completion(&l->output.reg_update);
 
-		if (camss->version == CAMSS_8x16) {
+		if (camss->res->version == CAMSS_8x16) {
 			if (i == VFE_LINE_PIX) {
 				l->formats = formats_pix_8x16;
 				l->nformats = ARRAY_SIZE(formats_pix_8x16);
@@ -1400,8 +1400,8 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
 				l->formats = formats_rdi_8x16;
 				l->nformats = ARRAY_SIZE(formats_rdi_8x16);
 			}
-		} else if (camss->version == CAMSS_8x96 ||
-			   camss->version == CAMSS_660) {
+		} else if (camss->res->version == CAMSS_8x96 ||
+			   camss->res->version == CAMSS_660) {
 			if (i == VFE_LINE_PIX) {
 				l->formats = formats_pix_8x96;
 				l->nformats = ARRAY_SIZE(formats_pix_8x96);
@@ -1409,8 +1409,8 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
 				l->formats = formats_rdi_8x96;
 				l->nformats = ARRAY_SIZE(formats_rdi_8x96);
 			}
-		} else if (camss->version == CAMSS_845 ||
-			   camss->version == CAMSS_8250) {
+		} else if (camss->res->version == CAMSS_845 ||
+			   camss->res->version == CAMSS_8250) {
 			l->formats = formats_rdi_845;
 			l->nformats = ARRAY_SIZE(formats_rdi_845);
 		} else {
@@ -1542,8 +1542,8 @@ int msm_vfe_register_entities(struct vfe_device *vfe,
 		}
 
 		video_out->ops = &vfe->video_ops;
-		if (vfe->camss->version == CAMSS_845 ||
-		    vfe->camss->version == CAMSS_8250)
+		if (vfe->camss->res->version == CAMSS_845 ||
+		    vfe->camss->res->version == CAMSS_8250)
 			video_out->bpl_alignment = 16;
 		else
 			video_out->bpl_alignment = 8;
diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 1d686aef92eaf..46a89b5f6c171 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -1006,7 +1006,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 
 	mutex_init(&video->lock);
 
-	if (video->camss->version == CAMSS_8x16) {
+	if (video->camss->res->version == CAMSS_8x16) {
 		if (is_pix) {
 			video->formats = formats_pix_8x16;
 			video->nformats = ARRAY_SIZE(formats_pix_8x16);
@@ -1014,8 +1014,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 			video->formats = formats_rdi_8x16;
 			video->nformats = ARRAY_SIZE(formats_rdi_8x16);
 		}
-	} else if (video->camss->version == CAMSS_8x96 ||
-		   video->camss->version == CAMSS_660) {
+	} else if (video->camss->res->version == CAMSS_8x96 ||
+		   video->camss->res->version == CAMSS_660) {
 		if (is_pix) {
 			video->formats = formats_pix_8x96;
 			video->nformats = ARRAY_SIZE(formats_pix_8x96);
@@ -1023,8 +1023,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 			video->formats = formats_rdi_8x96;
 			video->nformats = ARRAY_SIZE(formats_rdi_8x96);
 		}
-	}  else if (video->camss->version == CAMSS_845 ||
-		    video->camss->version == CAMSS_8250) {
+	}  else if (video->camss->res->version == CAMSS_845 ||
+		    video->camss->res->version == CAMSS_8250) {
 		video->formats = formats_rdi_845;
 		video->nformats = ARRAY_SIZE(formats_rdi_845);
 	} else {
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 9a15f2253aa20..c8a2571e664fe 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1005,7 +1005,7 @@ int camss_pm_domain_on(struct camss *camss, int id)
 {
 	int ret = 0;
 
-	if (id < camss->vfe_num) {
+	if (id < camss->res->vfe_num) {
 		struct vfe_device *vfe = &camss->vfe[id];
 
 		ret = vfe->ops->pm_domain_on(vfe);
@@ -1016,7 +1016,7 @@ int camss_pm_domain_on(struct camss *camss, int id)
 
 void camss_pm_domain_off(struct camss *camss, int id)
 {
-	if (id < camss->vfe_num) {
+	if (id < camss->res->vfe_num) {
 		struct vfe_device *vfe = &camss->vfe[id];
 
 		vfe->ops->pm_domain_off(vfe);
@@ -1125,7 +1125,7 @@ static int camss_init_subdevices(struct camss *camss)
 	unsigned int i;
 	int ret;
 
-	for (i = 0; i < camss->csiphy_num; i++) {
+	for (i = 0; i < camss->res->csiphy_num; i++) {
 		ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
 					     &res->csiphy_res[i], i);
 		if (ret < 0) {
@@ -1137,7 +1137,7 @@ static int camss_init_subdevices(struct camss *camss)
 	}
 
 	/* note: SM8250 requires VFE to be initialized before CSID */
-	for (i = 0; i < camss->vfe_num + camss->vfe_lite_num; i++) {
+	for (i = 0; i < camss->res->vfe_total_num; i++) {
 		ret = msm_vfe_subdev_init(camss, &camss->vfe[i],
 					  &res->vfe_res[i], i);
 		if (ret < 0) {
@@ -1147,7 +1147,7 @@ static int camss_init_subdevices(struct camss *camss)
 		}
 	}
 
-	for (i = 0; i < camss->csid_num; i++) {
+	for (i = 0; i < camss->res->csid_num; i++) {
 		ret = msm_csid_subdev_init(camss, &camss->csid[i],
 					   &res->csid_res[i], i);
 		if (ret < 0) {
@@ -1179,7 +1179,7 @@ static int camss_register_entities(struct camss *camss)
 	int i, j, k;
 	int ret;
 
-	for (i = 0; i < camss->csiphy_num; i++) {
+	for (i = 0; i < camss->res->csiphy_num; i++) {
 		ret = msm_csiphy_register_entity(&camss->csiphy[i],
 						 &camss->v4l2_dev);
 		if (ret < 0) {
@@ -1190,7 +1190,7 @@ static int camss_register_entities(struct camss *camss)
 		}
 	}
 
-	for (i = 0; i < camss->csid_num; i++) {
+	for (i = 0; i < camss->res->csid_num; i++) {
 		ret = msm_csid_register_entity(&camss->csid[i],
 					       &camss->v4l2_dev);
 		if (ret < 0) {
@@ -1209,7 +1209,7 @@ static int camss_register_entities(struct camss *camss)
 		goto err_reg_ispif;
 	}
 
-	for (i = 0; i < camss->vfe_num + camss->vfe_lite_num; i++) {
+	for (i = 0; i < camss->res->vfe_total_num; i++) {
 		ret = msm_vfe_register_entities(&camss->vfe[i],
 						&camss->v4l2_dev);
 		if (ret < 0) {
@@ -1220,8 +1220,8 @@ static int camss_register_entities(struct camss *camss)
 		}
 	}
 
-	for (i = 0; i < camss->csiphy_num; i++) {
-		for (j = 0; j < camss->csid_num; j++) {
+	for (i = 0; i < camss->res->csiphy_num; i++) {
+		for (j = 0; j < camss->res->csid_num; j++) {
 			ret = media_create_pad_link(
 				&camss->csiphy[i].subdev.entity,
 				MSM_CSIPHY_PAD_SRC,
@@ -1240,7 +1240,7 @@ static int camss_register_entities(struct camss *camss)
 	}
 
 	if (camss->ispif) {
-		for (i = 0; i < camss->csid_num; i++) {
+		for (i = 0; i < camss->res->csid_num; i++) {
 			for (j = 0; j < camss->ispif->line_num; j++) {
 				ret = media_create_pad_link(
 					&camss->csid[i].subdev.entity,
@@ -1260,7 +1260,7 @@ static int camss_register_entities(struct camss *camss)
 		}
 
 		for (i = 0; i < camss->ispif->line_num; i++)
-			for (k = 0; k < camss->vfe_num; k++)
+			for (k = 0; k < camss->res->vfe_num; k++)
 				for (j = 0; j < camss->vfe[k].line_num; j++) {
 					struct v4l2_subdev *ispif = &camss->ispif->line[i].subdev;
 					struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
@@ -1280,8 +1280,8 @@ static int camss_register_entities(struct camss *camss)
 					}
 				}
 	} else {
-		for (i = 0; i < camss->csid_num; i++)
-			for (k = 0; k < camss->vfe_num + camss->vfe_lite_num; k++)
+		for (i = 0; i < camss->res->csid_num; i++)
+			for (k = 0; k < camss->res->vfe_total_num; k++)
 				for (j = 0; j < camss->vfe[k].line_num; j++) {
 					struct v4l2_subdev *csid = &camss->csid[i].subdev;
 					struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
@@ -1305,7 +1305,7 @@ static int camss_register_entities(struct camss *camss)
 	return 0;
 
 err_link:
-	i = camss->vfe_num + camss->vfe_lite_num;
+	i = camss->res->vfe_total_num;
 err_reg_vfe:
 	for (i--; i >= 0; i--)
 		msm_vfe_unregister_entities(&camss->vfe[i]);
@@ -1313,12 +1313,12 @@ static int camss_register_entities(struct camss *camss)
 err_reg_ispif:
 	msm_ispif_unregister_entities(camss->ispif);
 
-	i = camss->csid_num;
+	i = camss->res->csid_num;
 err_reg_csid:
 	for (i--; i >= 0; i--)
 		msm_csid_unregister_entity(&camss->csid[i]);
 
-	i = camss->csiphy_num;
+	i = camss->res->csiphy_num;
 err_reg_csiphy:
 	for (i--; i >= 0; i--)
 		msm_csiphy_unregister_entity(&camss->csiphy[i]);
@@ -1336,15 +1336,15 @@ static void camss_unregister_entities(struct camss *camss)
 {
 	unsigned int i;
 
-	for (i = 0; i < camss->csiphy_num; i++)
+	for (i = 0; i < camss->res->csiphy_num; i++)
 		msm_csiphy_unregister_entity(&camss->csiphy[i]);
 
-	for (i = 0; i < camss->csid_num; i++)
+	for (i = 0; i < camss->res->csid_num; i++)
 		msm_csid_unregister_entity(&camss->csid[i]);
 
 	msm_ispif_unregister_entities(camss->ispif);
 
-	for (i = 0; i < camss->vfe_num + camss->vfe_lite_num; i++)
+	for (i = 0; i < camss->res->vfe_total_num; i++)
 		msm_vfe_unregister_entities(&camss->vfe[i]);
 }
 
@@ -1463,7 +1463,7 @@ static int camss_configure_pd(struct camss *camss)
 		}
 	}
 
-	if (i > camss->vfe_num) {
+	if (i > camss->res->vfe_num) {
 		camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1],
 							   DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
 							   DL_FLAG_RPM_ACTIVE);
@@ -1509,7 +1509,8 @@ static int camss_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct camss *camss;
-	int num_subdevs, ret;
+	int num_subdevs;
+	int ret;
 
 	camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL);
 	if (!camss)
@@ -1523,59 +1524,24 @@ static int camss_probe(struct platform_device *pdev)
 	camss->dev = dev;
 	platform_set_drvdata(pdev, camss);
 
-	if (of_device_is_compatible(dev->of_node, "qcom,msm8916-camss")) {
-		camss->version = CAMSS_8x16;
-		camss->csiphy_num = 2;
-		camss->csid_num = 2;
-		camss->vfe_num = 1;
-	} else if (of_device_is_compatible(dev->of_node,
-					   "qcom,msm8996-camss")) {
-		camss->version = CAMSS_8x96;
-		camss->csiphy_num = 3;
-		camss->csid_num = 4;
-		camss->vfe_num = 2;
-	} else if (of_device_is_compatible(dev->of_node,
-					   "qcom,sdm660-camss")) {
-		camss->version = CAMSS_660;
-		camss->csiphy_num = 3;
-		camss->csid_num = 4;
-		camss->vfe_num = 2;
-	} else if (of_device_is_compatible(dev->of_node,
-					   "qcom,sdm845-camss")) {
-		camss->version = CAMSS_845;
-		camss->csiphy_num = 4;
-		camss->csid_num = 3;
-		camss->vfe_num = 2;
-		camss->vfe_lite_num = 1;
-	} else if (of_device_is_compatible(dev->of_node,
-					   "qcom,sm8250-camss")) {
-		camss->version = CAMSS_8250;
-		camss->csiphy_num = 6;
-		camss->csid_num = 4;
-		camss->vfe_num = 2;
-		camss->vfe_lite_num = 2;
-	} else {
-		return -EINVAL;
-	}
-
-	camss->csiphy = devm_kcalloc(dev, camss->csiphy_num,
+	camss->csiphy = devm_kcalloc(dev, camss->res->csiphy_num,
 				     sizeof(*camss->csiphy), GFP_KERNEL);
 	if (!camss->csiphy)
 		return -ENOMEM;
 
-	camss->csid = devm_kcalloc(dev, camss->csid_num, sizeof(*camss->csid),
+	camss->csid = devm_kcalloc(dev, camss->res->csid_num, sizeof(*camss->csid),
 				   GFP_KERNEL);
 	if (!camss->csid)
 		return -ENOMEM;
 
-	if (camss->version == CAMSS_8x16 ||
-	    camss->version == CAMSS_8x96) {
+	if (camss->res->version == CAMSS_8x16 ||
+	    camss->res->version == CAMSS_8x96) {
 		camss->ispif = devm_kcalloc(dev, 1, sizeof(*camss->ispif), GFP_KERNEL);
 		if (!camss->ispif)
 			return -ENOMEM;
 	}
 
-	camss->vfe = devm_kcalloc(dev, camss->vfe_num + camss->vfe_lite_num,
+	camss->vfe = devm_kcalloc(dev, camss->res->vfe_total_num,
 				  sizeof(*camss->vfe), GFP_KERNEL);
 	if (!camss->vfe)
 		return -ENOMEM;
@@ -1675,7 +1641,7 @@ void camss_delete(struct camss *camss)
 	if (camss->genpd_num == 1)
 		return;
 
-	if (camss->genpd_num > camss->vfe_num)
+	if (camss->genpd_num > camss->res->vfe_num)
 		device_link_del(camss->genpd_link[camss->genpd_num - 1]);
 
 	for (i = 0; i < camss->genpd_num; i++)
@@ -1701,38 +1667,65 @@ static void camss_remove(struct platform_device *pdev)
 }
 
 static const struct camss_resources msm8916_resources = {
+	.version = CAMSS_8x16,
 	.csiphy_res = csiphy_res_8x16,
 	.csid_res = csid_res_8x16,
 	.ispif_res = &ispif_res_8x16,
 	.vfe_res = vfe_res_8x16,
+	.csiphy_num = ARRAY_SIZE(csiphy_res_8x16),
+	.csid_num = ARRAY_SIZE(csid_res_8x16),
+	.vfe_num = ARRAY_SIZE(vfe_res_8x16),
+	.vfe_total_num = ARRAY_SIZE(vfe_res_8x16),
 };
 
 static const struct camss_resources msm8996_resources = {
+	.version = CAMSS_8x96,
 	.csiphy_res = csiphy_res_8x96,
 	.csid_res = csid_res_8x96,
 	.ispif_res = &ispif_res_8x96,
 	.vfe_res = vfe_res_8x96,
+	.csiphy_num = ARRAY_SIZE(csiphy_res_8x96),
+	.csid_num = ARRAY_SIZE(csid_res_8x96),
+	.vfe_num = ARRAY_SIZE(vfe_res_8x96),
+	.vfe_total_num = ARRAY_SIZE(vfe_res_8x96),
 };
 
 static const struct camss_resources sdm660_resources = {
+	.version = CAMSS_660,
 	.csiphy_res = csiphy_res_660,
 	.csid_res = csid_res_660,
 	.ispif_res = &ispif_res_660,
 	.vfe_res = vfe_res_660,
+	.csiphy_num = ARRAY_SIZE(csiphy_res_660),
+	.csid_num = ARRAY_SIZE(csid_res_660),
+	.vfe_num = ARRAY_SIZE(vfe_res_660),
+	.vfe_total_num = ARRAY_SIZE(vfe_res_660),
 };
 
 static const struct camss_resources sdm845_resources = {
+	.version = CAMSS_845,
 	.csiphy_res = csiphy_res_845,
 	.csid_res = csid_res_845,
 	.vfe_res = vfe_res_845,
+	.csiphy_num = ARRAY_SIZE(csiphy_res_845),
+	.csid_num = ARRAY_SIZE(csid_res_845),
+	.vfe_num = 2,
+	.vfe_lite_num = 1,
+	.vfe_total_num = 3,
 };
 
 static const struct camss_resources sm8250_resources = {
+	.version = CAMSS_8250,
 	.csiphy_res = csiphy_res_8250,
 	.csid_res = csid_res_8250,
 	.vfe_res = vfe_res_8250,
 	.icc_res = icc_res_sm8250,
 	.icc_path_num = ARRAY_SIZE(icc_res_sm8250),
+	.csiphy_num = ARRAY_SIZE(csiphy_res_8250),
+	.csid_num = ARRAY_SIZE(csid_res_8250),
+	.vfe_num = 2,
+	.vfe_lite_num = 2,
+	.vfe_total_num = 4,
 };
 
 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 988c313b63551..dd8c58d349685 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -80,27 +80,28 @@ enum icc_count {
 };
 
 struct camss_resources {
+	enum camss_version version;
 	const struct resources *csiphy_res;
 	const struct resources *csid_res;
 	const struct resources *ispif_res;
 	const struct resources *vfe_res;
 	const struct resources_icc *icc_res;
 	const int icc_path_num;
+	int csiphy_num;
+	int csid_num;
+	int vfe_num;
+	int vfe_lite_num;
+	int vfe_total_num;
 };
 
 struct camss {
-	enum camss_version version;
 	struct v4l2_device v4l2_dev;
 	struct v4l2_async_notifier notifier;
 	struct media_device media_dev;
 	struct device *dev;
-	int csiphy_num;
 	struct csiphy_device *csiphy;
-	int csid_num;
 	struct csid_device *csid;
 	struct ispif_device *ispif;
-	int vfe_num;
-	int vfe_lite_num;
 	struct vfe_device *vfe;
 	atomic_t ref_count;
 	int genpd_num;
-- 
2.41.0

[PATCH v3 05/15] media: qcom: camss: Pass line_num from compat resources

[Bryan O'Donoghue]

line_num indicates the number of RDI - raw data interface channels which
are associated with a given IFE/VFE - image/video front end.

On several SoCs the RDI number is not static for each VFE - for example
on sm8250 VFE Lite has four RDIs where regular VFE has three.

Assigning line_num statically in the subdev_init() phase initialises
each VFE to the lower number, meaning in practical terms that we are
lobbing off one RDI on some VFEs.

Interrupt handling uses static for (i = RDI0; i < RDI2; i++) {} in some
of our VFE blocks but this can't work for situations where we have a
mixture of VFE @ 3 RDI and VFE-lite @ 4 RDI blocks.

First step to remediate is to pass line_num from a compat string
controlled data-structure and do so on a per-VFE basis.

Later patches will assign the correct number of RDI blocks per VFE.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../media/platform/qcom/camss/camss-vfe-170.c |  2 --
 .../media/platform/qcom/camss/camss-vfe-4-1.c |  2 --
 .../media/platform/qcom/camss/camss-vfe-4-7.c |  2 --
 .../media/platform/qcom/camss/camss-vfe-4-8.c |  2 --
 .../media/platform/qcom/camss/camss-vfe-480.c |  1 -
 drivers/media/platform/qcom/camss/camss-vfe.c |  5 +++
 drivers/media/platform/qcom/camss/camss.c     | 36 ++++++++++++-------
 drivers/media/platform/qcom/camss/camss.h     |  1 +
 8 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
index 9905bb06b3823..8aa921400ded0 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
@@ -756,8 +756,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 {
 	vfe->isr_ops = vfe_isr_ops_170;
 	vfe->video_ops = vfe_video_ops_170;
-
-	vfe->line_num = VFE_LINE_NUM_GEN2;
 }
 
 const struct vfe_hw_ops vfe_ops_170 = {
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
index bc309f326f519..2911e4126e7ad 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
@@ -992,8 +992,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 	vfe->isr_ops = vfe_isr_ops_gen1;
 	vfe->ops_gen1 = &vfe_ops_gen1_4_1;
 	vfe->video_ops = vfe_video_ops_gen1;
-
-	vfe->line_num = VFE_LINE_NUM_GEN1;
 }
 
 const struct vfe_hw_ops vfe_ops_4_1 = {
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
index 8acd76c9746ba..b65ed0fef595e 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
@@ -1188,8 +1188,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 	vfe->isr_ops = vfe_isr_ops_gen1;
 	vfe->ops_gen1 = &vfe_ops_gen1_4_7;
 	vfe->video_ops = vfe_video_ops_gen1;
-
-	vfe->line_num = VFE_LINE_NUM_GEN1;
 }
 
 const struct vfe_hw_ops vfe_ops_4_7 = {
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
index 3a0167ecf873a..7b3805177f037 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
@@ -1173,8 +1173,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 	vfe->isr_ops = vfe_isr_ops_gen1;
 	vfe->ops_gen1 = &vfe_ops_gen1_4_8;
 	vfe->video_ops = vfe_video_ops_gen1;
-
-	vfe->line_num = VFE_LINE_NUM_GEN1;
 }
 
 const struct vfe_hw_ops vfe_ops_4_8 = {
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
index 80338efceb9e1..b1a07e846e25b 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
@@ -572,7 +572,6 @@ static const struct camss_video_ops vfe_video_ops_480 = {
 static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 {
 	vfe->video_ops = vfe_video_ops_480;
-	vfe->line_num = MAX_VFE_OUTPUT_LINES;
 }
 
 const struct vfe_hw_ops vfe_ops_480 = {
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 526dd4ab343fe..b789b3b2e4cfd 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -1305,6 +1305,11 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
 	default:
 		return -EINVAL;
 	}
+
+	if (!res->line_num)
+		return -EINVAL;
+
+	vfe->line_num = res->line_num;
 	vfe->ops->subdev_init(dev, vfe);
 
 	/* Memory */
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index c8a2571e664fe..ce0d86e45fe48 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -123,7 +123,8 @@ static const struct resources vfe_res_8x16[] = {
 				{ 0 },
 				{ 0 } },
 		.reg = { "vfe0" },
-		.interrupt = { "vfe0" }
+		.interrupt = { "vfe0" },
+		.line_num = VFE_LINE_NUM_GEN1,
 	}
 };
 
@@ -263,7 +264,8 @@ static const struct resources vfe_res_8x96[] = {
 				{ 0 },
 				{ 0 } },
 		.reg = { "vfe0" },
-		.interrupt = { "vfe0" }
+		.interrupt = { "vfe0" },
+		.line_num = VFE_LINE_NUM_GEN1,
 	},
 
 	/* VFE1 */
@@ -281,7 +283,8 @@ static const struct resources vfe_res_8x96[] = {
 				{ 0 },
 				{ 0 } },
 		.reg = { "vfe1" },
-		.interrupt = { "vfe1" }
+		.interrupt = { "vfe1" },
+		.line_num = VFE_LINE_NUM_GEN1,
 	}
 };
 
@@ -442,7 +445,8 @@ static const struct resources vfe_res_660[] = {
 				{ 0 },
 				{ 0 } },
 		.reg = { "vfe0" },
-		.interrupt = { "vfe0" }
+		.interrupt = { "vfe0" },
+		.line_num = VFE_LINE_NUM_GEN1,
 	},
 
 	/* VFE1 */
@@ -463,7 +467,8 @@ static const struct resources vfe_res_660[] = {
 				{ 0 },
 				{ 0 } },
 		.reg = { "vfe1" },
-		.interrupt = { "vfe1" }
+		.interrupt = { "vfe1" },
+		.line_num = VFE_LINE_NUM_GEN1,
 	}
 };
 
@@ -621,7 +626,8 @@ static const struct resources vfe_res_845[] = {
 				{ 19200000, 75000000, 384000000, 538666667 },
 				{ 384000000 } },
 		.reg = { "vfe0" },
-		.interrupt = { "vfe0" }
+		.interrupt = { "vfe0" },
+		.line_num = VFE_LINE_NUM_GEN2,
 	},
 
 	/* VFE1 */
@@ -641,7 +647,8 @@ static const struct resources vfe_res_845[] = {
 				{ 19200000, 75000000, 384000000, 538666667 },
 				{ 384000000 } },
 		.reg = { "vfe1" },
-		.interrupt = { "vfe1" }
+		.interrupt = { "vfe1" },
+		.line_num = VFE_LINE_NUM_GEN2,
 	},
 
 	/* VFE-lite */
@@ -660,7 +667,8 @@ static const struct resources vfe_res_845[] = {
 				{ 19200000, 75000000, 384000000, 538666667 },
 				{ 384000000 } },
 		.reg = { "vfe_lite" },
-		.interrupt = { "vfe_lite" }
+		.interrupt = { "vfe_lite" },
+		.line_num = VFE_LINE_NUM_GEN2,
 	}
 };
 
@@ -787,7 +795,8 @@ static const struct resources vfe_res_8250[] = {
 				{ 0 },
 				{ 0 } },
 		.reg = { "vfe0" },
-		.interrupt = { "vfe0" }
+		.interrupt = { "vfe0" },
+		.line_num = 4,
 	},
 	/* VFE1 */
 	{
@@ -805,7 +814,8 @@ static const struct resources vfe_res_8250[] = {
 				{ 0 },
 				{ 0 } },
 		.reg = { "vfe1" },
-		.interrupt = { "vfe1" }
+		.interrupt = { "vfe1" },
+		.line_num = 4,
 	},
 	/* VFE2 (lite) */
 	{
@@ -822,7 +832,8 @@ static const struct resources vfe_res_8250[] = {
 				{ 400000000, 480000000 },
 				{ 0 } },
 		.reg = { "vfe_lite0" },
-		.interrupt = { "vfe_lite0" }
+		.interrupt = { "vfe_lite0" },
+		.line_num = 4,
 	},
 	/* VFE3 (lite) */
 	{
@@ -839,7 +850,8 @@ static const struct resources vfe_res_8250[] = {
 				{ 400000000, 480000000 },
 				{ 0 } },
 		.reg = { "vfe_lite1" },
-		.interrupt = { "vfe_lite1" }
+		.interrupt = { "vfe_lite1" },
+		.line_num = 4,
 	},
 };
 
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index dd8c58d349685..101ce6e527931 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -48,6 +48,7 @@ struct resources {
 	u32 clock_rate[CAMSS_RES_MAX][CAMSS_RES_MAX];
 	char *reg[CAMSS_RES_MAX];
 	char *interrupt[CAMSS_RES_MAX];
+	u8 line_num;
 };
 
 struct icc_bw_tbl {
-- 
2.41.0

[PATCH v3 06/15] media: qcom: camss: Assign the correct number of RDIs per VFE

[Bryan O'Donoghue]

Each Video Front End - VFE - has a variable number of Raw Data Interfaces -
RDIs associated with it.

The CAMSS code started from a naive implementation where a fixed define was
used as a control in a for(){} loop iterating through RDIs.

That model scales badly. An attempt was made with  VFE_LINE_NUM_GEN2 and
VFE_LINE_NUM_GEN1 to differentiate between SoCs but, the problem with that
is "gen1" and "gen2" have no meaning in the silicon. There is no fixed
constraint in the silicon between VFE and RDI, it is entirely up to the SoC
designers how many VFEs are populated and how many RDIs to associate with
each VFE.

As an example sdm845 has VFE version 175 and sm8250 VFE version 480.
sdm845 has 2 VFEs with 4 RDIs and 1 VFE Lite with 4 RDIs.
sm8250 has 2 VFEs with 3 RDIs and 2 VFE Lite with 4 RDIs.

Clearly then we need a more granular model to capture the necessary data.

The defines have gone away to be replaced with per-SoC data but, we haven't
populated the parameter data with the real values.

Let's call those values out now

msm8916:
1 x VFE
3 x RDI per VFE (not 4)

msm8996:
2 x VFE
3 x RDI per VFE (not 4)

sdm660:
2 x VFE
3 x RDI per VFE (not 4)

sdm845:
2 x VFE
4 x RDI per VFE (not 3)
1 x VFE Lite
4 x RDI per VFE Lite (not 3)

sm8250:
2 x VFE
3 x RDI per VFE (not 4)
2 x VFE Lite
4 x RDI per VFE

This more complex and correct mapping was not possible prior to passing
values via driver data. Now that we have that change in place we can
correctly map VFEs to RDIs for each VFE.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index ce0d86e45fe48..c8b8ad176ee2b 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -124,7 +124,7 @@ static const struct resources vfe_res_8x16[] = {
 				{ 0 } },
 		.reg = { "vfe0" },
 		.interrupt = { "vfe0" },
-		.line_num = VFE_LINE_NUM_GEN1,
+		.line_num = 3,
 	}
 };
 
@@ -265,7 +265,7 @@ static const struct resources vfe_res_8x96[] = {
 				{ 0 } },
 		.reg = { "vfe0" },
 		.interrupt = { "vfe0" },
-		.line_num = VFE_LINE_NUM_GEN1,
+		.line_num = 3,
 	},
 
 	/* VFE1 */
@@ -284,7 +284,7 @@ static const struct resources vfe_res_8x96[] = {
 				{ 0 } },
 		.reg = { "vfe1" },
 		.interrupt = { "vfe1" },
-		.line_num = VFE_LINE_NUM_GEN1,
+		.line_num = 3,
 	}
 };
 
@@ -446,7 +446,7 @@ static const struct resources vfe_res_660[] = {
 				{ 0 } },
 		.reg = { "vfe0" },
 		.interrupt = { "vfe0" },
-		.line_num = VFE_LINE_NUM_GEN1,
+		.line_num = 3,
 	},
 
 	/* VFE1 */
@@ -468,7 +468,7 @@ static const struct resources vfe_res_660[] = {
 				{ 0 } },
 		.reg = { "vfe1" },
 		.interrupt = { "vfe1" },
-		.line_num = VFE_LINE_NUM_GEN1,
+		.line_num = 3,
 	}
 };
 
@@ -627,7 +627,7 @@ static const struct resources vfe_res_845[] = {
 				{ 384000000 } },
 		.reg = { "vfe0" },
 		.interrupt = { "vfe0" },
-		.line_num = VFE_LINE_NUM_GEN2,
+		.line_num = 4,
 	},
 
 	/* VFE1 */
@@ -648,7 +648,7 @@ static const struct resources vfe_res_845[] = {
 				{ 384000000 } },
 		.reg = { "vfe1" },
 		.interrupt = { "vfe1" },
-		.line_num = VFE_LINE_NUM_GEN2,
+		.line_num = 4,
 	},
 
 	/* VFE-lite */
@@ -668,7 +668,7 @@ static const struct resources vfe_res_845[] = {
 				{ 384000000 } },
 		.reg = { "vfe_lite" },
 		.interrupt = { "vfe_lite" },
-		.line_num = VFE_LINE_NUM_GEN2,
+		.line_num = 4,
 	}
 };
 
@@ -796,7 +796,7 @@ static const struct resources vfe_res_8250[] = {
 				{ 0 } },
 		.reg = { "vfe0" },
 		.interrupt = { "vfe0" },
-		.line_num = 4,
+		.line_num = 3,
 	},
 	/* VFE1 */
 	{
@@ -815,7 +815,7 @@ static const struct resources vfe_res_8250[] = {
 				{ 0 } },
 		.reg = { "vfe1" },
 		.interrupt = { "vfe1" },
-		.line_num = 4,
+		.line_num = 3,
 	},
 	/* VFE2 (lite) */
 	{
-- 
2.41.0

[PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Bryan O'Donoghue]

From sdm845 onwards we need to ensure the VFE is powered on prior to
switching on the CSID.

Alternatively we could model up the GDSCs and clocks the CSID needs
without the VFE but, there's a real question of the legitimacy of such a
use-case.

For now drawing a line at sdm845 and switching on the associated VFEs is
a perfectly valid thing to do.

Rather than continually extend out this clause for at least two new SoCs
with this same model - making the vfe_get/vfe_put path start to look
like spaghetti we can encoded the dependency in a helper function.

Use csid_depends_vfe() for this purpose.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../media/platform/qcom/camss/camss-csid.c    | 20 +++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index 08991b070bd61..fd04ed112b564 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid)
 	return 0;
 }
 
+static bool csid_depends_vfe(u32 version)
+{
+	bool ret = false;
+
+	switch (version) {
+	case CAMSS_845:
+	case CAMSS_8250:
+		ret = true;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
 /*
  * csid_set_power - Power on/off CSID module
  * @sd: CSID V4L2 subdevice
@@ -163,7 +179,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
 	int ret = 0;
 
 	if (on) {
-		if (version == CAMSS_8250 || version == CAMSS_845) {
+		if (csid_depends_vfe(version)) {
 			ret = vfe_get(vfe);
 			if (ret < 0)
 				return ret;
@@ -217,7 +233,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
 		regulator_bulk_disable(csid->num_supplies,
 				       csid->supplies);
 		pm_runtime_put_sync(dev);
-		if (version == CAMSS_8250 || version == CAMSS_845)
+		if (csid_depends_vfe(version))
 			vfe_put(vfe);
 	}
 
-- 
2.41.0

[PATCH v3 08/15] media: qcom: camss: Untangle if/else spaghetti in camss

[Bryan O'Donoghue]

I refuse to add another SoC to this convoluted if/else structure. By the
time we added in a third SoC we ought to have transitioned these control
flow strutures to switches.

Adding in another Soc or two will make some of these if statements into
five clause behemoths.

Introduce switches in the obvious places to despaghetiify.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../media/platform/qcom/camss/camss-csid.c    | 16 ++++---
 .../media/platform/qcom/camss/camss-csiphy.c  | 22 ++++++---
 drivers/media/platform/qcom/camss/camss-vfe.c | 45 +++++++++++++------
 .../media/platform/qcom/camss/camss-video.c   | 16 ++++---
 4 files changed, 68 insertions(+), 31 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index fd04ed112b564..5dbbcda5232ac 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -592,15 +592,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
 	csid->camss = camss;
 	csid->id = id;
 
-	if (camss->res->version == CAMSS_8x16) {
+	switch (camss->res->version) {
+	case CAMSS_8x16:
 		csid->ops = &csid_ops_4_1;
-	} else if (camss->res->version == CAMSS_8x96 ||
-		   camss->res->version == CAMSS_660) {
+		break;
+	case CAMSS_8x96:
+	case CAMSS_660:
 		csid->ops = &csid_ops_4_7;
-	} else if (camss->res->version == CAMSS_845 ||
-		   camss->res->version == CAMSS_8250) {
+		break;
+	case CAMSS_845:
+	case CAMSS_8250:
 		csid->ops = &csid_ops_gen2;
-	} else {
+		break;
+	default:
 		return -EINVAL;
 	}
 	csid->ops->subdev_init(csid);
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index 593aec5c97bc2..0e8c2a59ea241 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -557,21 +557,31 @@ int msm_csiphy_subdev_init(struct camss *camss,
 	csiphy->id = id;
 	csiphy->cfg.combo_mode = 0;
 
-	if (camss->res->version == CAMSS_8x16) {
+	switch (camss->res->version) {
+	case CAMSS_8x16:
+	{
 		csiphy->ops = &csiphy_ops_2ph_1_0;
 		csiphy->formats = csiphy_formats_8x16;
 		csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x16);
-	} else if (camss->res->version == CAMSS_8x96 ||
-		   camss->res->version == CAMSS_660) {
+		break;
+	}
+	case CAMSS_8x96:
+	case CAMSS_660:
+	{
 		csiphy->ops = &csiphy_ops_3ph_1_0;
 		csiphy->formats = csiphy_formats_8x96;
 		csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x96);
-	} else if (camss->res->version == CAMSS_845 ||
-		   camss->res->version == CAMSS_8250) {
+		break;
+	}
+	case CAMSS_845:
+	case CAMSS_8250:
+	{
 		csiphy->ops = &csiphy_ops_3ph_1_0;
 		csiphy->formats = csiphy_formats_sdm845;
 		csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845);
-	} else {
+		break;
+	}
+	default:
 		return -EINVAL;
 	}
 
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index b789b3b2e4cfd..8f48401e31cd3 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -170,7 +170,9 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
 {
 	struct vfe_device *vfe = to_vfe(line);
 
-	if (vfe->camss->res->version == CAMSS_8x16)
+	switch (vfe->camss->res->version) {
+	case CAMSS_8x16:
+	{
 		switch (sink_code) {
 		case MEDIA_BUS_FMT_YUYV8_1X16:
 		{
@@ -218,10 +220,13 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
 
 			return sink_code;
 		}
-	else if (vfe->camss->res->version == CAMSS_8x96 ||
-		 vfe->camss->res->version == CAMSS_660 ||
-		 vfe->camss->res->version == CAMSS_845 ||
-		 vfe->camss->res->version == CAMSS_8250)
+		break;
+	}
+	case CAMSS_8x96:
+	case CAMSS_660:
+	case CAMSS_845:
+	case CAMSS_8250:
+	{
 		switch (sink_code) {
 		case MEDIA_BUS_FMT_YUYV8_1X16:
 		{
@@ -281,8 +286,12 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
 
 			return sink_code;
 		}
-	else
-		return 0;
+		break;
+	}
+	default:
+		break;
+	}
+	return 0;
 }
 
 int vfe_reset(struct vfe_device *vfe)
@@ -1397,7 +1406,9 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
 		init_completion(&l->output.sof);
 		init_completion(&l->output.reg_update);
 
-		if (camss->res->version == CAMSS_8x16) {
+		switch (camss->res->version) {
+		case CAMSS_8x16:
+		{
 			if (i == VFE_LINE_PIX) {
 				l->formats = formats_pix_8x16;
 				l->nformats = ARRAY_SIZE(formats_pix_8x16);
@@ -1405,8 +1416,11 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
 				l->formats = formats_rdi_8x16;
 				l->nformats = ARRAY_SIZE(formats_rdi_8x16);
 			}
-		} else if (camss->res->version == CAMSS_8x96 ||
-			   camss->res->version == CAMSS_660) {
+			break;
+		}
+		case CAMSS_8x96:
+		case CAMSS_660:
+		{
 			if (i == VFE_LINE_PIX) {
 				l->formats = formats_pix_8x96;
 				l->nformats = ARRAY_SIZE(formats_pix_8x96);
@@ -1414,11 +1428,16 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
 				l->formats = formats_rdi_8x96;
 				l->nformats = ARRAY_SIZE(formats_rdi_8x96);
 			}
-		} else if (camss->res->version == CAMSS_845 ||
-			   camss->res->version == CAMSS_8250) {
+			break;
+		}
+		case CAMSS_845:
+		case CAMSS_8250:
+		{
 			l->formats = formats_rdi_845;
 			l->nformats = ARRAY_SIZE(formats_rdi_845);
-		} else {
+			break;
+		}
+		default:
 			return -EINVAL;
 		}
 	}
diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 46a89b5f6c171..e695724437ce1 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -1006,7 +1006,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 
 	mutex_init(&video->lock);
 
-	if (video->camss->res->version == CAMSS_8x16) {
+	switch (video->camss->res->version) {
+	case CAMSS_8x16:
 		if (is_pix) {
 			video->formats = formats_pix_8x16;
 			video->nformats = ARRAY_SIZE(formats_pix_8x16);
@@ -1014,8 +1015,9 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 			video->formats = formats_rdi_8x16;
 			video->nformats = ARRAY_SIZE(formats_rdi_8x16);
 		}
-	} else if (video->camss->res->version == CAMSS_8x96 ||
-		   video->camss->res->version == CAMSS_660) {
+		break;
+	case CAMSS_8x96:
+	case CAMSS_660:
 		if (is_pix) {
 			video->formats = formats_pix_8x96;
 			video->nformats = ARRAY_SIZE(formats_pix_8x96);
@@ -1023,11 +1025,13 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 			video->formats = formats_rdi_8x96;
 			video->nformats = ARRAY_SIZE(formats_rdi_8x96);
 		}
-	}  else if (video->camss->res->version == CAMSS_845 ||
-		    video->camss->res->version == CAMSS_8250) {
+		break;
+	case CAMSS_845:
+	case CAMSS_8250:
 		video->formats = formats_rdi_845;
 		video->nformats = ARRAY_SIZE(formats_rdi_845);
-	} else {
+		break;
+	default:
 		ret = -EINVAL;
 		goto error_video_register;
 	}
-- 
2.41.0

[PATCH v3 09/15] media: qcom: camss: Improve error printout on icc_get fail

[Bryan O'Donoghue]

If icc_get() fails print the name of the failing path.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index c8b8ad176ee2b..72a0a9f304bb2 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1504,8 +1504,11 @@ static int camss_icc_get(struct camss *camss)
 	for (i = 0; i < camss->res->icc_path_num; i++) {
 		camss->icc_path[i] = devm_of_icc_get(camss->dev,
 						     icc_res[i].name);
-		if (IS_ERR(camss->icc_path[i]))
+		if (IS_ERR(camss->icc_path[i])) {
+			dev_err(camss->dev, "Unable to icc_get %s\n",
+				icc_res[i].name);
 			return PTR_ERR(camss->icc_path[i]);
+		}
 	}
 
 	return 0;
-- 
2.41.0

[PATCH v3 10/15] media: qcom: camss: Allow clocks vfeN vfe_liteN or vfe_lite

[Bryan O'Donoghue]

The number of Video Front End - VFE or Image Front End - IFE supported
with new SoCs can vary both for the full and lite cases.

For example sdm845 has one vfe_lite and two vfe interfaces with the vfe
clock called simply "vfe_lite" with no integer postfix. sc8280xp has four
vfe and four vfe lite blocks.

We need to support the following clock name formats

- vfeN
- vfe_liteN
- vfe_lite

with N being any reasonably sized integer.

There are two sites in this code which need to do the same thing,
constructing and matching strings with the pattern above, so encapsulate
the logic in one function.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-vfe.c | 22 ++++++++++++++-----
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 8f48401e31cd3..73380e75dbb22 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -437,6 +437,20 @@ void vfe_isr_reset_ack(struct vfe_device *vfe)
 	complete(&vfe->reset_complete);
 }
 
+static int vfe_match_clock_names(struct vfe_device *vfe,
+				 struct camss_clock *clock)
+{
+	char vfe_name[CAMSS_RES_MAX];
+	char vfe_lite_name[CAMSS_RES_MAX];
+
+	snprintf(vfe_name, sizeof(vfe_name), "vfe%d", vfe->id);
+	snprintf(vfe_lite_name, sizeof(vfe_lite_name), "vfe_lite%d", vfe->id);
+
+	return (!strcmp(clock->name, vfe_name) ||
+		!strcmp(clock->name, vfe_lite_name) ||
+		!strcmp(clock->name, "vfe_lite"));
+}
+
 /*
  * vfe_set_clock_rates - Calculate and set clock rates on VFE module
  * @vfe: VFE device
@@ -460,9 +474,7 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
 	for (i = 0; i < vfe->nclocks; i++) {
 		struct camss_clock *clock = &vfe->clock[i];
 
-		if (!strcmp(clock->name, "vfe0") ||
-		    !strcmp(clock->name, "vfe1") ||
-		    !strcmp(clock->name, "vfe_lite")) {
+		if (vfe_match_clock_names(vfe, clock)) {
 			u64 min_rate = 0;
 			long rate;
 
@@ -543,9 +555,7 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
 	for (i = 0; i < vfe->nclocks; i++) {
 		struct camss_clock *clock = &vfe->clock[i];
 
-		if (!strcmp(clock->name, "vfe0") ||
-		    !strcmp(clock->name, "vfe1") ||
-		    !strcmp(clock->name, "vfe_lite")) {
+		if (vfe_match_clock_names(vfe, clock)) {
 			u64 min_rate = 0;
 			unsigned long rate;
 
-- 
2.41.0

[PATCH v3 11/15] media: qcom: camss: Functionally decompose CSIPHY clock lookups

[Bryan O'Donoghue]

The csiphyX_timer and csiX_phy values need not be hard-coded. We can
functionally decompose the string matching inside of a loop.

Static string values are brittle, difficult to extend and not required
anyway since the camss->res->csiphy_num value informs us of the number
of CSIPHYs and hence the set of potential clocks for a given CSIPHY.

In simple terms if we have five CSIPHYs we can have no more and no less
than five csiphy_timer clocks. Similarly csi_phy core clocks have a 1:1
relationship with the PHY they clock.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../media/platform/qcom/camss/camss-csiphy.c  | 37 ++++++++++++-------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index 0e8c2a59ea241..baf78c525fbfc 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -536,6 +536,15 @@ static int csiphy_init_formats(struct v4l2_subdev *sd,
 	return csiphy_set_format(sd, fh ? fh->state : NULL, &format);
 }
 
+static bool csiphy_match_clock_name(const char *clock_name, const char *format,
+				    int index)
+{
+	char name[CAMSS_RES_MAX];
+
+	snprintf(name, sizeof(name), format, index);
+	return !strcmp(clock_name, name);
+}
+
 /*
  * msm_csiphy_subdev_init - Initialize CSIPHY device structure and resources
  * @csiphy: CSIPHY device
@@ -550,7 +559,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
 {
 	struct device *dev = camss->dev;
 	struct platform_device *pdev = to_platform_device(dev);
-	int i, j;
+	int i, j, k;
 	int ret;
 
 	csiphy->camss = camss;
@@ -666,19 +675,19 @@ int msm_csiphy_subdev_init(struct camss *camss,
 		for (j = 0; j < clock->nfreqs; j++)
 			clock->freq[j] = res->clock_rate[i][j];
 
-		if (!strcmp(clock->name, "csiphy0_timer") ||
-		    !strcmp(clock->name, "csiphy1_timer") ||
-		    !strcmp(clock->name, "csiphy2_timer") ||
-		    !strcmp(clock->name, "csiphy3_timer") ||
-		    !strcmp(clock->name, "csiphy4_timer") ||
-		    !strcmp(clock->name, "csiphy5_timer"))
-			csiphy->rate_set[i] = true;
-
-		if (camss->res->version == CAMSS_660 &&
-		    (!strcmp(clock->name, "csi0_phy") ||
-		     !strcmp(clock->name, "csi1_phy") ||
-		     !strcmp(clock->name, "csi2_phy")))
-			csiphy->rate_set[i] = true;
+		for (k = 0; k < camss->res->csiphy_num; k++) {
+			csiphy->rate_set[i] = csiphy_match_clock_name(clock->name,
+								      "csiphy%d_timer", k);
+			if (csiphy->rate_set[i])
+				break;
+
+			if (camss->res->version == CAMSS_660) {
+				csiphy->rate_set[i] = csiphy_match_clock_name(clock->name,
+									      "csi%d_phy", k);
+				if (csiphy->rate_set[i])
+					break;
+			}
+		}
 	}
 
 	return 0;
-- 
2.41.0

[PATCH v3 12/15] media: qcom: camss: Fix support for setting CSIPHY clock name csiphyX

[Bryan O'Donoghue]

Several of our upstream and soon-to-be upstream SoC CAMSS dtsi declare
csiphyX as opposed to the older clock name csiX_phy.

Right now the CAMSS code will fail to set the csiphyX clock even if we have
declared it in our list of clocks. For sdm845 and sm8250 we appear to "get
away" with this error, however on sc8280xp we don't.

The right approach here is to set the clock when it is declared. If a SoC
doesn't require or a SoC driver implementer doesn't think we need, then the
clock ought to simply be omitted from the clock list.

Include csiphyX in the set of permissible strings which will subsequently
lead to the csiphyX clock being set during csiphy_set_clock_rates() phase.

sdm845 and sm8250 will work with the code as-is so I've omitted this from a
suggested Fixes list.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index baf78c525fbfc..d9c751f457703 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -687,6 +687,10 @@ int msm_csiphy_subdev_init(struct camss *camss,
 				if (csiphy->rate_set[i])
 					break;
 			}
+
+			csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, "csiphy%d", k);
+			if (csiphy->rate_set[i])
+				break;
 		}
 	}
 
-- 
2.41.0

[PATCH v3 13/15] media: qcom: camss: Support RDI3 for VFE 17x

[Bryan O'Donoghue]

Some VFEs have four RDIs apiece. Right now the ISR code has a hard-coded
value which tops-out at RDI2 meaning only three RDIs can be utilised in
practice.

Extend out the various routines in camss-vfe-17x.c to support the higher
RDI count.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-vfe-170.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
index 8aa921400ded0..c2487a8196db7 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
@@ -210,7 +210,8 @@ static void vfe_global_reset(struct vfe_device *vfe)
 			 GLOBAL_RESET_CMD_IDLE_CGC	|
 			 GLOBAL_RESET_CMD_RDI0		|
 			 GLOBAL_RESET_CMD_RDI1		|
-			 GLOBAL_RESET_CMD_RDI2;
+			 GLOBAL_RESET_CMD_RDI2		|
+			 GLOBAL_RESET_CMD_RDI3;
 
 	writel_relaxed(BIT(31), vfe->base + VFE_IRQ_MASK_0);
 
@@ -344,7 +345,7 @@ static void vfe_violation_read(struct vfe_device *vfe)
 static irqreturn_t vfe_isr(int irq, void *dev)
 {
 	struct vfe_device *vfe = dev;
-	u32 status0, status1, vfe_bus_status[3];
+	u32 status0, status1, vfe_bus_status[VFE_LINE_NUM_MAX];
 	int i, wm;
 
 	status0 = readl_relaxed(vfe->base + VFE_IRQ_STATUS_0);
@@ -353,7 +354,7 @@ static irqreturn_t vfe_isr(int irq, void *dev)
 	writel_relaxed(status0, vfe->base + VFE_IRQ_CLEAR_0);
 	writel_relaxed(status1, vfe->base + VFE_IRQ_CLEAR_1);
 
-	for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++) {
+	for (i = VFE_LINE_RDI0; i < vfe->line_num; i++) {
 		vfe_bus_status[i] = readl_relaxed(vfe->base + VFE_BUS_IRQ_STATUS(i));
 		writel_relaxed(vfe_bus_status[i], vfe->base + VFE_BUS_IRQ_CLEAR(i));
 	}
@@ -367,11 +368,11 @@ static irqreturn_t vfe_isr(int irq, void *dev)
 	if (status0 & STATUS_0_RESET_ACK)
 		vfe->isr_ops.reset_ack(vfe);
 
-	for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++)
+	for (i = VFE_LINE_RDI0; i < vfe->line_num; i++)
 		if (status0 & STATUS_0_RDI_REG_UPDATE(i))
 			vfe->isr_ops.reg_update(vfe, i);
 
-	for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++)
+	for (i = VFE_LINE_RDI0; i < vfe->line_num; i++)
 		if (status0 & STATUS_1_RDI_SOF(i))
 			vfe->isr_ops.sof(vfe, i);
 
-- 
2.41.0

[PATCH v3 14/15] media: qcom: camss: Convert vfe_disable() from int to void

[Bryan O'Donoghue]

No version of vfe_disable() currently returns nor has ever as near as I
can discern ever returned a meaningful error code.

Convert from the unused int return to void.

Fixes: 633b388f85c5 ("media: camss: Refactor VFE HW version support")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-vfe-170.c  | 5 ++---
 drivers/media/platform/qcom/camss/camss-vfe-480.c  | 5 ++---
 drivers/media/platform/qcom/camss/camss-vfe-gen1.c | 5 ++---
 drivers/media/platform/qcom/camss/camss-vfe-gen1.h | 3 +--
 drivers/media/platform/qcom/camss/camss-vfe.c      | 5 +----
 drivers/media/platform/qcom/camss/camss-vfe.h      | 2 +-
 6 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
index c2487a8196db7..b6b425ac2e867 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
@@ -562,9 +562,8 @@ static int vfe_enable(struct vfe_line *line)
  * vfe_disable - Disable streaming on VFE line
  * @line: VFE line
  *
- * Return 0 on success or a negative error code otherwise
  */
-static int vfe_disable(struct vfe_line *line)
+static void vfe_disable(struct vfe_line *line)
 {
 	struct vfe_device *vfe = to_vfe(line);
 
@@ -578,7 +577,7 @@ static int vfe_disable(struct vfe_line *line)
 
 	mutex_unlock(&vfe->stream_lock);
 
-	return 0;
+	return;
 }
 
 /*
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
index b1a07e846e25b..be00a713163e1 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
@@ -397,9 +397,8 @@ static int vfe_enable(struct vfe_line *line)
  * vfe_disable - Disable streaming on VFE line
  * @line: VFE line
  *
- * Return 0 on success or a negative error code otherwise
  */
-static int vfe_disable(struct vfe_line *line)
+static void vfe_disable(struct vfe_line *line)
 {
 	struct vfe_device *vfe = to_vfe(line);
 
@@ -413,7 +412,7 @@ static int vfe_disable(struct vfe_line *line)
 
 	mutex_unlock(&vfe->stream_lock);
 
-	return 0;
+	return;
 }
 
 /*
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-gen1.c b/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
index 239d3d4ac6661..5e8319e565bbc 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
@@ -95,9 +95,8 @@ static int vfe_disable_output(struct vfe_line *line)
  * vfe_gen1_disable - Disable streaming on VFE line
  * @line: VFE line
  *
- * Return 0 on success or a negative error code otherwise
  */
-int vfe_gen1_disable(struct vfe_line *line)
+void vfe_gen1_disable(struct vfe_line *line)
 {
 	struct vfe_device *vfe = to_vfe(line);
 
@@ -114,7 +113,7 @@ int vfe_gen1_disable(struct vfe_line *line)
 
 	mutex_unlock(&vfe->stream_lock);
 
-	return 0;
+	return;
 }
 
 static void vfe_output_init_addrs(struct vfe_device *vfe,
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-gen1.h b/drivers/media/platform/qcom/camss/camss-vfe-gen1.h
index 6d5f9656562c8..b82137a2cb578 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-gen1.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe-gen1.h
@@ -82,9 +82,8 @@ static inline u8 vfe_calc_interp_reso(u16 input, u16 output)
  * vfe_gen1_disable - Disable streaming on VFE line
  * @line: VFE line
  *
- * Return 0 on success or a negative error code otherwise
  */
-int vfe_gen1_disable(struct vfe_line *line);
+void vfe_gen1_disable(struct vfe_line *line);
 
 /*
  * vfe_gen1_enable - Enable VFE module
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 73380e75dbb22..d6c5628d51903 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -766,10 +766,7 @@ static int vfe_set_stream(struct v4l2_subdev *sd, int enable)
 			dev_err(vfe->camss->dev,
 				"Failed to enable vfe outputs\n");
 	} else {
-		ret = vfe->ops->vfe_disable(line);
-		if (ret < 0)
-			dev_err(vfe->camss->dev,
-				"Failed to disable vfe outputs\n");
+		vfe->ops->vfe_disable(line);
 	}
 
 	return ret;
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index cbc314c4e244b..b2f4192576364 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h
@@ -112,7 +112,7 @@ struct vfe_hw_ops {
 	void (*reg_update_clear)(struct vfe_device *vfe,
 				 enum vfe_line_id line_id);
 	void (*subdev_init)(struct device *dev, struct vfe_device *vfe);
-	int (*vfe_disable)(struct vfe_line *line);
+	void (*vfe_disable)(struct vfe_line *line);
 	int (*vfe_enable)(struct vfe_line *line);
 	int (*vfe_halt)(struct vfe_device *vfe);
 	void (*violation_read)(struct vfe_device *vfe);
-- 
2.41.0

[PATCH v3 15/15] media: qcom: camss: Comment CSID dt_id field

[Bryan O'Donoghue]

Digging into the documentation we find that the DT_ID bitfield is used to
map the six bit DT to a two bit ID code. This value is concatenated to the
VC bitfield to create a CID value. DT_ID is the two least significant bits
of CID and VC the most significant bits.

Originally we set dt_id = vc * 4 in and then subsequently set dt_id = vc.

commit 3c4ed72a16bc ("media: camss: sm8250: Virtual channels for CSID")
silently fixed the multiplication by four which would give a better
value for the generated CID without mentioning what was being done or why.

Next up I haplessly changed the value back to "dt_id = vc * 4" since there
didn't appear to be any logic behind it.

Hans asked what the change was for and I honestly couldn't remember the
provenance of it, so I dug in.

Link: https://lore.kernel.org/linux-arm-msm/edd4bf9b-0e1b-883c-1a4d-50f4102c3924@xs4all.nl/

Add a comment so the next hapless programmer doesn't make this same
mistake.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-csid-gen2.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
index 6ba2b10326444..cee50fc87e9de 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
@@ -352,6 +352,11 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
 		phy_sel = csid->phy.csiphy_id;
 
 	if (enable) {
+		/*
+		 * A value caled 'CID' gets generated internal to CAMSS logic
+		 * which is a concatenation of [vc:6 | dt_id:2] hence we reuse
+		 * the least significant two bits of the VC to 'stuff' the CID value.
+		 */
 		u8 dt_id = vc;
 
 		if (tg->enabled) {
-- 
2.41.0

Re: [PATCH v3 01/15] media: qcom: camss: Amalgamate struct resource with struct resource_ispif

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> There is no good reason to differentiate the two resource structures
> here. As part of a general tidyup of the declaration and passing of
> resources within in the CAMSS driver it will be advantageous to have
> one unified resource structure.
> 
> The two structures are very similar anyway thus leading more credence
> still to the argument there should be only one.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v3 04/15] media: qcom: camss: Pass remainder of variables as resources

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> The following variables are being assigned statically based on
> compatible strings in the probe path.
> 
> * enum camss_version version;
> * int csiphy_num;
> * int csid_num;
> * int vfe_num;
> * int vfe_lite_num;
> * int vfe_total_num;
> 
> Migrate those variables to resource parameters passed in on platform
> probe arguments. The one caveat is for VFE it has been necessary to
> intoduce a new variable vfe_total_num to capture the aggregate value of
> vfe_num + vfe_lite_num.
> 
> All the rest of the changes are rote camss->variable to
> camss->res->variable with the parameter tables now populating the listed
> variables.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
I don't see anything immediately wrong. This was long necessary!

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> From sdm845 onwards we need to ensure the VFE is powered on prior to
> switching on the CSID.
And what's the symptom if we fail to ensure this? How can someone
adding support for another platform tell whether the match-list
should be expanded?

> 
> Alternatively we could model up the GDSCs and clocks the CSID needs
> without the VFE but, there's a real question of the legitimacy of such a
> use-case.
> 
> For now drawing a line at sdm845 and switching on the associated VFEs is
> a perfectly valid thing to do.
> 
> Rather than continually extend out this clause for at least two new SoCs
> with this same model - making the vfe_get/vfe_put path start to look
> like spaghetti we can encoded the dependency in a helper function.
> 
> Use csid_depends_vfe() for this purpose.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-csid.c    | 20 +++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 08991b070bd61..fd04ed112b564 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid)
>  	return 0;
>  }
>  
> +static bool csid_depends_vfe(u32 version)
toggle_vfe_before_csid?

> +{
> +	bool ret = false;
> +
> +	switch (version) {
> +	case CAMSS_845:
> +	case CAMSS_8250:
> +		ret = true;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
I'm not sure if it would be okay with like C conventions and
stuff, but this can be made shorter by returning from within
the switch statement

Konrad

Re: [PATCH v3 08/15] media: qcom: camss: Untangle if/else spaghetti in camss

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> I refuse to add another SoC to this convoluted if/else structure. By the
> time we added in a third SoC we ought to have transitioned these control
> flow strutures to switches.
> 
> Adding in another Soc or two will make some of these if statements into
> five clause behemoths.
> 
> Introduce switches in the obvious places to despaghetiify.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v3 09/15] media: qcom: camss: Improve error printout on icc_get fail

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> If icc_get() fails print the name of the failing path.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
Aren't the return messages propagated from

* of_icc_get()
	|_ of_icc_get_by_index()

enough here?

Perhaps we should use dev_err_probe in camss too

Konrad

Re: [PATCH v3 10/15] media: qcom: camss: Allow clocks vfeN vfe_liteN or vfe_lite

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> The number of Video Front End - VFE or Image Front End - IFE supported
> with new SoCs can vary both for the full and lite cases.
> 
> For example sdm845 has one vfe_lite and two vfe interfaces with the vfe
> clock called simply "vfe_lite" with no integer postfix. sc8280xp has four
> vfe and four vfe lite blocks.
> 
> We need to support the following clock name formats
> 
> - vfeN
> - vfe_liteN
> - vfe_lite
> 
> with N being any reasonably sized integer.
> 
> There are two sites in this code which need to do the same thing,
> constructing and matching strings with the pattern above, so encapsulate
> the logic in one function.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss-vfe.c | 22 ++++++++++++++-----
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 8f48401e31cd3..73380e75dbb22 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -437,6 +437,20 @@ void vfe_isr_reset_ack(struct vfe_device *vfe)
>  	complete(&vfe->reset_complete);
>  }
>  
> +static int vfe_match_clock_names(struct vfe_device *vfe,
> +				 struct camss_clock *clock)
> +{
> +	char vfe_name[CAMSS_RES_MAX];
> +	char vfe_lite_name[CAMSS_RES_MAX];
I don't think using the "number of resources" define to define
the maximum length of a resource name is a good idea.

Perhaps we can do:

char vfe_name[5]; /* "vfeX\0" */
char vfe_lite_name[10]; /* "vfe_liteX\0" */

if index > 9
   return INCREASE_THE_BUFFER

Konrad

Re: [PATCH v3 11/15] media: qcom: camss: Functionally decompose CSIPHY clock lookups

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> The csiphyX_timer and csiX_phy values need not be hard-coded. We can
> functionally decompose the string matching inside of a loop.
> 
> Static string values are brittle, difficult to extend and not required
> anyway since the camss->res->csiphy_num value informs us of the number
> of CSIPHYs and hence the set of potential clocks for a given CSIPHY.
> 
> In simple terms if we have five CSIPHYs we can have no more and no less
> than five csiphy_timer clocks. Similarly csi_phy core clocks have a 1:1
> relationship with the PHY they clock.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-csiphy.c  | 37 ++++++++++++-------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index 0e8c2a59ea241..baf78c525fbfc 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -536,6 +536,15 @@ static int csiphy_init_formats(struct v4l2_subdev *sd,
>  	return csiphy_set_format(sd, fh ? fh->state : NULL, &format);
>  }
>  
> +static bool csiphy_match_clock_name(const char *clock_name, const char *format,
> +				    int index)
> +{
> +	char name[CAMSS_RES_MAX];
similar comment to the previous patch about the buf size

> +
> +	snprintf(name, sizeof(name), format, index);
> +	return !strcmp(clock_name, name);
> +}
> +
>  /*
>   * msm_csiphy_subdev_init - Initialize CSIPHY device structure and resources
>   * @csiphy: CSIPHY device
> @@ -550,7 +559,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
>  {
>  	struct device *dev = camss->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
> -	int i, j;
> +	int i, j, k;
>  	int ret;
>  
>  	csiphy->camss = camss;
> @@ -666,19 +675,19 @@ int msm_csiphy_subdev_init(struct camss *camss,
>  		for (j = 0; j < clock->nfreqs; j++)
>  			clock->freq[j] = res->clock_rate[i][j];
>  
> -		if (!strcmp(clock->name, "csiphy0_timer") ||
> -		    !strcmp(clock->name, "csiphy1_timer") ||
> -		    !strcmp(clock->name, "csiphy2_timer") ||
> -		    !strcmp(clock->name, "csiphy3_timer") ||
> -		    !strcmp(clock->name, "csiphy4_timer") ||
> -		    !strcmp(clock->name, "csiphy5_timer"))
> -			csiphy->rate_set[i] = true;
> -
> -		if (camss->res->version == CAMSS_660 &&
> -		    (!strcmp(clock->name, "csi0_phy") ||
> -		     !strcmp(clock->name, "csi1_phy") ||
> -		     !strcmp(clock->name, "csi2_phy")))
> -			csiphy->rate_set[i] = true;
> +		for (k = 0; k < camss->res->csiphy_num; k++) {
> +			csiphy->rate_set[i] = csiphy_match_clock_name(clock->name,
> +								      "csiphy%d_timer", k);
This entire functions is like.. soooo over-engineered

adding something like csiphy_timer_clks and cisphy_clks and stuff
would make this string comparison mess unnecessary

> +			if (csiphy->rate_set[i])
> +				break;
> +
> +			if (camss->res->version == CAMSS_660) {
> +				csiphy->rate_set[i] = csiphy_match_clock_name(clock->name,
> +									      "csi%d_phy", k);
hm, only ratesetting on 660 sounds very sus

Konrad

Re: [PATCH v3 12/15] media: qcom: camss: Fix support for setting CSIPHY clock name csiphyX

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> Several of our upstream and soon-to-be upstream SoC CAMSS dtsi declare
> csiphyX as opposed to the older clock name csiX_phy.
This only reinforces my point about adding like csiphy_clks or so

Konrad
> 
> Right now the CAMSS code will fail to set the csiphyX clock even if we have
> declared it in our list of clocks. For sdm845 and sm8250 we appear to "get
> away" with this error, however on sc8280xp we don't.
> 
> The right approach here is to set the clock when it is declared. If a SoC
> doesn't require or a SoC driver implementer doesn't think we need, then the
> clock ought to simply be omitted from the clock list.
> 
> Include csiphyX in the set of permissible strings which will subsequently
> lead to the csiphyX clock being set during csiphy_set_clock_rates() phase.
> 
> sdm845 and sm8250 will work with the code as-is so I've omitted this from a
> suggested Fixes list.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss-csiphy.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index baf78c525fbfc..d9c751f457703 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -687,6 +687,10 @@ int msm_csiphy_subdev_init(struct camss *camss,
>  				if (csiphy->rate_set[i])
>  					break;
>  			}
> +
> +			csiphy->rate_set[i] = csiphy_match_clock_name(clock->name, "csiphy%d", k);
> +			if (csiphy->rate_set[i])
> +				break;
>  		}
>  	}
>  

Re: [PATCH v3 14/15] media: qcom: camss: Convert vfe_disable() from int to void

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> No version of vfe_disable() currently returns nor has ever as near as I
> can discern ever returned a meaningful error code.
> 
> Convert from the unused int return to void.
> 
> Fixes: 633b388f85c5 ("media: camss: Refactor VFE HW version support")
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
Up the chain, vfe_reset can fail but the error isn't propagated

Konrad

Re: [PATCH v3 15/15] media: qcom: camss: Comment CSID dt_id field

[Konrad Dybcio]

On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> Digging into the documentation we find that the DT_ID bitfield is used to
> map the six bit DT to a two bit ID code. This value is concatenated to the
> VC bitfield to create a CID value. DT_ID is the two least significant bits
> of CID and VC the most significant bits.
> 
> Originally we set dt_id = vc * 4 in and then subsequently set dt_id = vc.
> 
> commit 3c4ed72a16bc ("media: camss: sm8250: Virtual channels for CSID")
> silently fixed the multiplication by four which would give a better
> value for the generated CID without mentioning what was being done or why.
> 
> Next up I haplessly changed the value back to "dt_id = vc * 4" since there
> didn't appear to be any logic behind it.
> 
> Hans asked what the change was for and I honestly couldn't remember the
> provenance of it, so I dug in.
> 
> Link: https://lore.kernel.org/linux-arm-msm/edd4bf9b-0e1b-883c-1a4d-50f4102c3924@xs4all.nl/
> 
> Add a comment so the next hapless programmer doesn't make this same
> mistake.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss-csid-gen2.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> index 6ba2b10326444..cee50fc87e9de 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> @@ -352,6 +352,11 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
>  		phy_sel = csid->phy.csiphy_id;
>  
>  	if (enable) {
> +		/*
> +		 * A value caled 'CID' gets generated internal to CAMSS logic
> +		 * which is a concatenation of [vc:6 | dt_id:2] hence we reuse
> +		 * the least significant two bits of the VC to 'stuff' the CID value.
> +		 */
>  		u8 dt_id = vc;
And where are you discarding the non-2-lsb?

Konrad

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Bryan O'Donoghue]

On 26/08/2023 11:02, Konrad Dybcio wrote:
> On 23.08.2023 12:44, Bryan O'Donoghue wrote:
>>  From sdm845 onwards we need to ensure the VFE is powered on prior to
>> switching on the CSID.
> And what's the symptom if we fail to ensure this? How can someone
> adding support for another platform tell whether the match-list
> should be expanded?

That person has to understand the dependency.

The first version of this patch >= SDM845 would mitigate needing to know 
to expand the list.

Rather than revisit that discussion, I will amend the commit log.

> 
>>
>> Alternatively we could model up the GDSCs and clocks the CSID needs
>> without the VFE but, there's a real question of the legitimacy of such a
>> use-case.
>>
>> For now drawing a line at sdm845 and switching on the associated VFEs is
>> a perfectly valid thing to do.
>>
>> Rather than continually extend out this clause for at least two new SoCs
>> with this same model - making the vfe_get/vfe_put path start to look
>> like spaghetti we can encoded the dependency in a helper function.
>>
>> Use csid_depends_vfe() for this purpose.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   .../media/platform/qcom/camss/camss-csid.c    | 20 +++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
>> index 08991b070bd61..fd04ed112b564 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid)
>>   	return 0;
>>   }
>>   
>> +static bool csid_depends_vfe(u32 version)
> toggle_vfe_before_csid?

If that's clearer np.

>> +{
>> +	bool ret = false;
>> +
>> +	switch (version) {
>> +	case CAMSS_845:
>> +	case CAMSS_8250:
>> +		ret = true;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return ret;
> I'm not sure if it would be okay with like C conventions and
> stuff, but this can be made shorter by returning from within
> the switch statement

Yes but you still need the explicit return at the end of the function or 
from memory at least some of the compiler/static analysis or checkpatch 
stuff - I forget which - will complain.

---
bod

Re: [PATCH v3 09/15] media: qcom: camss: Improve error printout on icc_get fail

[Bryan O'Donoghue]

On 26/08/2023 11:05, Konrad Dybcio wrote:
> On 23.08.2023 12:44, Bryan O'Donoghue wrote:
>> If icc_get() fails print the name of the failing path.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
> Aren't the return messages propagated from
> 
> * of_icc_get()
> 	|_ of_icc_get_by_index()
> 
> enough here?
> 
> Perhaps we should use dev_err_probe in camss too
> 
> Konrad

IDK, I will check

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Konrad Dybcio]

On 26.08.2023 14:01, Bryan O'Donoghue wrote:
> On 26/08/2023 11:02, Konrad Dybcio wrote:
>> On 23.08.2023 12:44, Bryan O'Donoghue wrote:
>>>  From sdm845 onwards we need to ensure the VFE is powered on prior to
>>> switching on the CSID.
>> And what's the symptom if we fail to ensure this? How can someone
>> adding support for another platform tell whether the match-list
>> should be expanded?
> 
> That person has to understand the dependency.
If we need this workaround, there surely must be something that doesn't
work without it, a measurable symptom. What is it?

Konrad

Re: [PATCH v3 10/15] media: qcom: camss: Allow clocks vfeN vfe_liteN or vfe_lite

[Bryan O'Donoghue]

On 26/08/2023 11:08, Konrad Dybcio wrote:
> On 23.08.2023 12:44, Bryan O'Donoghue wrote:
>> The number of Video Front End - VFE or Image Front End - IFE supported
>> with new SoCs can vary both for the full and lite cases.
>>
>> For example sdm845 has one vfe_lite and two vfe interfaces with the vfe
>> clock called simply "vfe_lite" with no integer postfix. sc8280xp has four
>> vfe and four vfe lite blocks.
>>
>> We need to support the following clock name formats
>>
>> - vfeN
>> - vfe_liteN
>> - vfe_lite
>>
>> with N being any reasonably sized integer.
>>
>> There are two sites in this code which need to do the same thing,
>> constructing and matching strings with the pattern above, so encapsulate
>> the logic in one function.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   drivers/media/platform/qcom/camss/camss-vfe.c | 22 ++++++++++++++-----
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index 8f48401e31cd3..73380e75dbb22 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>> @@ -437,6 +437,20 @@ void vfe_isr_reset_ack(struct vfe_device *vfe)
>>   	complete(&vfe->reset_complete);
>>   }
>>   
>> +static int vfe_match_clock_names(struct vfe_device *vfe,
>> +				 struct camss_clock *clock)
>> +{
>> +	char vfe_name[CAMSS_RES_MAX];
>> +	char vfe_lite_name[CAMSS_RES_MAX];
> I don't think using the "number of resources" define to define
> the maximum length of a resource name is a good idea.
> 
> Perhaps we can do:
> 
> char vfe_name[5]; /* "vfeX\0" */
> char vfe_lite_name[10]; /* "vfe_liteX\0" */
> 
> if index > 9
>     return INCREASE_THE_BUFFER
> 
> Konrad

I'm reluctant to fix only the VFE clock name string length in isolation, 
plus I'm aware of another patchset coming down the line from other 
people which will likely address the string length stuff.

But in the interests of consensus I will restrict the length in the helper.

---
bod

Re: [PATCH v3 11/15] media: qcom: camss: Functionally decompose CSIPHY clock lookups

[Bryan O'Donoghue]

On 26/08/2023 11:12, Konrad Dybcio wrote:
>> -			csiphy->rate_set[i] = true;
>> +		for (k = 0; k < camss->res->csiphy_num; k++) {
>> +			csiphy->rate_set[i] = csiphy_match_clock_name(clock->name,
>> +								      "csiphy%d_timer", k);
> This entire functions is like.. soooo over-engineered

I'm going to accept your compliment there.


> adding something like csiphy_timer_clks and cisphy_clks and stuff
> would make this string comparison mess unnecessary

I don't understand your comment.

Having a litany of static comparisons is definitely inferior to a 
generic helper function.

I'm not sure what you are asking/arguing for here.

---
bod

Re: [PATCH v3 12/15] media: qcom: camss: Fix support for setting CSIPHY clock name csiphyX

[Bryan O'Donoghue]

On 26/08/2023 11:13, Konrad Dybcio wrote:
> On 23.08.2023 12:44, Bryan O'Donoghue wrote:
>> Several of our upstream and soon-to-be upstream SoC CAMSS dtsi declare
>> csiphyX as opposed to the older clock name csiX_phy.
> This only reinforces my point about adding like csiphy_clks or so
> 
> Konrad

I really don't understand your point. Could you please restate it ?

---
bod

Re: [PATCH v3 11/15] media: qcom: camss: Functionally decompose CSIPHY clock lookups

[Konrad Dybcio]

On 26.08.2023 14:07, Bryan O'Donoghue wrote:
> On 26/08/2023 11:12, Konrad Dybcio wrote:
>>> -            csiphy->rate_set[i] = true;
>>> +        for (k = 0; k < camss->res->csiphy_num; k++) {
>>> +            csiphy->rate_set[i] = csiphy_match_clock_name(clock->name,
>>> +                                      "csiphy%d_timer", k);
>> This entire functions is like.. soooo over-engineered
> 
> I'm going to accept your compliment there.
> 
> 
>> adding something like csiphy_timer_clks and cisphy_clks and stuff
>> would make this string comparison mess unnecessary
> 
> I don't understand your comment.
> 
> Having a litany of static comparisons is definitely inferior to a generic helper function.

portray this

struct camss_whatever_it_was_called {
	struct clk_bulk_data *csiphy_clks;
	struct clk_bulk_data *csiphy_timer_clks;
	[...]
}

and then

clk_bulk_prepare_enable(csiphy_clks)

etc

instead of weird looping and matching

Konrad

Re: [PATCH v3 12/15] media: qcom: camss: Fix support for setting CSIPHY clock name csiphyX

[Konrad Dybcio]

On 26.08.2023 14:08, Bryan O'Donoghue wrote:
> On 26/08/2023 11:13, Konrad Dybcio wrote:
>> On 23.08.2023 12:44, Bryan O'Donoghue wrote:
>>> Several of our upstream and soon-to-be upstream SoC CAMSS dtsi declare
>>> csiphyX as opposed to the older clock name csiX_phy.
>> This only reinforces my point about adding like csiphy_clks or so
>>
>> Konrad
> 
> I really don't understand your point. Could you please restate it ?
If we categorized the clocks at probe time (these ones go to csiphy, these
ones go to vfe or whatever), name matching like this could be avoided

Konrad

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Bryan O'Donoghue]

On 26/08/2023 13:04, Konrad Dybcio wrote:
>>>>   From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>> switching on the CSID.
>>> And what's the symptom if we fail to ensure this? How can someone
>>> adding support for another platform tell whether the match-list
>>> should be expanded?
>> That person has to understand the dependency.
> If we need this workaround, there surely must be something that doesn't
> work without it, a measurable symptom. What is it?

The CSID lives inside of the VFE therefore the VFE must be power prior 
to the CSID.

The symptom will be .. the CSID doesn't come out of reset.

---
bod

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Konrad Dybcio]

On 26.08.2023 14:12, Bryan O'Donoghue wrote:
> On 26/08/2023 13:04, Konrad Dybcio wrote:
>>>>>   From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>>> switching on the CSID.
>>>> And what's the symptom if we fail to ensure this? How can someone
>>>> adding support for another platform tell whether the match-list
>>>> should be expanded?
>>> That person has to understand the dependency.
>> If we need this workaround, there surely must be something that doesn't
>> work without it, a measurable symptom. What is it?
> 
> The CSID lives inside of the VFE therefore the VFE must be power prior to the CSID.
> 
> The symptom will be .. the CSID doesn't come out of reset.
Good, that's what I needed to know.

Now we can rename that function to something like camss_csid_inside_vfe()
to make it more meaningful

Konrad

Re: [PATCH v3 11/15] media: qcom: camss: Functionally decompose CSIPHY clock lookups

[Bryan O'Donoghue]

On 26/08/2023 13:11, Konrad Dybcio wrote:
>>> adding something like csiphy_timer_clks and cisphy_clks and stuff
>>> would make this string comparison mess unnecessary
>> I don't understand your comment.
>>
>> Having a litany of static comparisons is definitely inferior to a generic helper function.
> portray this
> 
> struct camss_whatever_it_was_called {
> 	struct clk_bulk_data *csiphy_clks;
> 	struct clk_bulk_data *csiphy_timer_clks;
> 	[...]
> }
> 
> and then
> 
> clk_bulk_prepare_enable(csiphy_clks)

Ah would be grateful if you had just said "hey could you try using 
clk_bulk_prepare_enable()"

But, OK.

Thanks !

---
bod

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Bryan O'Donoghue]

On 26/08/2023 13:13, Konrad Dybcio wrote:
> On 26.08.2023 14:12, Bryan O'Donoghue wrote:
>> On 26/08/2023 13:04, Konrad Dybcio wrote:
>>>>>>    From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>>>> switching on the CSID.
>>>>> And what's the symptom if we fail to ensure this? How can someone
>>>>> adding support for another platform tell whether the match-list
>>>>> should be expanded?
>>>> That person has to understand the dependency.
>>> If we need this workaround, there surely must be something that doesn't
>>> work without it, a measurable symptom. What is it?
>>
>> The CSID lives inside of the VFE therefore the VFE must be power prior to the CSID.
>>
>> The symptom will be .. the CSID doesn't come out of reset.
> Good, that's what I needed to know.
> 
> Now we can rename that function to something like camss_csid_inside_vfe()
> to make it more meaningful

If you feel there is a meaningful distinction between 
"csid_depends_vfe()" and "camss_csid_inside_vfe()"

I'm happy to humour you.

---
bod

Re: [PATCH v3 11/15] media: qcom: camss: Functionally decompose CSIPHY clock lookups

[Konrad Dybcio]

On 26.08.2023 14:14, Bryan O'Donoghue wrote:
> On 26/08/2023 13:11, Konrad Dybcio wrote:
>>>> adding something like csiphy_timer_clks and cisphy_clks and stuff
>>>> would make this string comparison mess unnecessary
>>> I don't understand your comment.
>>>
>>> Having a litany of static comparisons is definitely inferior to a generic helper function.
>> portray this
>>
>> struct camss_whatever_it_was_called {
>>     struct clk_bulk_data *csiphy_clks;
>>     struct clk_bulk_data *csiphy_timer_clks;
>>     [...]
>> }
>>
>> and then
>>
>> clk_bulk_prepare_enable(csiphy_clks)
> 
> Ah would be grateful if you had just said "hey could you try using clk_bulk_prepare_enable()"
Right I could have been more clear

Konrad

Re: [PATCH v3 15/15] media: qcom: camss: Comment CSID dt_id field

[Bryan O'Donoghue]

On 26/08/2023 11:18, Konrad Dybcio wrote:
>> +		/*
>> +		 * A value caled 'CID' gets generated internal to CAMSS logic
>> +		 * which is a concatenation of [vc:6 | dt_id:2] hence we reuse
>> +		 * the least significant two bits of the VC to 'stuff' the CID value.
>> +		 */
>>   		u8 dt_id = vc;
> And where are you discarding the non-2-lsb?

At the assignment of dt_id

vc:6
dt_id:2

=>

cid:8 = [vc:6 | dt_id:2]
vc == 00110110
cid = [110110 | 10]

I have no more information what CID is or how the bitfield is populated 
than I have already indicated in the comment/commit log.

---
bod

Re: [PATCH v3 15/15] media: qcom: camss: Comment CSID dt_id field

[Konrad Dybcio]

On 28.08.2023 17:34, Bryan O'Donoghue wrote:
> On 26/08/2023 11:18, Konrad Dybcio wrote:
>>> +        /*
>>> +         * A value caled 'CID' gets generated internal to CAMSS logic
>>> +         * which is a concatenation of [vc:6 | dt_id:2] hence we reuse
>>> +         * the least significant two bits of the VC to 'stuff' the CID value.
>>> +         */
>>>           u8 dt_id = vc;
>> And where are you discarding the non-2-lsb?
> 
> At the assignment of dt_id
> 
> vc:6
> dt_id:2
> 
> =>
> 
> cid:8 = [vc:6 | dt_id:2]
> vc == 00110110
> cid = [110110 | 10]
> 
> I have no more information what CID is or how the bitfield is populated than I have already indicated in the comment/commit log.
> 
OK so you're discarding the 2 lsb of the [vc:6|dt_id:2]

however

the way I read the comment would suggest that you're taking
vc[6:2]

Konrad

Re: [PATCH v3 15/15] media: qcom: camss: Comment CSID dt_id field

[Bryan O'Donoghue]

On 28/08/2023 16:38, Konrad Dybcio wrote:
> On 28.08.2023 17:34, Bryan O'Donoghue wrote:
>> On 26/08/2023 11:18, Konrad Dybcio wrote:
>>>> +        /*
>>>> +         * A value caled 'CID' gets generated internal to CAMSS logic
>>>> +         * which is a concatenation of [vc:6 | dt_id:2] hence we reuse
>>>> +         * the least significant two bits of the VC to 'stuff' the CID value.
>>>> +         */
>>>>            u8 dt_id = vc;
>>> And where are you discarding the non-2-lsb?
>>
>> At the assignment of dt_id
>>
>> vc:6
>> dt_id:2
>>
>> =>
>>
>> cid:8 = [vc:6 | dt_id:2]
>> vc == 00110110
>> cid = [110110 | 10]
>>
>> I have no more information what CID is or how the bitfield is populated than I have already indicated in the comment/commit log.
>>
> OK so you're discarding the 2 lsb of the [vc:6|dt_id:2]
> 
> however
> 
> the way I read the comment would suggest that you're taking
> vc[6:2]
> 
> Konrad

Fair enough, obvs the comment needs work so.

---
bod

Re: [PATCH v3 01/15] media: qcom: camss: Amalgamate struct resource with struct resource_ispif

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:30AM +0100, Bryan O'Donoghue wrote:
> There is no good reason to differentiate the two resource structures
> here. As part of a general tidyup of the declaration and passing of
> resources within in the CAMSS driver it will be advantageous to have
> one unified resource structure.
> 
> The two structures are very similar anyway thus leading more credence
> still to the argument there should be only one.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss-ispif.c |  4 ++--
>  drivers/media/platform/qcom/camss/camss-ispif.h |  4 ++--
>  drivers/media/platform/qcom/camss/camss.c       | 14 +++++++-------
>  drivers/media/platform/qcom/camss/camss.h       |  8 +-------
>  4 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c
> index 1c7e4b1b49401..61765b874b9a2 100644
> --- a/drivers/media/platform/qcom/camss/camss-ispif.c
> +++ b/drivers/media/platform/qcom/camss/camss-ispif.c
> @@ -1095,7 +1095,7 @@ static int ispif_init_formats(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>   * Return 0 on success or a negative error code otherwise
>   */
>  int msm_ispif_subdev_init(struct camss *camss,
> -			  const struct resources_ispif *res)
> +			  const struct resources *res)
>  {
>  	struct device *dev = camss->dev;
>  	struct ispif_device *ispif = camss->ispif;
> @@ -1152,7 +1152,7 @@ int msm_ispif_subdev_init(struct camss *camss,
>  
>  	/* Interrupt */
>  
> -	ret = platform_get_irq_byname(pdev, res->interrupt);
> +	ret = platform_get_irq_byname(pdev, res->interrupt[0]);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/media/platform/qcom/camss/camss-ispif.h b/drivers/media/platform/qcom/camss/camss-ispif.h
> index fdf28e68cc7d8..c7c41f7afcaad 100644
> --- a/drivers/media/platform/qcom/camss/camss-ispif.h
> +++ b/drivers/media/platform/qcom/camss/camss-ispif.h
> @@ -66,10 +66,10 @@ struct ispif_device {
>  	struct camss *camss;
>  };
>  
> -struct resources_ispif;
> +struct resources;
>  
>  int msm_ispif_subdev_init(struct camss *camss,
> -			  const struct resources_ispif *res);
> +			  const struct resources *res);
>  
>  int msm_ispif_register_entities(struct ispif_device *ispif,
>  				struct v4l2_device *v4l2_dev);
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index f4948bdf3f8f9..de39dc987444f 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -93,14 +93,14 @@ static const struct resources csid_res_8x16[] = {
>  	},
>  };
>  
> -static const struct resources_ispif ispif_res_8x16 = {
> +static const struct resources ispif_res_8x16 = {
>  	/* ISPIF */
>  	.clock = { "top_ahb", "ahb", "ispif_ahb",
>  		   "csi0", "csi0_pix", "csi0_rdi",
>  		   "csi1", "csi1_pix", "csi1_rdi" },
>  	.clock_for_reset = { "vfe0", "csi_vfe0" },
>  	.reg = { "ispif", "csi_clk_mux" },
> -	.interrupt = "ispif"
> +	.interrupt = { "ispif" }
>  
>  };
>  
> @@ -234,7 +234,7 @@ static const struct resources csid_res_8x96[] = {
>  	}
>  };
>  
> -static const struct resources_ispif ispif_res_8x96 = {
> +static const struct resources ispif_res_8x96 = {
>  	/* ISPIF */
>  	.clock = { "top_ahb", "ahb", "ispif_ahb",
>  		   "csi0", "csi0_pix", "csi0_rdi",
> @@ -243,7 +243,7 @@ static const struct resources_ispif ispif_res_8x96 = {
>  		   "csi3", "csi3_pix", "csi3_rdi" },
>  	.clock_for_reset = { "vfe0", "csi_vfe0", "vfe1", "csi_vfe1" },
>  	.reg = { "ispif", "csi_clk_mux" },
> -	.interrupt = "ispif"
> +	.interrupt = { "ispif" }
>  };
>  
>  static const struct resources vfe_res_8x96[] = {
> @@ -410,7 +410,7 @@ static const struct resources csid_res_660[] = {
>  	}
>  };
>  
> -static const struct resources_ispif ispif_res_660 = {
> +static const struct resources ispif_res_660 = {
>  	/* ISPIF */
>  	.clock = { "top_ahb", "ahb", "ispif_ahb",
>  		   "csi0", "csi0_pix", "csi0_rdi",
> @@ -419,7 +419,7 @@ static const struct resources_ispif ispif_res_660 = {
>  		   "csi3", "csi3_pix", "csi3_rdi" },
>  	.clock_for_reset = { "vfe0", "csi_vfe0", "vfe1", "csi_vfe1" },
>  	.reg = { "ispif", "csi_clk_mux" },
> -	.interrupt = "ispif"
> +	.interrupt = { "ispif" }
>  };
>  
>  static const struct resources vfe_res_660[] = {
> @@ -1122,7 +1122,7 @@ static int camss_init_subdevices(struct camss *camss)
>  {
>  	const struct resources *csiphy_res;
>  	const struct resources *csid_res;
> -	const struct resources_ispif *ispif_res;
> +	const struct resources *ispif_res;
>  	const struct resources *vfe_res;
>  	unsigned int i;
>  	int ret;
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index f6c326cb853b8..e95211cdb1fd6 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -44,18 +44,12 @@
>  struct resources {

While at it, or in a separate patch, could you rename this to
camss_resources ? resources is a too generic name, too likely to clash.
With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	char *regulators[CAMSS_RES_MAX];
>  	char *clock[CAMSS_RES_MAX];
> +	char *clock_for_reset[CAMSS_RES_MAX];
>  	u32 clock_rate[CAMSS_RES_MAX][CAMSS_RES_MAX];
>  	char *reg[CAMSS_RES_MAX];
>  	char *interrupt[CAMSS_RES_MAX];
>  };
>  
> -struct resources_ispif {
> -	char *clock[CAMSS_RES_MAX];
> -	char *clock_for_reset[CAMSS_RES_MAX];
> -	char *reg[CAMSS_RES_MAX];
> -	char *interrupt;
> -};
> -
>  struct icc_bw_tbl {
>  	u32 avg;
>  	u32 peak;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/15] media: qcom: camss: Start to move to module compat matched resources

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:31AM +0100, Bryan O'Donoghue wrote:
> There is a lot of unnecessary if/elsing in this code that arguably
> should never have made it upstream when adding a second let alone
> subsequent SoC.
> 
> I'm guilty of not fixing the mess myself when adding in the sm8250.
> Before adding in any new SoCs or resources lets take the time to cleanup
> the resource passing.
> 
> First step is to pass the generic struct camss_resources as a parameter
> per the compatible list.
> 
> Subsequent patches will address the other somewhat dispirate strutures

s/dispirate/disparate/ ?

> which we are also doing if/else on and assigning statically.
> 
> Squashed down a commit to drop useless NULL assignment for ispif resources.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss.c | 92 ++++++++++++-----------
>  drivers/media/platform/qcom/camss/camss.h |  8 ++
>  2 files changed, 56 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index de39dc987444f..82e679c8ca011 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -14,6 +14,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_domain.h>
> @@ -1120,47 +1121,13 @@ static int camss_of_parse_ports(struct camss *camss)
>   */
>  static int camss_init_subdevices(struct camss *camss)
>  {
> -	const struct resources *csiphy_res;
> -	const struct resources *csid_res;
> -	const struct resources *ispif_res;
> -	const struct resources *vfe_res;
> +	const struct camss_resources *res = camss->res;
>  	unsigned int i;
>  	int ret;
>  
> -	if (camss->version == CAMSS_8x16) {
> -		csiphy_res = csiphy_res_8x16;
> -		csid_res = csid_res_8x16;
> -		ispif_res = &ispif_res_8x16;
> -		vfe_res = vfe_res_8x16;
> -	} else if (camss->version == CAMSS_8x96) {
> -		csiphy_res = csiphy_res_8x96;
> -		csid_res = csid_res_8x96;
> -		ispif_res = &ispif_res_8x96;
> -		vfe_res = vfe_res_8x96;
> -	} else if (camss->version == CAMSS_660) {
> -		csiphy_res = csiphy_res_660;
> -		csid_res = csid_res_660;
> -		ispif_res = &ispif_res_660;
> -		vfe_res = vfe_res_660;
> -	}  else if (camss->version == CAMSS_845) {
> -		csiphy_res = csiphy_res_845;
> -		csid_res = csid_res_845;
> -		/* Titan VFEs don't have an ISPIF  */
> -		ispif_res = NULL;
> -		vfe_res = vfe_res_845;
> -	} else if (camss->version == CAMSS_8250) {
> -		csiphy_res = csiphy_res_8250;
> -		csid_res = csid_res_8250;
> -		/* Titan VFEs don't have an ISPIF  */
> -		ispif_res = NULL;
> -		vfe_res = vfe_res_8250;
> -	} else {
> -		return -EINVAL;
> -	}
> -
>  	for (i = 0; i < camss->csiphy_num; i++) {
>  		ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
> -					     &csiphy_res[i], i);
> +					     &res->csiphy_res[i], i);
>  		if (ret < 0) {
>  			dev_err(camss->dev,
>  				"Failed to init csiphy%d sub-device: %d\n",
> @@ -1172,7 +1139,7 @@ static int camss_init_subdevices(struct camss *camss)
>  	/* note: SM8250 requires VFE to be initialized before CSID */
>  	for (i = 0; i < camss->vfe_num + camss->vfe_lite_num; i++) {
>  		ret = msm_vfe_subdev_init(camss, &camss->vfe[i],
> -					  &vfe_res[i], i);
> +					  &res->vfe_res[i], i);
>  		if (ret < 0) {
>  			dev_err(camss->dev,
>  				"Fail to init vfe%d sub-device: %d\n", i, ret);
> @@ -1182,7 +1149,7 @@ static int camss_init_subdevices(struct camss *camss)
>  
>  	for (i = 0; i < camss->csid_num; i++) {
>  		ret = msm_csid_subdev_init(camss, &camss->csid[i],
> -					   &csid_res[i], i);
> +					   &res->csid_res[i], i);
>  		if (ret < 0) {
>  			dev_err(camss->dev,
>  				"Failed to init csid%d sub-device: %d\n",
> @@ -1191,7 +1158,7 @@ static int camss_init_subdevices(struct camss *camss)
>  		}
>  	}
>  
> -	ret = msm_ispif_subdev_init(camss, ispif_res);
> +	ret = msm_ispif_subdev_init(camss, res->ispif_res);
>  	if (ret < 0) {
>  		dev_err(camss->dev, "Failed to init ispif sub-device: %d\n",
>  		ret);
> @@ -1554,6 +1521,10 @@ static int camss_probe(struct platform_device *pdev)
>  	if (!camss)
>  		return -ENOMEM;
>  
> +	camss->res = of_device_get_match_data(dev);
> +	if (!camss->res)
> +		return -ENODEV;

You could possibly drop the error check, as this can't happen.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	atomic_set(&camss->ref_count, 0);
>  	camss->dev = dev;
>  	platform_set_drvdata(pdev, camss);
> @@ -1735,12 +1706,45 @@ static void camss_remove(struct platform_device *pdev)
>  		camss_delete(camss);
>  }
>  
> +static const struct camss_resources msm8916_resources = {
> +	.csiphy_res = csiphy_res_8x16,
> +	.csid_res = csid_res_8x16,
> +	.ispif_res = &ispif_res_8x16,
> +	.vfe_res = vfe_res_8x16,
> +};
> +
> +static const struct camss_resources msm8996_resources = {
> +	.csiphy_res = csiphy_res_8x96,
> +	.csid_res = csid_res_8x96,
> +	.ispif_res = &ispif_res_8x96,
> +	.vfe_res = vfe_res_8x96,
> +};
> +
> +static const struct camss_resources sdm660_resources = {
> +	.csiphy_res = csiphy_res_660,
> +	.csid_res = csid_res_660,
> +	.ispif_res = &ispif_res_660,
> +	.vfe_res = vfe_res_660,
> +};
> +
> +static const struct camss_resources sdm845_resources = {
> +	.csiphy_res = csiphy_res_845,
> +	.csid_res = csid_res_845,
> +	.vfe_res = vfe_res_845,
> +};
> +
> +static const struct camss_resources sm8250_resources = {
> +	.csiphy_res = csiphy_res_8250,
> +	.csid_res = csid_res_8250,
> +	.vfe_res = vfe_res_8250,
> +};
> +
>  static const struct of_device_id camss_dt_match[] = {
> -	{ .compatible = "qcom,msm8916-camss" },
> -	{ .compatible = "qcom,msm8996-camss" },
> -	{ .compatible = "qcom,sdm660-camss" },
> -	{ .compatible = "qcom,sdm845-camss" },
> -	{ .compatible = "qcom,sm8250-camss" },
> +	{ .compatible = "qcom,msm8916-camss", .data = &msm8916_resources },
> +	{ .compatible = "qcom,msm8996-camss", .data = &msm8996_resources },
> +	{ .compatible = "qcom,sdm660-camss", .data = &sdm660_resources },
> +	{ .compatible = "qcom,sdm845-camss", .data = &sdm845_resources },
> +	{ .compatible = "qcom,sm8250-camss", .data = &sm8250_resources },
>  	{ }
>  };
>  
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index e95211cdb1fd6..f632ee49ad83e 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -79,6 +79,13 @@ enum icc_count {
>  	ICC_SM8250_COUNT = 4,
>  };
>  
> +struct camss_resources {
> +	const struct resources *csiphy_res;
> +	const struct resources *csid_res;
> +	const struct resources *ispif_res;
> +	const struct resources *vfe_res;
> +};
> +
>  struct camss {
>  	enum camss_version version;
>  	struct v4l2_device v4l2_dev;
> @@ -99,6 +106,7 @@ struct camss {
>  	struct device_link **genpd_link;
>  	struct icc_path *icc_path[ICC_SM8250_COUNT];
>  	struct icc_bw_tbl icc_bw_tbl[ICC_SM8250_COUNT];
> +	const struct camss_resources *res;
>  };
>  
>  struct camss_camera_interface {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 03/15] media: qcom: camss: Pass icc bandwidth table as a platform parameter

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:32AM +0100, Bryan O'Donoghue wrote:
> Pass the bandwidth table as a platform parameter not if/else derived
> pointer to the static table.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/qcom/camss/camss.c | 29 +++++++----------------
>  drivers/media/platform/qcom/camss/camss.h |  3 ++-
>  2 files changed, 11 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 82e679c8ca011..9a15f2253aa20 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1485,21 +1485,15 @@ static int camss_configure_pd(struct camss *camss)
>  static int camss_icc_get(struct camss *camss)
>  {
>  	const struct resources_icc *icc_res;
> -	int nbr_icc_paths = 0;
>  	int i;
>  
> -	if (camss->version == CAMSS_8250) {
> -		icc_res = &icc_res_sm8250[0];
> -		nbr_icc_paths =	ICC_SM8250_COUNT;
> -	}
> +	icc_res = camss->res->icc_res;
>  
> -	for (i = 0; i < nbr_icc_paths; i++) {
> +	for (i = 0; i < camss->res->icc_path_num; i++) {
>  		camss->icc_path[i] = devm_of_icc_get(camss->dev,
>  						     icc_res[i].name);
>  		if (IS_ERR(camss->icc_path[i]))
>  			return PTR_ERR(camss->icc_path[i]);
> -
> -		camss->icc_bw_tbl[i] = icc_res[i].icc_bw_tbl;
>  	}
>  
>  	return 0;
> @@ -1737,6 +1731,8 @@ static const struct camss_resources sm8250_resources = {
>  	.csiphy_res = csiphy_res_8250,
>  	.csid_res = csid_res_8250,
>  	.vfe_res = vfe_res_8250,
> +	.icc_res = icc_res_sm8250,
> +	.icc_path_num = ARRAY_SIZE(icc_res_sm8250),
>  };
>  
>  static const struct of_device_id camss_dt_match[] = {
> @@ -1753,14 +1749,10 @@ MODULE_DEVICE_TABLE(of, camss_dt_match);
>  static int __maybe_unused camss_runtime_suspend(struct device *dev)
>  {
>  	struct camss *camss = dev_get_drvdata(dev);
> -	int nbr_icc_paths = 0;
>  	int i;
>  	int ret;
>  
> -	if (camss->version == CAMSS_8250)
> -		nbr_icc_paths =	ICC_SM8250_COUNT;
> -
> -	for (i = 0; i < nbr_icc_paths; i++) {
> +	for (i = 0; i < camss->res->icc_path_num; i++) {
>  		ret = icc_set_bw(camss->icc_path[i], 0, 0);
>  		if (ret)
>  			return ret;
> @@ -1772,17 +1764,14 @@ static int __maybe_unused camss_runtime_suspend(struct device *dev)
>  static int __maybe_unused camss_runtime_resume(struct device *dev)
>  {
>  	struct camss *camss = dev_get_drvdata(dev);
> -	int nbr_icc_paths = 0;
> +	const struct resources_icc *icc_res = camss->res->icc_res;
>  	int i;
>  	int ret;
>  
> -	if (camss->version == CAMSS_8250)
> -		nbr_icc_paths =	ICC_SM8250_COUNT;
> -
> -	for (i = 0; i < nbr_icc_paths; i++) {
> +	for (i = 0; i < camss->res->icc_path_num; i++) {
>  		ret = icc_set_bw(camss->icc_path[i],
> -				 camss->icc_bw_tbl[i].avg,
> -				 camss->icc_bw_tbl[i].peak);
> +				 icc_res[i].icc_bw_tbl.avg,
> +				 icc_res[i].icc_bw_tbl.peak);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index f632ee49ad83e..988c313b63551 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -84,6 +84,8 @@ struct camss_resources {
>  	const struct resources *csid_res;
>  	const struct resources *ispif_res;
>  	const struct resources *vfe_res;
> +	const struct resources_icc *icc_res;
> +	const int icc_path_num;
>  };
>  
>  struct camss {
> @@ -105,7 +107,6 @@ struct camss {
>  	struct device **genpd;
>  	struct device_link **genpd_link;
>  	struct icc_path *icc_path[ICC_SM8250_COUNT];
> -	struct icc_bw_tbl icc_bw_tbl[ICC_SM8250_COUNT];
>  	const struct camss_resources *res;
>  };
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 04/15] media: qcom: camss: Pass remainder of variables as resources

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:33AM +0100, Bryan O'Donoghue wrote:
> The following variables are being assigned statically based on
> compatible strings in the probe path.
> 
> * enum camss_version version;
> * int csiphy_num;
> * int csid_num;
> * int vfe_num;
> * int vfe_lite_num;
> * int vfe_total_num;
> 
> Migrate those variables to resource parameters passed in on platform
> probe arguments. The one caveat is for VFE it has been necessary to
> intoduce a new variable vfe_total_num to capture the aggregate value of

s/intoduce/introduce/

What is this required, can't you compute it at runtime as done already ?

> vfe_num + vfe_lite_num.
> 
> All the rest of the changes are rote camss->variable to
> camss->res->variable with the parameter tables now populating the listed
> variables.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-csid.c    |  14 +--
>  .../qcom/camss/camss-csiphy-3ph-1-0.c         |   8 +-
>  .../media/platform/qcom/camss/camss-csiphy.c  |  16 +--
>  .../media/platform/qcom/camss/camss-ispif.c   |  28 ++---
>  .../media/platform/qcom/camss/camss-vfe-170.c |   4 +-
>  .../media/platform/qcom/camss/camss-vfe-480.c |   4 +-
>  drivers/media/platform/qcom/camss/camss-vfe.c |  26 ++--
>  .../media/platform/qcom/camss/camss-video.c   |  10 +-
>  drivers/media/platform/qcom/camss/camss.c     | 119 +++++++++---------
>  drivers/media/platform/qcom/camss/camss.h     |  11 +-
>  10 files changed, 117 insertions(+), 123 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 9d7fb1adebb98..08991b070bd61 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -159,7 +159,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>  	struct camss *camss = csid->camss;
>  	struct device *dev = camss->dev;
>  	struct vfe_device *vfe = &camss->vfe[csid->id];
> -	u32 version = camss->version;
> +	u32 version = camss->res->version;
>  	int ret = 0;
>  
>  	if (on) {
> @@ -576,13 +576,13 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>  	csid->camss = camss;
>  	csid->id = id;
>  
> -	if (camss->version == CAMSS_8x16) {
> +	if (camss->res->version == CAMSS_8x16) {
>  		csid->ops = &csid_ops_4_1;

Another candidate to be moved to the resources structure (in a separate
patch) ?

> -	} else if (camss->version == CAMSS_8x96 ||
> -		   camss->version == CAMSS_660) {
> +	} else if (camss->res->version == CAMSS_8x96 ||
> +		   camss->res->version == CAMSS_660) {
>  		csid->ops = &csid_ops_4_7;
> -	} else if (camss->version == CAMSS_845 ||
> -		   camss->version == CAMSS_8250) {
> +	} else if (camss->res->version == CAMSS_845 ||
> +		   camss->res->version == CAMSS_8250) {
>  		csid->ops = &csid_ops_gen2;
>  	} else {
>  		return -EINVAL;
> @@ -591,7 +591,7 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>  
>  	/* Memory */
>  
> -	if (camss->version == CAMSS_8250) {
> +	if (camss->res->version == CAMSS_8250) {
>  		/* for titan 480, CSID registers are inside the VFE region,
>  		 * between the VFE "top" and "bus" registers. this requires
>  		 * VFE to be initialized before CSID
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index 4dba61b8d3f2a..f50e2235c37fc 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> @@ -402,7 +402,7 @@ static void csiphy_gen1_config_lanes(struct csiphy_device *csiphy,
>  	val = CSIPHY_3PH_LNn_CFG1_SWI_REC_DLY_PRG;
>  	writel_relaxed(val, csiphy->base + CSIPHY_3PH_LNn_CFG1(l));
>  
> -	if (csiphy->camss->version == CAMSS_660)
> +	if (csiphy->camss->res->version == CAMSS_660)
>  		val = CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS_660;
>  	else
>  		val = CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS;
> @@ -419,7 +419,7 @@ static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
>  	int i, l, array_size;
>  	u32 val;
>  
> -	switch (csiphy->camss->version) {
> +	switch (csiphy->camss->res->version) {
>  	case CAMSS_845:
>  		r = &lane_regs_sdm845[0][0];
>  		array_size = ARRAY_SIZE(lane_regs_sdm845[0]);
> @@ -468,8 +468,8 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>  				s64 link_freq, u8 lane_mask)
>  {
>  	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
> -	bool is_gen2 = (csiphy->camss->version == CAMSS_845 ||
> -			csiphy->camss->version == CAMSS_8250);
> +	bool is_gen2 = (csiphy->camss->res->version == CAMSS_845 ||
> +			csiphy->camss->res->version == CAMSS_8250);
>  	u8 settle_cnt;
>  	u8 val;
>  	int i;
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index c5ae9c61ee0b1..593aec5c97bc2 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -557,17 +557,17 @@ int msm_csiphy_subdev_init(struct camss *camss,
>  	csiphy->id = id;
>  	csiphy->cfg.combo_mode = 0;
>  
> -	if (camss->version == CAMSS_8x16) {
> +	if (camss->res->version == CAMSS_8x16) {
>  		csiphy->ops = &csiphy_ops_2ph_1_0;
>  		csiphy->formats = csiphy_formats_8x16;
>  		csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x16);
> -	} else if (camss->version == CAMSS_8x96 ||
> -		   camss->version == CAMSS_660) {
> +	} else if (camss->res->version == CAMSS_8x96 ||
> +		   camss->res->version == CAMSS_660) {
>  		csiphy->ops = &csiphy_ops_3ph_1_0;
>  		csiphy->formats = csiphy_formats_8x96;
>  		csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x96);
> -	} else if (camss->version == CAMSS_845 ||
> -		   camss->version == CAMSS_8250) {
> +	} else if (camss->res->version == CAMSS_845 ||
> +		   camss->res->version == CAMSS_8250) {
>  		csiphy->ops = &csiphy_ops_3ph_1_0;
>  		csiphy->formats = csiphy_formats_sdm845;
>  		csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845);
> @@ -581,8 +581,8 @@ int msm_csiphy_subdev_init(struct camss *camss,
>  	if (IS_ERR(csiphy->base))
>  		return PTR_ERR(csiphy->base);
>  
> -	if (camss->version == CAMSS_8x16 ||
> -	    camss->version == CAMSS_8x96) {
> +	if (camss->res->version == CAMSS_8x16 ||
> +	    camss->res->version == CAMSS_8x96) {
>  		csiphy->base_clk_mux =
>  			devm_platform_ioremap_resource_byname(pdev, res->reg[1]);
>  		if (IS_ERR(csiphy->base_clk_mux))
> @@ -664,7 +664,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
>  		    !strcmp(clock->name, "csiphy5_timer"))
>  			csiphy->rate_set[i] = true;
>  
> -		if (camss->version == CAMSS_660 &&
> +		if (camss->res->version == CAMSS_660 &&
>  		    (!strcmp(clock->name, "csi0_phy") ||
>  		     !strcmp(clock->name, "csi1_phy") ||
>  		     !strcmp(clock->name, "csi2_phy")))
> diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c
> index 61765b874b9a2..0a58d2cf45007 100644
> --- a/drivers/media/platform/qcom/camss/camss-ispif.c
> +++ b/drivers/media/platform/qcom/camss/camss-ispif.c
> @@ -270,7 +270,7 @@ static int ispif_vfe_reset(struct ispif_device *ispif, u8 vfe_id)
>  	unsigned long time;
>  	u32 val;
>  
> -	if (vfe_id > (camss->vfe_num - 1)) {
> +	if (vfe_id > (camss->res->vfe_num - 1)) {

No need for parentheses by the way, and it could also be written

	if (vfe_id >= camss->res->vfe_num) {

>  		dev_err(camss->dev,
>  			"Error: asked reset for invalid VFE%d\n", vfe_id);
>  		return -ENOENT;
> @@ -829,8 +829,8 @@ static int ispif_set_stream(struct v4l2_subdev *sd, int enable)
>  		ispif_select_csid(ispif, intf, csid, vfe, 1);
>  		ispif_select_cid(ispif, intf, cid, vfe, 1);
>  		ispif_config_irq(ispif, intf, vfe, 1);
> -		if (camss->version == CAMSS_8x96 ||
> -		    camss->version == CAMSS_660)
> +		if (camss->res->version == CAMSS_8x96 ||
> +		    camss->res->version == CAMSS_660)
>  			ispif_config_pack(ispif,
>  					  line->fmt[MSM_ISPIF_PAD_SINK].code,
>  					  intf, cid, vfe, 1);
> @@ -847,8 +847,8 @@ static int ispif_set_stream(struct v4l2_subdev *sd, int enable)
>  			return ret;
>  
>  		mutex_lock(&ispif->config_lock);
> -		if (camss->version == CAMSS_8x96 ||
> -		    camss->version == CAMSS_660)
> +		if (camss->res->version == CAMSS_8x96 ||
> +		    camss->res->version == CAMSS_660)
>  			ispif_config_pack(ispif,
>  					  line->fmt[MSM_ISPIF_PAD_SINK].code,
>  					  intf, cid, vfe, 0);
> @@ -1109,10 +1109,10 @@ int msm_ispif_subdev_init(struct camss *camss,
>  	ispif->camss = camss;
>  
>  	/* Number of ISPIF lines - same as number of CSID hardware modules */
> -	if (camss->version == CAMSS_8x16)
> +	if (camss->res->version == CAMSS_8x16)
>  		ispif->line_num = 2;
> -	else if (camss->version == CAMSS_8x96 ||
> -		 camss->version == CAMSS_660)
> +	else if (camss->res->version == CAMSS_8x96 ||
> +		 camss->res->version == CAMSS_660)
>  		ispif->line_num = 4;
>  	else
>  		return -EINVAL;
> @@ -1126,12 +1126,12 @@ int msm_ispif_subdev_init(struct camss *camss,
>  		ispif->line[i].ispif = ispif;
>  		ispif->line[i].id = i;
>  
> -		if (camss->version == CAMSS_8x16) {
> +		if (camss->res->version == CAMSS_8x16) {
>  			ispif->line[i].formats = ispif_formats_8x16;
>  			ispif->line[i].nformats =
>  					ARRAY_SIZE(ispif_formats_8x16);
> -		} else if (camss->version == CAMSS_8x96 ||
> -			   camss->version == CAMSS_660) {
> +		} else if (camss->res->version == CAMSS_8x96 ||
> +			   camss->res->version == CAMSS_660) {
>  			ispif->line[i].formats = ispif_formats_8x96;
>  			ispif->line[i].nformats =
>  					ARRAY_SIZE(ispif_formats_8x96);
> @@ -1159,11 +1159,11 @@ int msm_ispif_subdev_init(struct camss *camss,
>  	ispif->irq = ret;
>  	snprintf(ispif->irq_name, sizeof(ispif->irq_name), "%s_%s",
>  		 dev_name(dev), MSM_ISPIF_NAME);
> -	if (camss->version == CAMSS_8x16)
> +	if (camss->res->version == CAMSS_8x16)
>  		ret = devm_request_irq(dev, ispif->irq, ispif_isr_8x16,
>  			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
> -	else if (camss->version == CAMSS_8x96 ||
> -		 camss->version == CAMSS_660)
> +	else if (camss->res->version == CAMSS_8x96 ||
> +		 camss->res->version == CAMSS_660)
>  		ret = devm_request_irq(dev, ispif->irq, ispif_isr_8x96,
>  			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>  	else
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> index ae9137633c301..9905bb06b3823 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> @@ -676,7 +676,7 @@ static void vfe_pm_domain_off(struct vfe_device *vfe)
>  {
>  	struct camss *camss = vfe->camss;
>  
> -	if (vfe->id >= camss->vfe_num)
> +	if (vfe->id >= camss->res->vfe_num)
>  		return;
>  
>  	device_link_del(camss->genpd_link[vfe->id]);
> @@ -691,7 +691,7 @@ static int vfe_pm_domain_on(struct vfe_device *vfe)
>  	struct camss *camss = vfe->camss;
>  	enum vfe_line_id id = vfe->id;
>  
> -	if (id >= camss->vfe_num)
> +	if (id >= camss->res->vfe_num)
>  		return 0;
>  
>  	camss->genpd_link[id] = device_link_add(camss->dev, camss->genpd[id],
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
> index a64d660abc538..80338efceb9e1 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
> @@ -502,7 +502,7 @@ static void vfe_pm_domain_off(struct vfe_device *vfe)
>  {
>  	struct camss *camss = vfe->camss;
>  
> -	if (vfe->id >= camss->vfe_num)
> +	if (vfe->id >= camss->res->vfe_num)
>  		return;
>  
>  	device_link_del(camss->genpd_link[vfe->id]);
> @@ -517,7 +517,7 @@ static int vfe_pm_domain_on(struct vfe_device *vfe)
>  	struct camss *camss = vfe->camss;
>  	enum vfe_line_id id = vfe->id;
>  
> -	if (id >= camss->vfe_num)
> +	if (id >= camss->res->vfe_num)
>  		return 0;
>  
>  	camss->genpd_link[id] = device_link_add(camss->dev, camss->genpd[id],
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index b021f81cef123..526dd4ab343fe 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -170,7 +170,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>  {
>  	struct vfe_device *vfe = to_vfe(line);
>  
> -	if (vfe->camss->version == CAMSS_8x16)
> +	if (vfe->camss->res->version == CAMSS_8x16)
>  		switch (sink_code) {
>  		case MEDIA_BUS_FMT_YUYV8_1X16:
>  		{
> @@ -218,10 +218,10 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>  
>  			return sink_code;
>  		}
> -	else if (vfe->camss->version == CAMSS_8x96 ||
> -		 vfe->camss->version == CAMSS_660 ||
> -		 vfe->camss->version == CAMSS_845 ||
> -		 vfe->camss->version == CAMSS_8250)
> +	else if (vfe->camss->res->version == CAMSS_8x96 ||
> +		 vfe->camss->res->version == CAMSS_660 ||
> +		 vfe->camss->res->version == CAMSS_845 ||
> +		 vfe->camss->res->version == CAMSS_8250)
>  		switch (sink_code) {
>  		case MEDIA_BUS_FMT_YUYV8_1X16:
>  		{
> @@ -1286,7 +1286,7 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>  	int i, j;
>  	int ret;
>  
> -	switch (camss->version) {
> +	switch (camss->res->version) {
>  	case CAMSS_8x16:
>  		vfe->ops = &vfe_ops_4_1;
>  		break;
> @@ -1392,7 +1392,7 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>  		init_completion(&l->output.sof);
>  		init_completion(&l->output.reg_update);
>  
> -		if (camss->version == CAMSS_8x16) {
> +		if (camss->res->version == CAMSS_8x16) {
>  			if (i == VFE_LINE_PIX) {
>  				l->formats = formats_pix_8x16;
>  				l->nformats = ARRAY_SIZE(formats_pix_8x16);
> @@ -1400,8 +1400,8 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>  				l->formats = formats_rdi_8x16;
>  				l->nformats = ARRAY_SIZE(formats_rdi_8x16);
>  			}
> -		} else if (camss->version == CAMSS_8x96 ||
> -			   camss->version == CAMSS_660) {
> +		} else if (camss->res->version == CAMSS_8x96 ||
> +			   camss->res->version == CAMSS_660) {
>  			if (i == VFE_LINE_PIX) {
>  				l->formats = formats_pix_8x96;
>  				l->nformats = ARRAY_SIZE(formats_pix_8x96);
> @@ -1409,8 +1409,8 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>  				l->formats = formats_rdi_8x96;
>  				l->nformats = ARRAY_SIZE(formats_rdi_8x96);
>  			}
> -		} else if (camss->version == CAMSS_845 ||
> -			   camss->version == CAMSS_8250) {
> +		} else if (camss->res->version == CAMSS_845 ||
> +			   camss->res->version == CAMSS_8250) {
>  			l->formats = formats_rdi_845;
>  			l->nformats = ARRAY_SIZE(formats_rdi_845);
>  		} else {
> @@ -1542,8 +1542,8 @@ int msm_vfe_register_entities(struct vfe_device *vfe,
>  		}
>  
>  		video_out->ops = &vfe->video_ops;
> -		if (vfe->camss->version == CAMSS_845 ||
> -		    vfe->camss->version == CAMSS_8250)
> +		if (vfe->camss->res->version == CAMSS_845 ||
> +		    vfe->camss->res->version == CAMSS_8250)
>  			video_out->bpl_alignment = 16;
>  		else
>  			video_out->bpl_alignment = 8;
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 1d686aef92eaf..46a89b5f6c171 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -1006,7 +1006,7 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>  
>  	mutex_init(&video->lock);
>  
> -	if (video->camss->version == CAMSS_8x16) {
> +	if (video->camss->res->version == CAMSS_8x16) {
>  		if (is_pix) {
>  			video->formats = formats_pix_8x16;
>  			video->nformats = ARRAY_SIZE(formats_pix_8x16);
> @@ -1014,8 +1014,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>  			video->formats = formats_rdi_8x16;
>  			video->nformats = ARRAY_SIZE(formats_rdi_8x16);
>  		}
> -	} else if (video->camss->version == CAMSS_8x96 ||
> -		   video->camss->version == CAMSS_660) {
> +	} else if (video->camss->res->version == CAMSS_8x96 ||
> +		   video->camss->res->version == CAMSS_660) {
>  		if (is_pix) {
>  			video->formats = formats_pix_8x96;
>  			video->nformats = ARRAY_SIZE(formats_pix_8x96);
> @@ -1023,8 +1023,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>  			video->formats = formats_rdi_8x96;
>  			video->nformats = ARRAY_SIZE(formats_rdi_8x96);
>  		}
> -	}  else if (video->camss->version == CAMSS_845 ||
> -		    video->camss->version == CAMSS_8250) {
> +	}  else if (video->camss->res->version == CAMSS_845 ||
> +		    video->camss->res->version == CAMSS_8250) {
>  		video->formats = formats_rdi_845;
>  		video->nformats = ARRAY_SIZE(formats_rdi_845);
>  	} else {
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 9a15f2253aa20..c8a2571e664fe 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1005,7 +1005,7 @@ int camss_pm_domain_on(struct camss *camss, int id)
>  {
>  	int ret = 0;
>  
> -	if (id < camss->vfe_num) {
> +	if (id < camss->res->vfe_num) {
>  		struct vfe_device *vfe = &camss->vfe[id];
>  
>  		ret = vfe->ops->pm_domain_on(vfe);
> @@ -1016,7 +1016,7 @@ int camss_pm_domain_on(struct camss *camss, int id)
>  
>  void camss_pm_domain_off(struct camss *camss, int id)
>  {
> -	if (id < camss->vfe_num) {
> +	if (id < camss->res->vfe_num) {
>  		struct vfe_device *vfe = &camss->vfe[id];
>  
>  		vfe->ops->pm_domain_off(vfe);
> @@ -1125,7 +1125,7 @@ static int camss_init_subdevices(struct camss *camss)
>  	unsigned int i;
>  	int ret;
>  
> -	for (i = 0; i < camss->csiphy_num; i++) {
> +	for (i = 0; i < camss->res->csiphy_num; i++) {
>  		ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
>  					     &res->csiphy_res[i], i);
>  		if (ret < 0) {
> @@ -1137,7 +1137,7 @@ static int camss_init_subdevices(struct camss *camss)
>  	}
>  
>  	/* note: SM8250 requires VFE to be initialized before CSID */
> -	for (i = 0; i < camss->vfe_num + camss->vfe_lite_num; i++) {
> +	for (i = 0; i < camss->res->vfe_total_num; i++) {
>  		ret = msm_vfe_subdev_init(camss, &camss->vfe[i],
>  					  &res->vfe_res[i], i);
>  		if (ret < 0) {
> @@ -1147,7 +1147,7 @@ static int camss_init_subdevices(struct camss *camss)
>  		}
>  	}
>  
> -	for (i = 0; i < camss->csid_num; i++) {
> +	for (i = 0; i < camss->res->csid_num; i++) {
>  		ret = msm_csid_subdev_init(camss, &camss->csid[i],
>  					   &res->csid_res[i], i);
>  		if (ret < 0) {
> @@ -1179,7 +1179,7 @@ static int camss_register_entities(struct camss *camss)
>  	int i, j, k;
>  	int ret;
>  
> -	for (i = 0; i < camss->csiphy_num; i++) {
> +	for (i = 0; i < camss->res->csiphy_num; i++) {
>  		ret = msm_csiphy_register_entity(&camss->csiphy[i],
>  						 &camss->v4l2_dev);
>  		if (ret < 0) {
> @@ -1190,7 +1190,7 @@ static int camss_register_entities(struct camss *camss)
>  		}
>  	}
>  
> -	for (i = 0; i < camss->csid_num; i++) {
> +	for (i = 0; i < camss->res->csid_num; i++) {
>  		ret = msm_csid_register_entity(&camss->csid[i],
>  					       &camss->v4l2_dev);
>  		if (ret < 0) {
> @@ -1209,7 +1209,7 @@ static int camss_register_entities(struct camss *camss)
>  		goto err_reg_ispif;
>  	}
>  
> -	for (i = 0; i < camss->vfe_num + camss->vfe_lite_num; i++) {
> +	for (i = 0; i < camss->res->vfe_total_num; i++) {
>  		ret = msm_vfe_register_entities(&camss->vfe[i],
>  						&camss->v4l2_dev);
>  		if (ret < 0) {
> @@ -1220,8 +1220,8 @@ static int camss_register_entities(struct camss *camss)
>  		}
>  	}
>  
> -	for (i = 0; i < camss->csiphy_num; i++) {
> -		for (j = 0; j < camss->csid_num; j++) {
> +	for (i = 0; i < camss->res->csiphy_num; i++) {
> +		for (j = 0; j < camss->res->csid_num; j++) {
>  			ret = media_create_pad_link(
>  				&camss->csiphy[i].subdev.entity,
>  				MSM_CSIPHY_PAD_SRC,
> @@ -1240,7 +1240,7 @@ static int camss_register_entities(struct camss *camss)
>  	}
>  
>  	if (camss->ispif) {
> -		for (i = 0; i < camss->csid_num; i++) {
> +		for (i = 0; i < camss->res->csid_num; i++) {
>  			for (j = 0; j < camss->ispif->line_num; j++) {
>  				ret = media_create_pad_link(
>  					&camss->csid[i].subdev.entity,
> @@ -1260,7 +1260,7 @@ static int camss_register_entities(struct camss *camss)
>  		}
>  
>  		for (i = 0; i < camss->ispif->line_num; i++)
> -			for (k = 0; k < camss->vfe_num; k++)
> +			for (k = 0; k < camss->res->vfe_num; k++)
>  				for (j = 0; j < camss->vfe[k].line_num; j++) {
>  					struct v4l2_subdev *ispif = &camss->ispif->line[i].subdev;
>  					struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
> @@ -1280,8 +1280,8 @@ static int camss_register_entities(struct camss *camss)
>  					}
>  				}
>  	} else {
> -		for (i = 0; i < camss->csid_num; i++)
> -			for (k = 0; k < camss->vfe_num + camss->vfe_lite_num; k++)
> +		for (i = 0; i < camss->res->csid_num; i++)
> +			for (k = 0; k < camss->res->vfe_total_num; k++)
>  				for (j = 0; j < camss->vfe[k].line_num; j++) {
>  					struct v4l2_subdev *csid = &camss->csid[i].subdev;
>  					struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
> @@ -1305,7 +1305,7 @@ static int camss_register_entities(struct camss *camss)
>  	return 0;
>  
>  err_link:
> -	i = camss->vfe_num + camss->vfe_lite_num;
> +	i = camss->res->vfe_total_num;
>  err_reg_vfe:
>  	for (i--; i >= 0; i--)
>  		msm_vfe_unregister_entities(&camss->vfe[i]);
> @@ -1313,12 +1313,12 @@ static int camss_register_entities(struct camss *camss)
>  err_reg_ispif:
>  	msm_ispif_unregister_entities(camss->ispif);
>  
> -	i = camss->csid_num;
> +	i = camss->res->csid_num;
>  err_reg_csid:
>  	for (i--; i >= 0; i--)
>  		msm_csid_unregister_entity(&camss->csid[i]);
>  
> -	i = camss->csiphy_num;
> +	i = camss->res->csiphy_num;
>  err_reg_csiphy:
>  	for (i--; i >= 0; i--)
>  		msm_csiphy_unregister_entity(&camss->csiphy[i]);
> @@ -1336,15 +1336,15 @@ static void camss_unregister_entities(struct camss *camss)
>  {
>  	unsigned int i;
>  
> -	for (i = 0; i < camss->csiphy_num; i++)
> +	for (i = 0; i < camss->res->csiphy_num; i++)
>  		msm_csiphy_unregister_entity(&camss->csiphy[i]);
>  
> -	for (i = 0; i < camss->csid_num; i++)
> +	for (i = 0; i < camss->res->csid_num; i++)
>  		msm_csid_unregister_entity(&camss->csid[i]);
>  
>  	msm_ispif_unregister_entities(camss->ispif);
>  
> -	for (i = 0; i < camss->vfe_num + camss->vfe_lite_num; i++)
> +	for (i = 0; i < camss->res->vfe_total_num; i++)
>  		msm_vfe_unregister_entities(&camss->vfe[i]);
>  }
>  
> @@ -1463,7 +1463,7 @@ static int camss_configure_pd(struct camss *camss)
>  		}
>  	}
>  
> -	if (i > camss->vfe_num) {
> +	if (i > camss->res->vfe_num) {
>  		camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1],
>  							   DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
>  							   DL_FLAG_RPM_ACTIVE);
> @@ -1509,7 +1509,8 @@ static int camss_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct camss *camss;
> -	int num_subdevs, ret;
> +	int num_subdevs;
> +	int ret;
>  
>  	camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL);
>  	if (!camss)
> @@ -1523,59 +1524,24 @@ static int camss_probe(struct platform_device *pdev)
>  	camss->dev = dev;
>  	platform_set_drvdata(pdev, camss);
>  
> -	if (of_device_is_compatible(dev->of_node, "qcom,msm8916-camss")) {
> -		camss->version = CAMSS_8x16;
> -		camss->csiphy_num = 2;
> -		camss->csid_num = 2;
> -		camss->vfe_num = 1;
> -	} else if (of_device_is_compatible(dev->of_node,
> -					   "qcom,msm8996-camss")) {
> -		camss->version = CAMSS_8x96;
> -		camss->csiphy_num = 3;
> -		camss->csid_num = 4;
> -		camss->vfe_num = 2;
> -	} else if (of_device_is_compatible(dev->of_node,
> -					   "qcom,sdm660-camss")) {
> -		camss->version = CAMSS_660;
> -		camss->csiphy_num = 3;
> -		camss->csid_num = 4;
> -		camss->vfe_num = 2;
> -	} else if (of_device_is_compatible(dev->of_node,
> -					   "qcom,sdm845-camss")) {
> -		camss->version = CAMSS_845;
> -		camss->csiphy_num = 4;
> -		camss->csid_num = 3;
> -		camss->vfe_num = 2;
> -		camss->vfe_lite_num = 1;
> -	} else if (of_device_is_compatible(dev->of_node,
> -					   "qcom,sm8250-camss")) {
> -		camss->version = CAMSS_8250;
> -		camss->csiphy_num = 6;
> -		camss->csid_num = 4;
> -		camss->vfe_num = 2;
> -		camss->vfe_lite_num = 2;
> -	} else {
> -		return -EINVAL;
> -	}
> -
> -	camss->csiphy = devm_kcalloc(dev, camss->csiphy_num,
> +	camss->csiphy = devm_kcalloc(dev, camss->res->csiphy_num,
>  				     sizeof(*camss->csiphy), GFP_KERNEL);
>  	if (!camss->csiphy)
>  		return -ENOMEM;
>  
> -	camss->csid = devm_kcalloc(dev, camss->csid_num, sizeof(*camss->csid),
> +	camss->csid = devm_kcalloc(dev, camss->res->csid_num, sizeof(*camss->csid),
>  				   GFP_KERNEL);
>  	if (!camss->csid)
>  		return -ENOMEM;
>  
> -	if (camss->version == CAMSS_8x16 ||
> -	    camss->version == CAMSS_8x96) {
> +	if (camss->res->version == CAMSS_8x16 ||
> +	    camss->res->version == CAMSS_8x96) {
>  		camss->ispif = devm_kcalloc(dev, 1, sizeof(*camss->ispif), GFP_KERNEL);
>  		if (!camss->ispif)
>  			return -ENOMEM;
>  	}
>  
> -	camss->vfe = devm_kcalloc(dev, camss->vfe_num + camss->vfe_lite_num,
> +	camss->vfe = devm_kcalloc(dev, camss->res->vfe_total_num,
>  				  sizeof(*camss->vfe), GFP_KERNEL);
>  	if (!camss->vfe)
>  		return -ENOMEM;
> @@ -1675,7 +1641,7 @@ void camss_delete(struct camss *camss)
>  	if (camss->genpd_num == 1)
>  		return;
>  
> -	if (camss->genpd_num > camss->vfe_num)
> +	if (camss->genpd_num > camss->res->vfe_num)
>  		device_link_del(camss->genpd_link[camss->genpd_num - 1]);
>  
>  	for (i = 0; i < camss->genpd_num; i++)
> @@ -1701,38 +1667,65 @@ static void camss_remove(struct platform_device *pdev)
>  }
>  
>  static const struct camss_resources msm8916_resources = {
> +	.version = CAMSS_8x16,
>  	.csiphy_res = csiphy_res_8x16,
>  	.csid_res = csid_res_8x16,
>  	.ispif_res = &ispif_res_8x16,
>  	.vfe_res = vfe_res_8x16,
> +	.csiphy_num = ARRAY_SIZE(csiphy_res_8x16),
> +	.csid_num = ARRAY_SIZE(csid_res_8x16),
> +	.vfe_num = ARRAY_SIZE(vfe_res_8x16),
> +	.vfe_total_num = ARRAY_SIZE(vfe_res_8x16),
>  };
>  
>  static const struct camss_resources msm8996_resources = {
> +	.version = CAMSS_8x96,
>  	.csiphy_res = csiphy_res_8x96,
>  	.csid_res = csid_res_8x96,
>  	.ispif_res = &ispif_res_8x96,
>  	.vfe_res = vfe_res_8x96,
> +	.csiphy_num = ARRAY_SIZE(csiphy_res_8x96),
> +	.csid_num = ARRAY_SIZE(csid_res_8x96),
> +	.vfe_num = ARRAY_SIZE(vfe_res_8x96),
> +	.vfe_total_num = ARRAY_SIZE(vfe_res_8x96),
>  };
>  
>  static const struct camss_resources sdm660_resources = {
> +	.version = CAMSS_660,
>  	.csiphy_res = csiphy_res_660,
>  	.csid_res = csid_res_660,
>  	.ispif_res = &ispif_res_660,
>  	.vfe_res = vfe_res_660,
> +	.csiphy_num = ARRAY_SIZE(csiphy_res_660),
> +	.csid_num = ARRAY_SIZE(csid_res_660),
> +	.vfe_num = ARRAY_SIZE(vfe_res_660),
> +	.vfe_total_num = ARRAY_SIZE(vfe_res_660),
>  };
>  
>  static const struct camss_resources sdm845_resources = {
> +	.version = CAMSS_845,
>  	.csiphy_res = csiphy_res_845,
>  	.csid_res = csid_res_845,
>  	.vfe_res = vfe_res_845,
> +	.csiphy_num = ARRAY_SIZE(csiphy_res_845),
> +	.csid_num = ARRAY_SIZE(csid_res_845),
> +	.vfe_num = 2,
> +	.vfe_lite_num = 1,
> +	.vfe_total_num = 3,
>  };
>  
>  static const struct camss_resources sm8250_resources = {
> +	.version = CAMSS_8250,
>  	.csiphy_res = csiphy_res_8250,
>  	.csid_res = csid_res_8250,
>  	.vfe_res = vfe_res_8250,
>  	.icc_res = icc_res_sm8250,
>  	.icc_path_num = ARRAY_SIZE(icc_res_sm8250),
> +	.csiphy_num = ARRAY_SIZE(csiphy_res_8250),
> +	.csid_num = ARRAY_SIZE(csid_res_8250),
> +	.vfe_num = 2,
> +	.vfe_lite_num = 2,
> +	.vfe_total_num = 4,
>  };
>  
>  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 988c313b63551..dd8c58d349685 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -80,27 +80,28 @@ enum icc_count {
>  };
>  
>  struct camss_resources {
> +	enum camss_version version;
>  	const struct resources *csiphy_res;
>  	const struct resources *csid_res;
>  	const struct resources *ispif_res;
>  	const struct resources *vfe_res;
>  	const struct resources_icc *icc_res;
>  	const int icc_path_num;
> +	int csiphy_num;
> +	int csid_num;
> +	int vfe_num;
> +	int vfe_lite_num;
> +	int vfe_total_num;

Should these be unsigned ?

With these small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  };
>  
>  struct camss {
> -	enum camss_version version;
>  	struct v4l2_device v4l2_dev;
>  	struct v4l2_async_notifier notifier;
>  	struct media_device media_dev;
>  	struct device *dev;
> -	int csiphy_num;
>  	struct csiphy_device *csiphy;
> -	int csid_num;
>  	struct csid_device *csid;
>  	struct ispif_device *ispif;
> -	int vfe_num;
> -	int vfe_lite_num;
>  	struct vfe_device *vfe;
>  	atomic_t ref_count;
>  	int genpd_num;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 05/15] media: qcom: camss: Pass line_num from compat resources

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:34AM +0100, Bryan O'Donoghue wrote:
> line_num indicates the number of RDI - raw data interface channels which
> are associated with a given IFE/VFE - image/video front end.

Should the variable then be renamed to num_rdi or similar ?

> On several SoCs the RDI number is not static for each VFE - for example
> on sm8250 VFE Lite has four RDIs where regular VFE has three.
> 
> Assigning line_num statically in the subdev_init() phase initialises
> each VFE to the lower number, meaning in practical terms that we are
> lobbing off one RDI on some VFEs.
> 
> Interrupt handling uses static for (i = RDI0; i < RDI2; i++) {} in some
> of our VFE blocks but this can't work for situations where we have a
> mixture of VFE @ 3 RDI and VFE-lite @ 4 RDI blocks.
> 
> First step to remediate is to pass line_num from a compat string
> controlled data-structure and do so on a per-VFE basis.
> 
> Later patches will assign the correct number of RDI blocks per VFE.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../media/platform/qcom/camss/camss-vfe-170.c |  2 --
>  .../media/platform/qcom/camss/camss-vfe-4-1.c |  2 --
>  .../media/platform/qcom/camss/camss-vfe-4-7.c |  2 --
>  .../media/platform/qcom/camss/camss-vfe-4-8.c |  2 --
>  .../media/platform/qcom/camss/camss-vfe-480.c |  1 -
>  drivers/media/platform/qcom/camss/camss-vfe.c |  5 +++
>  drivers/media/platform/qcom/camss/camss.c     | 36 ++++++++++++-------
>  drivers/media/platform/qcom/camss/camss.h     |  1 +
>  8 files changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> index 9905bb06b3823..8aa921400ded0 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> @@ -756,8 +756,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
>  {
>  	vfe->isr_ops = vfe_isr_ops_170;
>  	vfe->video_ops = vfe_video_ops_170;
> -
> -	vfe->line_num = VFE_LINE_NUM_GEN2;
>  }
>  
>  const struct vfe_hw_ops vfe_ops_170 = {
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
> index bc309f326f519..2911e4126e7ad 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
> @@ -992,8 +992,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
>  	vfe->isr_ops = vfe_isr_ops_gen1;
>  	vfe->ops_gen1 = &vfe_ops_gen1_4_1;
>  	vfe->video_ops = vfe_video_ops_gen1;
> -
> -	vfe->line_num = VFE_LINE_NUM_GEN1;
>  }
>  
>  const struct vfe_hw_ops vfe_ops_4_1 = {
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
> index 8acd76c9746ba..b65ed0fef595e 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-7.c
> @@ -1188,8 +1188,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
>  	vfe->isr_ops = vfe_isr_ops_gen1;
>  	vfe->ops_gen1 = &vfe_ops_gen1_4_7;
>  	vfe->video_ops = vfe_video_ops_gen1;
> -
> -	vfe->line_num = VFE_LINE_NUM_GEN1;
>  }
>  
>  const struct vfe_hw_ops vfe_ops_4_7 = {
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
> index 3a0167ecf873a..7b3805177f037 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
> @@ -1173,8 +1173,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
>  	vfe->isr_ops = vfe_isr_ops_gen1;
>  	vfe->ops_gen1 = &vfe_ops_gen1_4_8;
>  	vfe->video_ops = vfe_video_ops_gen1;
> -
> -	vfe->line_num = VFE_LINE_NUM_GEN1;
>  }
>  
>  const struct vfe_hw_ops vfe_ops_4_8 = {
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
> index 80338efceb9e1..b1a07e846e25b 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
> @@ -572,7 +572,6 @@ static const struct camss_video_ops vfe_video_ops_480 = {
>  static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
>  {
>  	vfe->video_ops = vfe_video_ops_480;
> -	vfe->line_num = MAX_VFE_OUTPUT_LINES;
>  }
>  
>  const struct vfe_hw_ops vfe_ops_480 = {
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 526dd4ab343fe..b789b3b2e4cfd 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -1305,6 +1305,11 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>  	default:
>  		return -EINVAL;
>  	}
> +
> +	if (!res->line_num)
> +		return -EINVAL;
> +
> +	vfe->line_num = res->line_num;
>  	vfe->ops->subdev_init(dev, vfe);
>  
>  	/* Memory */
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index c8a2571e664fe..ce0d86e45fe48 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -123,7 +123,8 @@ static const struct resources vfe_res_8x16[] = {
>  				{ 0 },
>  				{ 0 } },
>  		.reg = { "vfe0" },
> -		.interrupt = { "vfe0" }
> +		.interrupt = { "vfe0" },
> +		.line_num = VFE_LINE_NUM_GEN1,
>  	}
>  };
>  
> @@ -263,7 +264,8 @@ static const struct resources vfe_res_8x96[] = {
>  				{ 0 },
>  				{ 0 } },
>  		.reg = { "vfe0" },
> -		.interrupt = { "vfe0" }
> +		.interrupt = { "vfe0" },
> +		.line_num = VFE_LINE_NUM_GEN1,
>  	},
>  
>  	/* VFE1 */
> @@ -281,7 +283,8 @@ static const struct resources vfe_res_8x96[] = {
>  				{ 0 },
>  				{ 0 } },
>  		.reg = { "vfe1" },
> -		.interrupt = { "vfe1" }
> +		.interrupt = { "vfe1" },
> +		.line_num = VFE_LINE_NUM_GEN1,
>  	}
>  };
>  
> @@ -442,7 +445,8 @@ static const struct resources vfe_res_660[] = {
>  				{ 0 },
>  				{ 0 } },
>  		.reg = { "vfe0" },
> -		.interrupt = { "vfe0" }
> +		.interrupt = { "vfe0" },
> +		.line_num = VFE_LINE_NUM_GEN1,
>  	},
>  
>  	/* VFE1 */
> @@ -463,7 +467,8 @@ static const struct resources vfe_res_660[] = {
>  				{ 0 },
>  				{ 0 } },
>  		.reg = { "vfe1" },
> -		.interrupt = { "vfe1" }
> +		.interrupt = { "vfe1" },
> +		.line_num = VFE_LINE_NUM_GEN1,
>  	}
>  };
>  
> @@ -621,7 +626,8 @@ static const struct resources vfe_res_845[] = {
>  				{ 19200000, 75000000, 384000000, 538666667 },
>  				{ 384000000 } },
>  		.reg = { "vfe0" },
> -		.interrupt = { "vfe0" }
> +		.interrupt = { "vfe0" },
> +		.line_num = VFE_LINE_NUM_GEN2,
>  	},
>  
>  	/* VFE1 */
> @@ -641,7 +647,8 @@ static const struct resources vfe_res_845[] = {
>  				{ 19200000, 75000000, 384000000, 538666667 },
>  				{ 384000000 } },
>  		.reg = { "vfe1" },
> -		.interrupt = { "vfe1" }
> +		.interrupt = { "vfe1" },
> +		.line_num = VFE_LINE_NUM_GEN2,
>  	},
>  
>  	/* VFE-lite */
> @@ -660,7 +667,8 @@ static const struct resources vfe_res_845[] = {
>  				{ 19200000, 75000000, 384000000, 538666667 },
>  				{ 384000000 } },
>  		.reg = { "vfe_lite" },
> -		.interrupt = { "vfe_lite" }
> +		.interrupt = { "vfe_lite" },
> +		.line_num = VFE_LINE_NUM_GEN2,
>  	}
>  };
>  
> @@ -787,7 +795,8 @@ static const struct resources vfe_res_8250[] = {
>  				{ 0 },
>  				{ 0 } },
>  		.reg = { "vfe0" },
> -		.interrupt = { "vfe0" }
> +		.interrupt = { "vfe0" },
> +		.line_num = 4,
>  	},
>  	/* VFE1 */
>  	{
> @@ -805,7 +814,8 @@ static const struct resources vfe_res_8250[] = {
>  				{ 0 },
>  				{ 0 } },
>  		.reg = { "vfe1" },
> -		.interrupt = { "vfe1" }
> +		.interrupt = { "vfe1" },
> +		.line_num = 4,
>  	},
>  	/* VFE2 (lite) */
>  	{
> @@ -822,7 +832,8 @@ static const struct resources vfe_res_8250[] = {
>  				{ 400000000, 480000000 },
>  				{ 0 } },
>  		.reg = { "vfe_lite0" },
> -		.interrupt = { "vfe_lite0" }
> +		.interrupt = { "vfe_lite0" },
> +		.line_num = 4,
>  	},
>  	/* VFE3 (lite) */
>  	{
> @@ -839,7 +850,8 @@ static const struct resources vfe_res_8250[] = {
>  				{ 400000000, 480000000 },
>  				{ 0 } },
>  		.reg = { "vfe_lite1" },
> -		.interrupt = { "vfe_lite1" }
> +		.interrupt = { "vfe_lite1" },
> +		.line_num = 4,
>  	},
>  };
>  
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index dd8c58d349685..101ce6e527931 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -48,6 +48,7 @@ struct resources {
>  	u32 clock_rate[CAMSS_RES_MAX][CAMSS_RES_MAX];
>  	char *reg[CAMSS_RES_MAX];
>  	char *interrupt[CAMSS_RES_MAX];
> +	u8 line_num;
>  };
>  
>  struct icc_bw_tbl {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 06/15] media: qcom: camss: Assign the correct number of RDIs per VFE

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:35AM +0100, Bryan O'Donoghue wrote:
> Each Video Front End - VFE - has a variable number of Raw Data Interfaces -
> RDIs associated with it.
> 
> The CAMSS code started from a naive implementation where a fixed define was
> used as a control in a for(){} loop iterating through RDIs.
> 
> That model scales badly. An attempt was made with  VFE_LINE_NUM_GEN2 and
> VFE_LINE_NUM_GEN1 to differentiate between SoCs but, the problem with that
> is "gen1" and "gen2" have no meaning in the silicon. There is no fixed
> constraint in the silicon between VFE and RDI, it is entirely up to the SoC
> designers how many VFEs are populated and how many RDIs to associate with
> each VFE.
> 
> As an example sdm845 has VFE version 175 and sm8250 VFE version 480.
> sdm845 has 2 VFEs with 4 RDIs and 1 VFE Lite with 4 RDIs.
> sm8250 has 2 VFEs with 3 RDIs and 2 VFE Lite with 4 RDIs.
> 
> Clearly then we need a more granular model to capture the necessary data.
> 
> The defines have gone away to be replaced with per-SoC data but, we haven't
> populated the parameter data with the real values.

I think you forgot to drop the macros from camss-vfe.h.

> 
> Let's call those values out now
> 
> msm8916:
> 1 x VFE
> 3 x RDI per VFE (not 4)
> 
> msm8996:
> 2 x VFE
> 3 x RDI per VFE (not 4)
> 
> sdm660:
> 2 x VFE
> 3 x RDI per VFE (not 4)
> 
> sdm845:
> 2 x VFE
> 4 x RDI per VFE (not 3)
> 1 x VFE Lite
> 4 x RDI per VFE Lite (not 3)
> 
> sm8250:
> 2 x VFE
> 3 x RDI per VFE (not 4)
> 2 x VFE Lite
> 4 x RDI per VFE

Did you mean per VFE Lite here ?

> This more complex and correct mapping was not possible prior to passing
> values via driver data. Now that we have that change in place we can
> correctly map VFEs to RDIs for each VFE.

We could store the value per VFE type (VFE vs. VFE Lite), instead of per
VFE instance, but that would be more complex I suppose, for little gain.
However, if there are more values that depend on the VFE type instead of
the VFE instance, this should be considered.

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/qcom/camss/camss.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index ce0d86e45fe48..c8b8ad176ee2b 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -124,7 +124,7 @@ static const struct resources vfe_res_8x16[] = {
>  				{ 0 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	}
>  };
>  
> @@ -265,7 +265,7 @@ static const struct resources vfe_res_8x96[] = {
>  				{ 0 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	},
>  
>  	/* VFE1 */
> @@ -284,7 +284,7 @@ static const struct resources vfe_res_8x96[] = {
>  				{ 0 } },
>  		.reg = { "vfe1" },
>  		.interrupt = { "vfe1" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	}
>  };
>  
> @@ -446,7 +446,7 @@ static const struct resources vfe_res_660[] = {
>  				{ 0 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	},
>  
>  	/* VFE1 */
> @@ -468,7 +468,7 @@ static const struct resources vfe_res_660[] = {
>  				{ 0 } },
>  		.reg = { "vfe1" },
>  		.interrupt = { "vfe1" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	}
>  };
>  
> @@ -627,7 +627,7 @@ static const struct resources vfe_res_845[] = {
>  				{ 384000000 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = VFE_LINE_NUM_GEN2,
> +		.line_num = 4,
>  	},
>  
>  	/* VFE1 */
> @@ -648,7 +648,7 @@ static const struct resources vfe_res_845[] = {
>  				{ 384000000 } },
>  		.reg = { "vfe1" },
>  		.interrupt = { "vfe1" },
> -		.line_num = VFE_LINE_NUM_GEN2,
> +		.line_num = 4,
>  	},
>  
>  	/* VFE-lite */
> @@ -668,7 +668,7 @@ static const struct resources vfe_res_845[] = {
>  				{ 384000000 } },
>  		.reg = { "vfe_lite" },
>  		.interrupt = { "vfe_lite" },
> -		.line_num = VFE_LINE_NUM_GEN2,
> +		.line_num = 4,
>  	}
>  };
>  
> @@ -796,7 +796,7 @@ static const struct resources vfe_res_8250[] = {
>  				{ 0 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = 4,
> +		.line_num = 3,
>  	},
>  	/* VFE1 */
>  	{
> @@ -815,7 +815,7 @@ static const struct resources vfe_res_8250[] = {
>  				{ 0 } },
>  		.reg = { "vfe1" },
>  		.interrupt = { "vfe1" },
> -		.line_num = 4,
> +		.line_num = 3,
>  	},
>  	/* VFE2 (lite) */
>  	{

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
> From sdm845 onwards we need to ensure the VFE is powered on prior to
> switching on the CSID.

Based on the discussions with Konrad in the mail thread, I would record
here the reason for this requirement.

What happens if you unconditionally power up the VFE prior to switching
the CSID on ? Will this break something on platforms older than SDM845 ?

> Alternatively we could model up the GDSCs and clocks the CSID needs
> without the VFE but, there's a real question of the legitimacy of such a
> use-case.
> 
> For now drawing a line at sdm845 and switching on the associated VFEs is
> a perfectly valid thing to do.
> 
> Rather than continually extend out this clause for at least two new SoCs
> with this same model - making the vfe_get/vfe_put path start to look
> like spaghetti we can encoded the dependency in a helper function.
> 
> Use csid_depends_vfe() for this purpose.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-csid.c    | 20 +++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 08991b070bd61..fd04ed112b564 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid)
>  	return 0;
>  }
>  
> +static bool csid_depends_vfe(u32 version)
> +{
> +	bool ret = false;
> +
> +	switch (version) {
> +	case CAMSS_845:
> +	case CAMSS_8250:
> +		ret = true;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * csid_set_power - Power on/off CSID module
>   * @sd: CSID V4L2 subdevice
> @@ -163,7 +179,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>  	int ret = 0;
>  
>  	if (on) {
> -		if (version == CAMSS_8250 || version == CAMSS_845) {
> +		if (csid_depends_vfe(version)) {
>  			ret = vfe_get(vfe);
>  			if (ret < 0)
>  				return ret;
> @@ -217,7 +233,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>  		regulator_bulk_disable(csid->num_supplies,
>  				       csid->supplies);
>  		pm_runtime_put_sync(dev);
> -		if (version == CAMSS_8250 || version == CAMSS_845)
> +		if (csid_depends_vfe(version))
>  			vfe_put(vfe);
>  	}
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 08/15] media: qcom: camss: Untangle if/else spaghetti in camss

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:37AM +0100, Bryan O'Donoghue wrote:
> I refuse to add another SoC to this convoluted if/else structure. By the
> time we added in a third SoC we ought to have transitioned these control
> flow strutures to switches.
> 
> Adding in another Soc or two will make some of these if statements into
> five clause behemoths.
> 
> Introduce switches in the obvious places to despaghetiify.

I like the switch/case better as well, but the current code isn't as bad
as you make it sound. I don't think you need to make such a personal
statement in the commit message and threaten of starting a hunger strike
if the driver doesn't use switch/case :-) Just say that the switch/case
reads more naturally for this specific usage.

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-csid.c    | 16 ++++---
>  .../media/platform/qcom/camss/camss-csiphy.c  | 22 ++++++---
>  drivers/media/platform/qcom/camss/camss-vfe.c | 45 +++++++++++++------
>  .../media/platform/qcom/camss/camss-video.c   | 16 ++++---
>  4 files changed, 68 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index fd04ed112b564..5dbbcda5232ac 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -592,15 +592,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>  	csid->camss = camss;
>  	csid->id = id;
>  
> -	if (camss->res->version == CAMSS_8x16) {
> +	switch (camss->res->version) {
> +	case CAMSS_8x16:
>  		csid->ops = &csid_ops_4_1;
> -	} else if (camss->res->version == CAMSS_8x96 ||
> -		   camss->res->version == CAMSS_660) {
> +		break;
> +	case CAMSS_8x96:
> +	case CAMSS_660:
>  		csid->ops = &csid_ops_4_7;
> -	} else if (camss->res->version == CAMSS_845 ||
> -		   camss->res->version == CAMSS_8250) {
> +		break;
> +	case CAMSS_845:
> +	case CAMSS_8250:
>  		csid->ops = &csid_ops_gen2;
> -	} else {
> +		break;
> +	default:
>  		return -EINVAL;

This should never happen, as adding support for a new SoC should come
with an update for all the applicable switch/case statements. It's
useful to let the compiler complain if someone forgets to do so, but
with a default case, you will only see the issue at runtime. Could it be
caught at compile time ?

>  	}
>  	csid->ops->subdev_init(csid);
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index 593aec5c97bc2..0e8c2a59ea241 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -557,21 +557,31 @@ int msm_csiphy_subdev_init(struct camss *camss,
>  	csiphy->id = id;
>  	csiphy->cfg.combo_mode = 0;
>  
> -	if (camss->res->version == CAMSS_8x16) {
> +	switch (camss->res->version) {
> +	case CAMSS_8x16:
> +	{
>  		csiphy->ops = &csiphy_ops_2ph_1_0;
>  		csiphy->formats = csiphy_formats_8x16;
>  		csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x16);
> -	} else if (camss->res->version == CAMSS_8x96 ||
> -		   camss->res->version == CAMSS_660) {
> +		break;
> +	}
> +	case CAMSS_8x96:
> +	case CAMSS_660:
> +	{
>  		csiphy->ops = &csiphy_ops_3ph_1_0;
>  		csiphy->formats = csiphy_formats_8x96;
>  		csiphy->nformats = ARRAY_SIZE(csiphy_formats_8x96);
> -	} else if (camss->res->version == CAMSS_845 ||
> -		   camss->res->version == CAMSS_8250) {
> +		break;
> +	}
> +	case CAMSS_845:
> +	case CAMSS_8250:
> +	{
>  		csiphy->ops = &csiphy_ops_3ph_1_0;
>  		csiphy->formats = csiphy_formats_sdm845;
>  		csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845);
> -	} else {
> +		break;
> +	}
> +	default:
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index b789b3b2e4cfd..8f48401e31cd3 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -170,7 +170,9 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>  {
>  	struct vfe_device *vfe = to_vfe(line);
>  
> -	if (vfe->camss->res->version == CAMSS_8x16)
> +	switch (vfe->camss->res->version) {
> +	case CAMSS_8x16:
> +	{
>  		switch (sink_code) {
>  		case MEDIA_BUS_FMT_YUYV8_1X16:
>  		{
> @@ -218,10 +220,13 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>  
>  			return sink_code;
>  		}
> -	else if (vfe->camss->res->version == CAMSS_8x96 ||
> -		 vfe->camss->res->version == CAMSS_660 ||
> -		 vfe->camss->res->version == CAMSS_845 ||
> -		 vfe->camss->res->version == CAMSS_8250)
> +		break;
> +	}
> +	case CAMSS_8x96:
> +	case CAMSS_660:
> +	case CAMSS_845:
> +	case CAMSS_8250:
> +	{
>  		switch (sink_code) {
>  		case MEDIA_BUS_FMT_YUYV8_1X16:
>  		{
> @@ -281,8 +286,12 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>  
>  			return sink_code;
>  		}
> -	else
> -		return 0;
> +		break;
> +	}
> +	default:
> +		break;
> +	}
> +	return 0;
>  }
>  
>  int vfe_reset(struct vfe_device *vfe)
> @@ -1397,7 +1406,9 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>  		init_completion(&l->output.sof);
>  		init_completion(&l->output.reg_update);
>  
> -		if (camss->res->version == CAMSS_8x16) {
> +		switch (camss->res->version) {
> +		case CAMSS_8x16:
> +		{
>  			if (i == VFE_LINE_PIX) {
>  				l->formats = formats_pix_8x16;
>  				l->nformats = ARRAY_SIZE(formats_pix_8x16);
> @@ -1405,8 +1416,11 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>  				l->formats = formats_rdi_8x16;
>  				l->nformats = ARRAY_SIZE(formats_rdi_8x16);
>  			}
> -		} else if (camss->res->version == CAMSS_8x96 ||
> -			   camss->res->version == CAMSS_660) {
> +			break;
> +		}
> +		case CAMSS_8x96:
> +		case CAMSS_660:
> +		{
>  			if (i == VFE_LINE_PIX) {
>  				l->formats = formats_pix_8x96;
>  				l->nformats = ARRAY_SIZE(formats_pix_8x96);
> @@ -1414,11 +1428,16 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
>  				l->formats = formats_rdi_8x96;
>  				l->nformats = ARRAY_SIZE(formats_rdi_8x96);
>  			}
> -		} else if (camss->res->version == CAMSS_845 ||
> -			   camss->res->version == CAMSS_8250) {
> +			break;
> +		}
> +		case CAMSS_845:
> +		case CAMSS_8250:
> +		{
>  			l->formats = formats_rdi_845;
>  			l->nformats = ARRAY_SIZE(formats_rdi_845);
> -		} else {
> +			break;
> +		}
> +		default:
>  			return -EINVAL;
>  		}
>  	}
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 46a89b5f6c171..e695724437ce1 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -1006,7 +1006,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>  
>  	mutex_init(&video->lock);
>  
> -	if (video->camss->res->version == CAMSS_8x16) {
> +	switch (video->camss->res->version) {
> +	case CAMSS_8x16:
>  		if (is_pix) {
>  			video->formats = formats_pix_8x16;
>  			video->nformats = ARRAY_SIZE(formats_pix_8x16);
> @@ -1014,8 +1015,9 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>  			video->formats = formats_rdi_8x16;
>  			video->nformats = ARRAY_SIZE(formats_rdi_8x16);
>  		}
> -	} else if (video->camss->res->version == CAMSS_8x96 ||
> -		   video->camss->res->version == CAMSS_660) {
> +		break;
> +	case CAMSS_8x96:
> +	case CAMSS_660:
>  		if (is_pix) {
>  			video->formats = formats_pix_8x96;
>  			video->nformats = ARRAY_SIZE(formats_pix_8x96);
> @@ -1023,11 +1025,13 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>  			video->formats = formats_rdi_8x96;
>  			video->nformats = ARRAY_SIZE(formats_rdi_8x96);
>  		}
> -	}  else if (video->camss->res->version == CAMSS_845 ||
> -		    video->camss->res->version == CAMSS_8250) {
> +		break;
> +	case CAMSS_845:
> +	case CAMSS_8250:
>  		video->formats = formats_rdi_845;
>  		video->nformats = ARRAY_SIZE(formats_rdi_845);
> -	} else {
> +		break;
> +	default:
>  		ret = -EINVAL;
>  		goto error_video_register;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 10/15] media: qcom: camss: Allow clocks vfeN vfe_liteN or vfe_lite

[Laurent Pinchart]

On Wed, Aug 23, 2023 at 11:44:39AM +0100, Bryan O'Donoghue wrote:
> The number of Video Front End - VFE or Image Front End - IFE supported
> with new SoCs can vary both for the full and lite cases.
> 
> For example sdm845 has one vfe_lite and two vfe interfaces with the vfe
> clock called simply "vfe_lite" with no integer postfix. sc8280xp has four
> vfe and four vfe lite blocks.
> 
> We need to support the following clock name formats
> 
> - vfeN
> - vfe_liteN
> - vfe_lite
> 
> with N being any reasonably sized integer.
> 
> There are two sites in this code which need to do the same thing,
> constructing and matching strings with the pattern above, so encapsulate
> the logic in one function.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss-vfe.c | 22 ++++++++++++++-----
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 8f48401e31cd3..73380e75dbb22 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -437,6 +437,20 @@ void vfe_isr_reset_ack(struct vfe_device *vfe)
>  	complete(&vfe->reset_complete);
>  }
>  
> +static int vfe_match_clock_names(struct vfe_device *vfe,
> +				 struct camss_clock *clock)
> +{
> +	char vfe_name[CAMSS_RES_MAX];
> +	char vfe_lite_name[CAMSS_RES_MAX];
> +
> +	snprintf(vfe_name, sizeof(vfe_name), "vfe%d", vfe->id);
> +	snprintf(vfe_lite_name, sizeof(vfe_lite_name), "vfe_lite%d", vfe->id);
> +
> +	return (!strcmp(clock->name, vfe_name) ||
> +		!strcmp(clock->name, vfe_lite_name) ||
> +		!strcmp(clock->name, "vfe_lite"));

The code below will match "vfe0", "vfe1" and "vfe_lite", which the code
here matches "vfe%d", "vfe_life%d" and "vfe_lite". The commit message
doesn't explain why you can restrict the clock name to "vfe%d" instead
of matching both "vfe0" and "vfe1". I could try to guess, but it's
better to clarify it in the commit message.

> +}
> +
>  /*
>   * vfe_set_clock_rates - Calculate and set clock rates on VFE module
>   * @vfe: VFE device
> @@ -460,9 +474,7 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
>  	for (i = 0; i < vfe->nclocks; i++) {
>  		struct camss_clock *clock = &vfe->clock[i];
>  
> -		if (!strcmp(clock->name, "vfe0") ||
> -		    !strcmp(clock->name, "vfe1") ||
> -		    !strcmp(clock->name, "vfe_lite")) {
> +		if (vfe_match_clock_names(vfe, clock)) {
>  			u64 min_rate = 0;
>  			long rate;
>  
> @@ -543,9 +555,7 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>  	for (i = 0; i < vfe->nclocks; i++) {
>  		struct camss_clock *clock = &vfe->clock[i];
>  
> -		if (!strcmp(clock->name, "vfe0") ||
> -		    !strcmp(clock->name, "vfe1") ||
> -		    !strcmp(clock->name, "vfe_lite")) {
> +		if (vfe_match_clock_names(vfe, clock)) {
>  			u64 min_rate = 0;
>  			unsigned long rate;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 11/15] media: qcom: camss: Functionally decompose CSIPHY clock lookups

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:40AM +0100, Bryan O'Donoghue wrote:
> The csiphyX_timer and csiX_phy values need not be hard-coded. We can
> functionally decompose the string matching inside of a loop.
> 
> Static string values are brittle, difficult to extend and not required
> anyway since the camss->res->csiphy_num value informs us of the number
> of CSIPHYs and hence the set of potential clocks for a given CSIPHY.
> 
> In simple terms if we have five CSIPHYs we can have no more and no less
> than five csiphy_timer clocks. Similarly csi_phy core clocks have a 1:1
> relationship with the PHY they clock.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-csiphy.c  | 37 ++++++++++++-------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index 0e8c2a59ea241..baf78c525fbfc 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -536,6 +536,15 @@ static int csiphy_init_formats(struct v4l2_subdev *sd,
>  	return csiphy_set_format(sd, fh ? fh->state : NULL, &format);
>  }
>  
> +static bool csiphy_match_clock_name(const char *clock_name, const char *format,
> +				    int index)
> +{
> +	char name[CAMSS_RES_MAX];
> +
> +	snprintf(name, sizeof(name), format, index);
> +	return !strcmp(clock_name, name);

This is very error-prone. You hide the buffer size from the caller,
which will make it prone to buffer overflows.

> +}
> +
>  /*
>   * msm_csiphy_subdev_init - Initialize CSIPHY device structure and resources
>   * @csiphy: CSIPHY device
> @@ -550,7 +559,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
>  {
>  	struct device *dev = camss->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
> -	int i, j;
> +	int i, j, k;
>  	int ret;
>  
>  	csiphy->camss = camss;
> @@ -666,19 +675,19 @@ int msm_csiphy_subdev_init(struct camss *camss,
>  		for (j = 0; j < clock->nfreqs; j++)
>  			clock->freq[j] = res->clock_rate[i][j];
>  
> -		if (!strcmp(clock->name, "csiphy0_timer") ||
> -		    !strcmp(clock->name, "csiphy1_timer") ||
> -		    !strcmp(clock->name, "csiphy2_timer") ||
> -		    !strcmp(clock->name, "csiphy3_timer") ||
> -		    !strcmp(clock->name, "csiphy4_timer") ||
> -		    !strcmp(clock->name, "csiphy5_timer"))
> -			csiphy->rate_set[i] = true;
> -
> -		if (camss->res->version == CAMSS_660 &&
> -		    (!strcmp(clock->name, "csi0_phy") ||
> -		     !strcmp(clock->name, "csi1_phy") ||
> -		     !strcmp(clock->name, "csi2_phy")))
> -			csiphy->rate_set[i] = true;
> +		for (k = 0; k < camss->res->csiphy_num; k++) {
> +			csiphy->rate_set[i] = csiphy_match_clock_name(clock->name,
> +								      "csiphy%d_timer", k);
> +			if (csiphy->rate_set[i])
> +				break;
> +
> +			if (camss->res->version == CAMSS_660) {
> +				csiphy->rate_set[i] = csiphy_match_clock_name(clock->name,
> +									      "csi%d_phy", k);
> +				if (csiphy->rate_set[i])
> +					break;
> +			}
> +		}
>  	}
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 13/15] media: qcom: camss: Support RDI3 for VFE 17x

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:42AM +0100, Bryan O'Donoghue wrote:
> Some VFEs have four RDIs apiece. Right now the ISR code has a hard-coded
> value which tops-out at RDI2 meaning only three RDIs can be utilised in
> practice.
> 
> Extend out the various routines in camss-vfe-17x.c to support the higher
> RDI count.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/qcom/camss/camss-vfe-170.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> index 8aa921400ded0..c2487a8196db7 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> @@ -210,7 +210,8 @@ static void vfe_global_reset(struct vfe_device *vfe)
>  			 GLOBAL_RESET_CMD_IDLE_CGC	|
>  			 GLOBAL_RESET_CMD_RDI0		|
>  			 GLOBAL_RESET_CMD_RDI1		|
> -			 GLOBAL_RESET_CMD_RDI2;
> +			 GLOBAL_RESET_CMD_RDI2		|
> +			 GLOBAL_RESET_CMD_RDI3;
>  
>  	writel_relaxed(BIT(31), vfe->base + VFE_IRQ_MASK_0);
>  
> @@ -344,7 +345,7 @@ static void vfe_violation_read(struct vfe_device *vfe)
>  static irqreturn_t vfe_isr(int irq, void *dev)
>  {
>  	struct vfe_device *vfe = dev;
> -	u32 status0, status1, vfe_bus_status[3];
> +	u32 status0, status1, vfe_bus_status[VFE_LINE_NUM_MAX];
>  	int i, wm;
>  
>  	status0 = readl_relaxed(vfe->base + VFE_IRQ_STATUS_0);
> @@ -353,7 +354,7 @@ static irqreturn_t vfe_isr(int irq, void *dev)
>  	writel_relaxed(status0, vfe->base + VFE_IRQ_CLEAR_0);
>  	writel_relaxed(status1, vfe->base + VFE_IRQ_CLEAR_1);
>  
> -	for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++) {
> +	for (i = VFE_LINE_RDI0; i < vfe->line_num; i++) {
>  		vfe_bus_status[i] = readl_relaxed(vfe->base + VFE_BUS_IRQ_STATUS(i));
>  		writel_relaxed(vfe_bus_status[i], vfe->base + VFE_BUS_IRQ_CLEAR(i));
>  	}
> @@ -367,11 +368,11 @@ static irqreturn_t vfe_isr(int irq, void *dev)
>  	if (status0 & STATUS_0_RESET_ACK)
>  		vfe->isr_ops.reset_ack(vfe);
>  
> -	for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++)
> +	for (i = VFE_LINE_RDI0; i < vfe->line_num; i++)
>  		if (status0 & STATUS_0_RDI_REG_UPDATE(i))
>  			vfe->isr_ops.reg_update(vfe, i);
>  
> -	for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++)
> +	for (i = VFE_LINE_RDI0; i < vfe->line_num; i++)
>  		if (status0 & STATUS_1_RDI_SOF(i))
>  			vfe->isr_ops.sof(vfe, i);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 05/15] media: qcom: camss: Pass line_num from compat resources

[Bryan O'Donoghue]

On 28/08/2023 19:36, Laurent Pinchart wrote:
> Should the variable then be renamed to num_rdi or similar ?

Yes but, the use of "line num" is pretty extensive. I plan a series to 
normalise the namespace a bit later on. I'm just trying to scope limit 
the extent of the changes in one blob.

- rdi_num
- size of struct resource strings

being two that are "high up" on my TODOs.

---
bod

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Bryan O'Donoghue]

On 28/08/2023 19:47, Laurent Pinchart wrote:
> Hi Bryan,
> 
> Thank you for the patch.
> 
> On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
>>  From sdm845 onwards we need to ensure the VFE is powered on prior to
>> switching on the CSID.
> 
> Based on the discussions with Konrad in the mail thread, I would record
> here the reason for this requirement.
> 
> What happens if you unconditionally power up the VFE prior to switching
> the CSID on ? Will this break something on platforms older than SDM845 ?

vfe->power_count will bump and you will be fine.

---
bod

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Laurent Pinchart]

On Mon, Aug 28, 2023 at 08:37:44PM +0100, Bryan O'Donoghue wrote:
> On 28/08/2023 19:47, Laurent Pinchart wrote:
> > Hi Bryan,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
> >>  From sdm845 onwards we need to ensure the VFE is powered on prior to
> >> switching on the CSID.
> > 
> > Based on the discussions with Konrad in the mail thread, I would record
> > here the reason for this requirement.
> > 
> > What happens if you unconditionally power up the VFE prior to switching
> > the CSID on ? Will this break something on platforms older than SDM845 ?
> 
> vfe->power_count will bump and you will be fine.

Then maybe that would be a simpler solution than having a
device-specific power sequence ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Bryan O'Donoghue]

On 28/08/2023 20:40, Laurent Pinchart wrote:
> On Mon, Aug 28, 2023 at 08:37:44PM +0100, Bryan O'Donoghue wrote:
>> On 28/08/2023 19:47, Laurent Pinchart wrote:
>>> Hi Bryan,
>>>
>>> Thank you for the patch.
>>>
>>> On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
>>>>   From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>> switching on the CSID.
>>>
>>> Based on the discussions with Konrad in the mail thread, I would record
>>> here the reason for this requirement.
>>>
>>> What happens if you unconditionally power up the VFE prior to switching
>>> the CSID on ? Will this break something on platforms older than SDM845 ?
>>
>> vfe->power_count will bump and you will be fine.
> 
> Then maybe that would be a simpler solution than having a
> device-specific power sequence ?
> 

A fair point.

Worth a try.

---
bod

Re: [PATCH v3 08/15] media: qcom: camss: Untangle if/else spaghetti in camss

[Bryan O'Donoghue]

On 28/08/2023 19:51, Laurent Pinchart wrote:
>> +		break;
>> +	default:
>>   		return -EINVAL;
> This should never happen, as adding support for a new SoC should come
> with an update for all the applicable switch/case statements. It's
> useful to let the compiler complain if someone forgets to do so, but
> with a default case, you will only see the issue at runtime. Could it be
> caught at compile time ?
> 

Off the top of my head, I don't think it could be easily caught.

An assert() would catch it early in runtime..

---
bod

Re: [PATCH v3 07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

[Bryan O'Donoghue]

On 28/08/2023 20:40, Laurent Pinchart wrote:
> On Mon, Aug 28, 2023 at 08:37:44PM +0100, Bryan O'Donoghue wrote:
>> On 28/08/2023 19:47, Laurent Pinchart wrote:
>>> Hi Bryan,
>>>
>>> Thank you for the patch.
>>>
>>> On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
>>>>   From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>> switching on the CSID.
>>>
>>> Based on the discussions with Konrad in the mail thread, I would record
>>> here the reason for this requirement.
>>>
>>> What happens if you unconditionally power up the VFE prior to switching
>>> the CSID on ? Will this break something on platforms older than SDM845 ?
>>
>> vfe->power_count will bump and you will be fine.
> 
> Then maybe that would be a simpler solution than having a
> device-specific power sequence ?
> 

So, this works.

I'll send out a patch based on this suggestion.

---
bod

Re: [PATCH v3 08/15] media: qcom: camss: Untangle if/else spaghetti in camss

[Bryan O'Donoghue]

On 28/08/2023 19:51, Laurent Pinchart wrote:
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -592,15 +592,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>>   	csid->camss = camss;
>>   	csid->id = id;
>>   
>> -	if (camss->res->version == CAMSS_8x16) {
>> +	switch (camss->res->version) {
>> +	case CAMSS_8x16:
>>   		csid->ops = &csid_ops_4_1;
>> -	} else if (camss->res->version == CAMSS_8x96 ||
>> -		   camss->res->version == CAMSS_660) {
>> +		break;
>> +	case CAMSS_8x96:
>> +	case CAMSS_660:
>>   		csid->ops = &csid_ops_4_7;
>> -	} else if (camss->res->version == CAMSS_845 ||
>> -		   camss->res->version == CAMSS_8250) {
>> +		break;
>> +	case CAMSS_845:
>> +	case CAMSS_8250:
>>   		csid->ops = &csid_ops_gen2;
>> -	} else {
>> +		break;
>> +	default:
>>   		return -EINVAL;
> This should never happen, as adding support for a new SoC should come
> with an update for all the applicable switch/case statements. It's
> useful to let the compiler complain if someone forgets to do so, but
> with a default case, you will only see the issue at runtime. Could it be
> caught at compile time ?
> 

This can be done in fact.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wswitch_002denum-303

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

typedef enum {
         MO = 0,
         LARRY,
         CURLY,
         BINGO,
}my_type;

int main (int argc, char *argv[])
{
         my_type x;
         time_t t;

         srand((unsigned) time(&t));

         x = rand() % BINGO;

         switch(x) {
         case MO:
                 printf("mo\n");
                 break;
         case LARRY:
                 printf("larry\n");
                 break;
         default:
                 printf("blargh\n");
                 break;

         }

         return 0;
}

gcc -o test test.c -Wswitch-enum
test.c: In function ‘main’:
test.c:38:9: warning: enumeration value ‘CURLY’ not handled in switch 
[-Wswitch-enum]
    38 |         switch(x) {
       |         ^~~~~~

It looks like we only enable that switch for tools though

grep -r "Wswitch-enum" *
tools/scripts/Makefile.include:EXTRA_WARNINGS += -Wswitch-enum
tools/bpf/bpftool/Makefile:CFLAGS += $(filter-out -Wswitch-enum 
-Wnested-externs,$(EXTRA_WARNINGS))

I'll still implement the code though, since if we do introduce the 
switch for the kernel it would be caught.

---
bod

Re: [PATCH v3 12/15] media: qcom: camss: Fix support for setting CSIPHY clock name csiphyX

[Bryan O'Donoghue]

On 26/08/2023 13:12, Konrad Dybcio wrote:
>> I really don't understand your point. Could you please restate it ?
> If we categorized the clocks at probe time (these ones go to csiphy, these
> ones go to vfe or whatever), name matching like this could be avoided
> 
> Konrad

Yes, I like this idea.

I'd like to make that into a separate series. So I'd like to address 
your concern on the size of the string in the lookup and then punt the 
clock story over to another series since it will involved churning 
though a fair chunk of code, yaml and dtsi.

---
bod

Re: [PATCH v3 12/15] media: qcom: camss: Fix support for setting CSIPHY clock name csiphyX

[Konrad Dybcio]

On 4.09.2023 21:11, Bryan O'Donoghue wrote:
> On 26/08/2023 13:12, Konrad Dybcio wrote:
>>> I really don't understand your point. Could you please restate it ?
>> If we categorized the clocks at probe time (these ones go to csiphy, these
>> ones go to vfe or whatever), name matching like this could be avoided
>>
>> Konrad
> 
> Yes, I like this idea.
> 
> I'd like to make that into a separate series. So I'd like to address your concern on the size of the string in the lookup and then punt the clock story over to another series since it will involved churning though a fair chunk of code, yaml and dtsi.
I can only think of code changes, but fine, this series is large as-is.

Konrad

Re: [PATCH v3 12/15] media: qcom: camss: Fix support for setting CSIPHY clock name csiphyX

[Bryan O'Donoghue]

On 04/09/2023 20:32, Konrad Dybcio wrote:
> On 4.09.2023 21:11, Bryan O'Donoghue wrote:
>> On 26/08/2023 13:12, Konrad Dybcio wrote:
>>>> I really don't understand your point. Could you please restate it ?
>>> If we categorized the clocks at probe time (these ones go to csiphy, these
>>> ones go to vfe or whatever), name matching like this could be avoided
>>>
>>> Konrad
>>
>> Yes, I like this idea.
>>
>> I'd like to make that into a separate series. So I'd like to address your concern on the size of the string in the lookup and then punt the clock story over to another series since it will involved churning though a fair chunk of code, yaml and dtsi.
> I can only think of code changes, but fine, this series is large as-is.
> 
> Konrad

Agreed.

I pinky-swear to do this series..

---
bod