lkml.org 
[lkml]   [2023]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v5 4/7] media: qcom: camss: Move VFE power-domain specifics into vfe.c
    From


    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

    \
     
     \ /
      Last update: 2023-11-23 13:05    [W:3.002 / U:0.168 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site