Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <84a97960-dfef-4d2f-8462-d3acb6f5a125@linaro.org>
Date:   Thu, 23 Nov 2023 13:04:34 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 4/7] media: qcom: camss: Move VFE power-domain
 specifics into vfe.c
Content-Language: en-US
To:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        hverkuil-cisco@xs4all.nl, laurent.pinchart@ideasonboard.com,
        Robert Foss <rfoss@kernel.org>,
        Todor Tomov <todor.too@gmail.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        matti.lehtimaki@gmail.com, quic_grosikop@quicinc.com
Cc:     linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20231118-b4-camss-named-power-domains-v5-0-55eb0f35a30a@linaro.org>
 <20231118-b4-camss-named-power-domains-v5-4-55eb0f35a30a@linaro.org>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <20231118-b4-camss-named-power-domains-v5-4-55eb0f35a30a@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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