On 11/18/23 13:11, Bryan O'Donoghue wrote:
Moving the location of the hooks to VFE power domains has several
advantages.
1. Separation of concerns and functional decomposition.
vfe.c should be responsible for and know best how manage
power-domains for a VFE, excising from camss.c follows this
principle.
2. Embedding a pointer to genpd in struct camss_vfe{} meas that we can
dispense with a bunch of kmalloc array inside of camss.c.
3. Splitting up titan top gdsc from vfe/ife gdsc provides a base for
breaking up magic indexes in dtsi.
Suggested-by: Matti Lehtimäki <matti.lehtimaki@xxxxxxxxx>
Tested-by: Matti Lehtimäki <matti.lehtimaki@xxxxxxxxx>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
---
drivers/media/platform/qcom/camss/camss-vfe.c | 24 +++++++++-
drivers/media/platform/qcom/camss/camss-vfe.h | 2 +
drivers/media/platform/qcom/camss/camss.c | 67 ++++++++++++++-------------
drivers/media/platform/qcom/camss/camss.h | 4 +-
4 files changed, 62 insertions(+), 35 deletions(-)
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 5172eb5612a1c..defff24f07ce3 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -14,6 +14,7 @@
#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
#include <linux/spinlock_types.h>
#include <linux/spinlock.h>
@@ -1381,8 +1382,13 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
if (!res->line_num)
return -EINVAL;
- if (res->has_pd)
- vfe->genpd = camss->genpd[id];
+ if (res->has_pd) {
+ vfe->genpd = dev_pm_domain_attach_by_id(camss->dev, id);
+ if (IS_ERR(vfe->genpd)) {
+ ret = PTR_ERR(vfe->genpd);
+ return ret;
Can't help but notice the two lines above could become one
[...]
+/*
+ * msm_vfe_genpd_cleanup - Cleanup VFE genpd linkages
+ * @vfe: VFE device
+ *
stray newline?
+ */
+void msm_vfe_genpd_cleanup(struct vfe_device *vfe)
+{
+ if (vfe->genpd_link)
+ device_link_del(vfe->genpd_link);
+
+ if (vfe->genpd)
+ dev_pm_domain_detach(vfe->genpd, true);
+}
+
/*
* vfe_link_setup - Setup VFE connections
* @entity: Pointer to media entity structure
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index 992a2103ec44c..cdbe59d8d437e 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h
@@ -159,6 +159,8 @@ struct camss_subdev_resources;
int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
const struct camss_subdev_resources *res, u8 id);
+void msm_vfe_genpd_cleanup(struct vfe_device *vfe);
+
int msm_vfe_register_entities(struct vfe_device *vfe,
struct v4l2_device *v4l2_dev);
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index ed01a3ac7a38e..5f7a3b17e25d7 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1487,7 +1487,9 @@ static const struct media_device_ops camss_media_ops = {
static int camss_configure_pd(struct camss *camss)
{
struct device *dev = camss->dev;
+ const struct camss_resources *res = camss->res;
int i;
+ int vfepd_num;
int ret;
Reverse-Christmas-tree, please
[...]
+static void camss_genpd_cleanup(struct camss *camss)
+{
if (camss->genpd_num == 1)
return;
- if (camss->genpd_num > camss->res->vfe_num)
- device_link_del(camss->genpd_link[camss->genpd_num - 1]);
+ if (camss->genpd_link)
+ device_link_del(camss->genpd_link);
+
+ dev_pm_domain_detach(camss->genpd, true);
- for (i = 0; i < camss->genpd_num; i++)
- dev_pm_domain_detach(camss->genpd[i], true);
+ camss_genpd_subdevice_cleanup(camss);
This changes the behavior, previously CAMSS_TOP was shut down last
(which makes more sense to me, anyway)
otherwise, I think this lgtm
Konrad
