Messages in this thread | | | Date | Thu, 23 Nov 2023 13:04:34 +0100 | Subject | Re: [PATCH v5 4/7] media: qcom: camss: Move VFE power-domain specifics into vfe.c | From | Konrad Dybcio <> |
| |
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@gmail.com> > Tested-by: Matti Lehtimäki <matti.lehtimaki@gmail.com> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > 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
| |