From: Andrzej Hajda <a.hajda@samsung.com>
To: Inki Dae <inki.dae@samsung.com>,
airlied@linux.ie, dri-devel@lists.freedesktop.org
Cc: linux-samsung-soc@vger.kernel.org, rmk+kernel@arm.linux.org.uk
Subject: Re: [PATCH 1/2] drm/exynos: consider deferred probe case
Date: Wed, 28 May 2014 13:22:21 +0200 [thread overview]
Message-ID: <5385C6ED.5080107@samsung.com> (raw)
In-Reply-To: <1401194563-803-2-git-send-email-inki.dae@samsung.com>
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@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 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;
> }
>
next prev parent reply other threads:[~2014-05-28 11:22 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-27 12:42 [PATCH 0/2] drm/exynos: consider deferred probe case Inki Dae
2014-05-27 12:42 ` [PATCH 1/2] " Inki Dae
2014-05-28 11:22 ` Andrzej Hajda [this message]
2014-05-27 12:42 ` [PATCH 2/2] drm/exynos: mipi-dsi: consider panel driver-deferred probe Inki Dae
2014-05-28 7:58 ` Andrzej Hajda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5385C6ED.5080107@samsung.com \
--to=a.hajda@samsung.com \
--cc=airlied@linux.ie \
--cc=dri-devel@lists.freedesktop.org \
--cc=inki.dae@samsung.com \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=rmk+kernel@arm.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.