Re: [PATCH v5 4/7] media: qcom: camss: Move VFE power-domain specifics into vfe.c

[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@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