Re: [PATCH 1/2] drm/exynos: consider deferred probe case

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Inki,

On 05/27/2014 02:42 PM, Inki Dae wrote:
> This patch makes sure that exynos drm framework handles deferred
> probe case correctly.
>
> Sub drivers could be probed before resources, clock, regulator,
> phy or panel, are ready for them so we should make sure that exynos
> drm core waits until all resources are ready and sub drivers are
> probed correctly.
>
> Signed-off-by: Inki Dae <inki.dae@xxxxxxxxxxx>
> Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/exynos/exynos_dp_core.c  |   16 +++++++-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  |   28 +++++++-------
>  drivers/gpu/drm/exynos/exynos_drm_drv.h  |    7 +---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c  |   31 +++++++++++----
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   49 +++++++++++++++++------
>  drivers/gpu/drm/exynos/exynos_hdmi.c     |   62 ++++++++++++++++++++++--------
>  drivers/gpu/drm/exynos/exynos_mixer.c    |   16 +++++++-
>  7 files changed, 151 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index ff63901..7526f36 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -1328,12 +1328,24 @@ static const struct component_ops exynos_dp_ops = {
>  
>  static int exynos_dp_probe(struct platform_device *pdev)
>  {
> -	return exynos_drm_component_add(&pdev->dev, &exynos_dp_ops);
> +	int ret;
> +
> +	ret = exynos_drm_component_add(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = component_add(&pdev->dev, &exynos_dp_ops);
> +	if (ret)
> +		exynos_drm_component_del(&pdev->dev);
> +
> +	return ret;

Hmm, exynos_drm_component_add adds component to the list of devices drm
master
should wait for and it is called from probe of each component. It is the
same concept
as the one I have proposed some times ago [1], the main difference is
that this one
is build on top of the component framework :)

[1]: http://permalink.gmane.org/gmane.comp.video.dri.devel/103817

Regarding the implementation, to allow it work with drivers unbinding
exynos_drm_component_del
should be removed and drm_component_list should be cleaned on drm module
removal.
This will also decrease significantly patch size, all those "goto
err_del_component" can be removed.

Two additional small comments below.

>  }
>  
>  static int exynos_dp_remove(struct platform_device *pdev)
>  {
> -	exynos_drm_component_del(&pdev->dev, &exynos_dp_ops);
> +	component_del(&pdev->dev, &exynos_dp_ops);
> +	exynos_drm_component_del(&pdev->dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index c5a401ae..f611f5a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -381,22 +381,25 @@ static const struct dev_pm_ops exynos_drm_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(exynos_drm_sys_suspend, exynos_drm_sys_resume)
>  };
>  
> -int exynos_drm_component_add(struct device *dev,
> -				const struct component_ops *ops)
> +int exynos_drm_component_add(struct device *dev)
>  {
>  	struct component_dev *cdev;
> -	int ret;
> +
> +	mutex_lock(&drm_component_lock);
> +
> +	list_for_each_entry(cdev, &drm_component_list, list) {
> +		if (cdev->dev == dev) {
> +			mutex_unlock(&drm_component_lock);
> +			return 0;
> +		}
> +	}
> +
> +	mutex_unlock(&drm_component_lock);
>  
>  	cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
>  	if (!cdev)
>  		return -ENOMEM;
>  
> -	ret = component_add(dev, ops);
> -	if (ret) {
> -		kfree(cdev);
> -		return ret;
> -	}
> -
>  	cdev->dev = dev;
>  
>  	mutex_lock(&drm_component_lock);
> @@ -406,8 +409,7 @@ int exynos_drm_component_add(struct device *dev,
>  	return 0;
>  }
>  
> -void exynos_drm_component_del(struct device *dev,
> -				const struct component_ops *ops)
> +void exynos_drm_component_del(struct device *dev)
>  {
>  	struct component_dev *cdev, *next;
>  
> @@ -422,8 +424,6 @@ void exynos_drm_component_del(struct device *dev,
>  	}
>  
>  	mutex_unlock(&drm_component_lock);
> -
> -	component_del(dev, ops);
>  }
>  
>  static int compare_of(struct device *dev, void *data)
> @@ -446,6 +446,8 @@ static int exynos_drm_add_components(struct device *dev, struct master *m)
>  		ret = component_master_add_child(m, compare_of, cdev->dev);
>  		if (!ret)
>  			attached_cnt++;
> +		else
> +			return ret;
>  
>  		mutex_lock(&drm_component_lock);
>  	}
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index e82e620..13480b2 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -354,12 +354,9 @@ void exynos_drm_remove_vidi(void);
>  int exynos_drm_create_enc_conn(struct drm_device *dev,
>  				struct exynos_drm_display *display);
>  
> -struct component_ops;
> -int exynos_drm_component_add(struct device *dev,
> -				const struct component_ops *ops);
> +int exynos_drm_component_add(struct device *dev);
>  
> -void exynos_drm_component_del(struct device *dev,
> -				const struct component_ops *ops);
> +void exynos_drm_component_del(struct device *dev);
>  
>  extern struct platform_driver fimd_driver;
>  extern struct platform_driver dp_driver;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 84661fe..1421d9b 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1423,10 +1423,15 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>  	struct exynos_dsi *dsi;
>  	int ret;
>  
> +	ret = exynos_drm_component_add(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
>  	dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
>  	if (!dsi) {
>  		dev_err(&pdev->dev, "failed to allocate dsi object.\n");
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_del_component;
>  	}
>  
>  	init_completion(&dsi->completed);
> @@ -1440,7 +1445,7 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>  
>  	ret = exynos_dsi_parse_dt(dsi);
>  	if (ret)
> -		return ret;
> +		goto err_del_component;
>  
>  	dsi->supplies[0].supply = "vddcore";
>  	dsi->supplies[1].supply = "vddio";
> @@ -1467,7 +1472,8 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>  	dsi->reg_base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(dsi->reg_base)) {
>  		dev_err(&pdev->dev, "failed to remap io region\n");
> -		return PTR_ERR(dsi->reg_base);
> +		ret = PTR_ERR(dsi->reg_base);
> +		goto err_del_component;
>  	}
>  
>  	dsi->phy = devm_phy_get(&pdev->dev, "dsim");
> @@ -1479,7 +1485,8 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>  	dsi->irq = platform_get_irq(pdev, 0);
>  	if (dsi->irq < 0) {
>  		dev_err(&pdev->dev, "failed to request dsi irq resource\n");
> -		return dsi->irq;
> +		ret = dsi->irq;
> +		goto err_del_component;
>  	}
>  
>  	irq_set_status_flags(dsi->irq, IRQ_NOAUTOEN);
> @@ -1488,19 +1495,29 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>  					dev_name(&pdev->dev), dsi);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to request dsi irq\n");
> -		return ret;
> +		goto err_del_component;
>  	}
>  
>  	exynos_dsi_display.ctx = dsi;
>  
>  	platform_set_drvdata(pdev, &exynos_dsi_display);
>  
> -	return exynos_drm_component_add(&pdev->dev, &exynos_dsi_component_ops);
> +	ret = component_add(&pdev->dev, &exynos_dsi_component_ops);
> +	if (ret)
> +		goto err_del_component;
> +
> +	return ret;
> +
> +err_del_component:
> +	exynos_drm_component_del(&pdev->dev);
> +	return ret;
>  }
>  
>  static int exynos_dsi_remove(struct platform_device *pdev)
>  {
> -	exynos_drm_component_del(&pdev->dev, &exynos_dsi_component_ops);
> +	component_del(&pdev->dev, &exynos_dsi_component_ops);
> +	exynos_drm_component_del(&pdev->dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index bd30d0c..5b85253 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -941,12 +941,21 @@ static int fimd_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	int ret = -EINVAL;
>  
> -	if (!dev->of_node)
> -		return -ENODEV;
> +
> +	ret = exynos_drm_component_add(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	if (!dev->of_node) {
> +		ret = -ENODEV;
> +		goto err_del_component;
> +	}
>  
>  	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> -	if (!ctx)
> -		return -ENOMEM;
> +	if (!ctx) {
> +		ret = -ENOMEM;
> +		goto err_del_component;
> +	}
>  
>  	ctx->dev = dev;
>  	ctx->suspended = true;
> @@ -959,32 +968,35 @@ static int fimd_probe(struct platform_device *pdev)
>  	ctx->bus_clk = devm_clk_get(dev, "fimd");
>  	if (IS_ERR(ctx->bus_clk)) {
>  		dev_err(dev, "failed to get bus clock\n");
> -		return PTR_ERR(ctx->bus_clk);
> +		return -EPROBE_DEFER;
I think it is not correct to mask any clock error by -EPROBE_DEFER.
It will be OK to return PTR_ERR(ctx->bus_clk).
>  	}
>  
>  	ctx->lcd_clk = devm_clk_get(dev, "sclk_fimd");
>  	if (IS_ERR(ctx->lcd_clk)) {
>  		dev_err(dev, "failed to get lcd clock\n");
> -		return PTR_ERR(ctx->lcd_clk);
> +		return -EPROBE_DEFER;
>  	}
ditto

Regards
Andrzej
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  
>  	ctx->regs = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(ctx->regs))
> -		return PTR_ERR(ctx->regs);
> +	if (IS_ERR(ctx->regs)) {
> +		ret = PTR_ERR(ctx->regs);
> +		goto err_del_component;
> +	}
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "vsync");
>  	if (!res) {
>  		dev_err(dev, "irq request failed.\n");
> -		return -ENXIO;
> +		ret = -ENXIO;
> +		goto err_del_component;
>  	}
>  
>  	ret = devm_request_irq(dev, res->start, fimd_irq_handler,
>  							0, "drm_fimd", ctx);
>  	if (ret) {
>  		dev_err(dev, "irq request failed.\n");
> -		return ret;
> +		goto err_del_component;
>  	}
>  
>  	ctx->driver_data = drm_fimd_get_driver_data(pdev);
> @@ -1001,14 +1013,27 @@ static int fimd_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(&pdev->dev);
>  
> -	return exynos_drm_component_add(&pdev->dev, &fimd_component_ops);
> +	ret = component_add(&pdev->dev, &fimd_component_ops);
> +	if (ret)
> +		goto err_disable_pm_runtime;
> +
> +	return ret;
> +
> +err_disable_pm_runtime:
> +	pm_runtime_disable(&pdev->dev);
> +
> +err_del_component:
> +	exynos_drm_component_del(&pdev->dev);
> +	return ret;
>  }
>  
>  static int fimd_remove(struct platform_device *pdev)
>  {
>  	pm_runtime_disable(&pdev->dev);
>  
> -	exynos_drm_component_del(&pdev->dev, &fimd_component_ops);
> +	component_del(&pdev->dev, &fimd_component_ops);
> +	exynos_drm_component_del(&pdev->dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index e05c86a..5919800 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -2275,24 +2275,36 @@ static int hdmi_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	int ret;
>  
> -	if (!dev->of_node)
> -		return -ENODEV;
> +	ret = exynos_drm_component_add(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	if (!dev->of_node) {
> +		ret = -ENODEV;
> +		goto err_del_component;
> +	}
>  
>  	pdata = drm_hdmi_dt_parse_pdata(dev);
> -	if (!pdata)
> -		return -EINVAL;
> +	if (!pdata) {
> +		ret = -EINVAL;
> +		goto err_del_component;
> +	}
>  
>  	hdata = devm_kzalloc(dev, sizeof(struct hdmi_context), GFP_KERNEL);
> -	if (!hdata)
> -		return -ENOMEM;
> +	if (!hdata) {
> +		ret = -ENOMEM;
> +		goto err_del_component;
> +	}
>  
>  	mutex_init(&hdata->hdmi_mutex);
>  
>  	platform_set_drvdata(pdev, &hdmi_display);
>  
>  	match = of_match_node(hdmi_match_types, dev->of_node);
> -	if (!match)
> -		return -ENODEV;
> +	if (!match) {
> +		ret = -ENODEV;
> +		goto err_del_component;
> +	}
>  
>  	drv_data = (struct hdmi_driver_data *)match->data;
>  	hdata->type = drv_data->type;
> @@ -2305,18 +2317,20 @@ static int hdmi_probe(struct platform_device *pdev)
>  	ret = hdmi_resources_init(hdata);
>  	if (ret) {
>  		DRM_ERROR("hdmi_resources_init failed\n");
> -		return -EINVAL;
> +		return -EPROBE_DEFER;
>  	}
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	hdata->regs = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(hdata->regs))
> -		return PTR_ERR(hdata->regs);
> +	if (IS_ERR(hdata->regs)) {
> +		ret = PTR_ERR(hdata->regs);
> +		goto err_del_component;
> +	}
>  
>  	ret = devm_gpio_request(dev, hdata->hpd_gpio, "HPD");
>  	if (ret) {
>  		DRM_ERROR("failed to request HPD gpio\n");
> -		return ret;
> +		goto err_del_component;
>  	}
>  
>  	ddc_node = hdmi_legacy_ddc_dt_binding(dev);
> @@ -2327,14 +2341,15 @@ static int hdmi_probe(struct platform_device *pdev)
>  	ddc_node = of_parse_phandle(dev->of_node, "ddc", 0);
>  	if (!ddc_node) {
>  		DRM_ERROR("Failed to find ddc node in device tree\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto err_del_component;
>  	}
>  
>  out_get_ddc_adpt:
>  	hdata->ddc_adpt = of_find_i2c_adapter_by_node(ddc_node);
>  	if (!hdata->ddc_adpt) {
>  		DRM_ERROR("Failed to get ddc i2c adapter by node\n");
> -		return -ENODEV;
> +		return -EPROBE_DEFER;
>  	}
>  
>  	phy_node = hdmi_legacy_phy_dt_binding(dev);
> @@ -2361,7 +2376,7 @@ out_get_phy_port:
>  		hdata->hdmiphy_port = of_find_i2c_device_by_node(phy_node);
>  		if (!hdata->hdmiphy_port) {
>  			DRM_ERROR("Failed to get hdmi phy i2c client\n");
> -			ret = -ENODEV;
> +			ret = -EPROBE_DEFER;
>  			goto err_ddc;
>  		}
>  	}
> @@ -2390,19 +2405,31 @@ out_get_phy_port:
>  			"samsung,syscon-phandle");
>  	if (IS_ERR(hdata->pmureg)) {
>  		DRM_ERROR("syscon regmap lookup failed.\n");
> +		ret = -EPROBE_DEFER;
>  		goto err_hdmiphy;
>  	}
>  
>  	pm_runtime_enable(dev);
>  	hdmi_display.ctx = hdata;
>  
> -	return exynos_drm_component_add(&pdev->dev, &hdmi_component_ops);
> +	ret = component_add(&pdev->dev, &hdmi_component_ops);
> +	if (ret)
> +		goto err_disable_pm_runtime;
> +
> +	return ret;
> +
> +err_disable_pm_runtime:
> +	pm_runtime_disable(dev);
>  
>  err_hdmiphy:
>  	if (hdata->hdmiphy_port)
>  		put_device(&hdata->hdmiphy_port->dev);
>  err_ddc:
>  	put_device(&hdata->ddc_adpt->dev);
> +
> +err_del_component:
> +	exynos_drm_component_del(&pdev->dev);
> +
>  	return ret;
>  }
>  
> @@ -2416,8 +2443,9 @@ static int hdmi_remove(struct platform_device *pdev)
>  	put_device(&hdata->ddc_adpt->dev);
>  
>  	pm_runtime_disable(&pdev->dev);
> +	component_del(&pdev->dev, &hdmi_component_ops);
>  
> -	exynos_drm_component_del(&pdev->dev, &hdmi_component_ops);
> +	exynos_drm_component_del(&pdev->dev);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 483d7c0..eaae17b 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -1273,12 +1273,24 @@ static const struct component_ops mixer_component_ops = {
>  
>  static int mixer_probe(struct platform_device *pdev)
>  {
> -	return exynos_drm_component_add(&pdev->dev, &mixer_component_ops);
> +	int ret;
> +
> +	ret = exynos_drm_component_add(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = component_add(&pdev->dev, &mixer_component_ops);
> +	if (ret)
> +		exynos_drm_component_del(&pdev->dev);
> +
> +	return ret;
>  }
>  
>  static int mixer_remove(struct platform_device *pdev)
>  {
> -	exynos_drm_component_del(&pdev->dev, &mixer_component_ops);
> +	component_del(&pdev->dev, &mixer_component_ops);
> +	exynos_drm_component_del(&pdev->dev);
> +
>  	return 0;
>  }
>  

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux