Re: [PATCH v1 3/4] drm/msm/mdp4: move resource allocation to the _probe function

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 6/20/2022 2:30 PM, Dmitry Baryshkov wrote:
> To let the probe function bail early if any of the resources is
> unavailable, move resource allocattion from kms_init directly to the
> probe callback. While we are at it, replace irq_of_parse_and_map() with
> platform_get_irq().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 107 +++++++++++------------
>   1 file changed, 51 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 41dc60784847..6499713eccf6 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -139,8 +139,6 @@ static void mdp4_destroy(struct msm_kms *kms)
>   		pm_runtime_disable(dev);
>   
>   	mdp_kms_destroy(&mdp4_kms->base);
> -
> -	kfree(mdp4_kms);
>   }
>   
>   static const struct mdp_kms_funcs kms_funcs = {
> @@ -383,57 +381,27 @@ static int mdp4_kms_init(struct drm_device *dev)
>   {
>   	struct platform_device *pdev = to_platform_device(dev->dev);
>   	struct msm_drm_private *priv = dev->dev_private;
> -	struct mdp4_kms *mdp4_kms;
> +	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(priv->kms));
>   	struct msm_kms *kms = NULL;
>   	struct iommu_domain *iommu;
>   	struct msm_gem_address_space *aspace;
> -	int irq, ret;
> +	int ret;
>   	u32 major, minor;
>   	unsigned long max_clk;
>   
>   	/* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */
>   	max_clk = 266667000;
>   
> -	mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL);
> -	if (!mdp4_kms) {
> -		DRM_DEV_ERROR(dev->dev, "failed to allocate kms\n");
> -		return -ENOMEM;
> -	}
> -
>   	ret = mdp_kms_init(&mdp4_kms->base, &kms_funcs);
>   	if (ret) {
>   		DRM_DEV_ERROR(dev->dev, "failed to init kms\n");
>   		goto fail;
>   	}
>   
> -	priv->kms = &mdp4_kms->base.base;
>   	kms = priv->kms;
>   
>   	mdp4_kms->dev = dev;
>   
> -	mdp4_kms->mmio = msm_ioremap(pdev, NULL);
> -	if (IS_ERR(mdp4_kms->mmio)) {
> -		ret = PTR_ERR(mdp4_kms->mmio);
> -		goto fail;
> -	}
> -
> -	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0) {
> -		ret = irq;
> -		DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret);
> -		goto fail;
> -	}
> -
> -	kms->irq = irq;
> -
> -	/* NOTE: driver for this regulator still missing upstream.. use
> -	 * _get_exclusive() and ignore the error if it does not exist
> -	 * (and hope that the bootloader left it on for us)
> -	 */
> -	mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd");
> -	if (IS_ERR(mdp4_kms->vdd))
> -		mdp4_kms->vdd = NULL;
> -
>   	if (mdp4_kms->vdd) {
>   		ret = regulator_enable(mdp4_kms->vdd);
>   		if (ret) {
> @@ -442,24 +410,6 @@ static int mdp4_kms_init(struct drm_device *dev)
>   		}
>   	}
>   
> -	mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk");
> -	if (IS_ERR(mdp4_kms->clk)) {
> -		DRM_DEV_ERROR(dev->dev, "failed to get core_clk\n");
> -		ret = PTR_ERR(mdp4_kms->clk);
> -		goto fail;
> -	}
> -
> -	mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk");
> -	if (IS_ERR(mdp4_kms->pclk))
> -		mdp4_kms->pclk = NULL;
> -
> -	mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk");
> -	if (IS_ERR(mdp4_kms->axi_clk)) {
> -		DRM_DEV_ERROR(dev->dev, "failed to get axi_clk\n");
> -		ret = PTR_ERR(mdp4_kms->axi_clk);
> -		goto fail;
> -	}
> -
>   	clk_set_rate(mdp4_kms->clk, max_clk);
>   
>   	read_mdp_hw_revision(mdp4_kms, &major, &minor);
> @@ -474,10 +424,9 @@ static int mdp4_kms_init(struct drm_device *dev)
>   	mdp4_kms->rev = minor;
>   
>   	if (mdp4_kms->rev >= 2) {
> -		mdp4_kms->lut_clk = devm_clk_get(&pdev->dev, "lut_clk");
> -		if (IS_ERR(mdp4_kms->lut_clk)) {
> +		if (!mdp4_kms->lut_clk) {
>   			DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n");
> -			ret = PTR_ERR(mdp4_kms->lut_clk);
> +			ret = -ENODEV;
>   			goto fail;
>   		}
>   		clk_set_rate(mdp4_kms->lut_clk, max_clk);
> @@ -560,7 +509,53 @@ static const struct dev_pm_ops mdp4_pm_ops = {
>   
>   static int mdp4_probe(struct platform_device *pdev)
>   {
> -	return msm_drv_probe(&pdev->dev, mdp4_kms_init, NULL);
> +	struct device *dev = &pdev->dev;
> +	struct mdp4_kms *mdp4_kms;
> +	int irq;
> +
> +	mdp4_kms = devm_kzalloc(dev, sizeof(*mdp4_kms), GFP_KERNEL);
> +	if (!mdp4_kms)
> +		return dev_err_probe(dev, -ENOMEM, "failed to allocate kms\n");
> +
> +	mdp4_kms->mmio = msm_ioremap(pdev, NULL);
> +	if (IS_ERR(mdp4_kms->mmio))
> +		return PTR_ERR(mdp4_kms->mmio);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq, "failed to get irq\n");
> +
> +	mdp4_kms->base.base.irq = irq;
> +
> +	/* NOTE: driver for this regulator still missing upstream.. use
> +	 * _get_exclusive() and ignore the error if it does not exist
> +	 * (and hope that the bootloader left it on for us)
> +	 */
> +	mdp4_kms->vdd = devm_regulator_get_exclusive(&pdev->dev, "vdd");
> +	if (IS_ERR(mdp4_kms->vdd))
> +		mdp4_kms->vdd = NULL;
> +
> +	mdp4_kms->clk = devm_clk_get(&pdev->dev, "core_clk");
> +	if (IS_ERR(mdp4_kms->clk))
> +		return dev_err_probe(dev, PTR_ERR(mdp4_kms->clk), "failed to get core_clk\n");
> +
> +	mdp4_kms->pclk = devm_clk_get(&pdev->dev, "iface_clk");
> +	if (IS_ERR(mdp4_kms->pclk))
> +		mdp4_kms->pclk = NULL;
> +
> +	mdp4_kms->axi_clk = devm_clk_get(&pdev->dev, "bus_clk");
> +	if (IS_ERR(mdp4_kms->axi_clk))
> +		return dev_err_probe(dev, PTR_ERR(mdp4_kms->axi_clk), "failed to get axi_clk\n");
> +
> +	/*
> +	 * This is required for revn >= 2. Handle errors here and let the kms
> +	 * init bail out if the clock is not provided.
> +	 */
> +	mdp4_kms->lut_clk = devm_clk_get_optional(&pdev->dev, "lut_clk");
> +	if (IS_ERR(mdp4_kms->lut_clk))
> +		return dev_err_probe(dev, PTR_ERR(mdp4_kms->lut_clk), "failed to get lut_clk\n");

I can see that you have moved this from init to probe and only rev >=2 
needs it.

But, your check here will end up returning from probe because you have a 
return. So I guess you means just having dev_err_probe without the 
return and let the init fail if the clk is not found because we have the 
hw_rev only in init.

> +
> +	return msm_drv_probe(&pdev->dev, mdp4_kms_init, &mdp4_kms->base.base);
>   }
>   
>   static int mdp4_remove(struct platform_device *pdev)