From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrzej Hajda <a.hajda@samsung.com>
Subject: Re: [PATCH v2 1/7] drm/exynos: add super device support
Date: Wed, 02 Apr 2014 16:06:40 +0200
Message-ID: <533C1970.70003@samsung.com>
References: <1396355882-17010-1-git-send-email-inki.dae@samsung.com>
 <1396355882-17010-2-git-send-email-inki.dae@samsung.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <dri-devel-bounces@lists.freedesktop.org>
Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com
 [210.118.77.12])
 by gabe.freedesktop.org (Postfix) with ESMTP id 9ECED6E64D
 for <dri-devel@lists.freedesktop.org>; Wed,  2 Apr 2014 07:06:45 -0700 (PDT)
Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245])
 by mailout2.w1.samsung.com
 (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov
 17 2011)) with ESMTP id <0N3E00GGIPUYLY50@mailout2.w1.samsung.com> for
 dri-devel@lists.freedesktop.org; Wed, 02 Apr 2014 15:06:34 +0100 (BST)
In-reply-to: <1396355882-17010-2-git-send-email-inki.dae@samsung.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/dri-devel>,
 <mailto:dri-devel-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/dri-devel>
List-Post: <mailto:dri-devel@lists.freedesktop.org>
List-Help: <mailto:dri-devel-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/dri-devel>,
 <mailto:dri-devel-request@lists.freedesktop.org?subject=subscribe>
Errors-To: dri-devel-bounces@lists.freedesktop.org
Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>
To: Inki Dae <inki.dae@samsung.com>, airlied@linux.ie, dri-devel@lists.freedesktop.org
Cc: kyungmin.park@samsung.com
List-Id: dri-devel@lists.freedesktop.org

On 04/01/2014 02:37 PM, Inki Dae wrote:
> This patch adds super device support to bind sub drivers
> using device tree.
> 
> For this, you should add a super device node to each machine dt files
> like belows,
> 
> In case of using MIPI-DSI,
> 	display-subsystem {
> 		compatible = "samsung,exynos-display-subsystem";
> 		ports = <&fimd>, <&dsi>;
> 	};
> 
> In case of using DisplayPort,
> 	display-subsystem {
> 		compatible = "samsung,exynos-display-subsystem";
> 		ports = <&fimd>, <&dp>;
> 	};
> 
> In case of using Parallel panel,
> 	display-subsystem {
> 		compatible = "samsung,exynos-display-subsystem";
> 		ports = <&fimd>;
> 	};
> 
> And if you don't add connector device node to ports property,
> default parallel panel driver, exynos_drm_dpi module, will be used.
> 
> ports property can have the following device nodes,
> 	fimd, mixer, Image Enhancer, MIPI-DSI, eDP, LVDS Bridge, or HDMI
> 
> With this patch, we can resolve the probing order issue without
> some global lists. So this patch also removes the unnecessary lists and
> stuff related to these lists.
> 

(...)

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 40fd6cc..7ebfe15 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -19,10 +19,12 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/component.h>
>  
>  #include <video/of_display_timing.h>
>  #include <video/of_videomode.h>
>  #include <video/samsung_fimd.h>
> +#include <drm/drm_panel.h>
>  #include <drm/exynos_drm.h>
>  
>  #include "exynos_drm_drv.h"
> @@ -144,12 +146,14 @@ static inline struct fimd_driver_data *drm_fimd_get_driver_data(
>  }
>  
>  static int fimd_mgr_initialize(struct exynos_drm_manager *mgr,
> -			struct drm_device *drm_dev, int pipe)
> +			struct drm_device *drm_dev)
>  {
>  	struct fimd_context *ctx = mgr->ctx;
> +	struct exynos_drm_private *priv;
> +	priv = drm_dev->dev_private;
>  
> -	ctx->drm_dev = drm_dev;
> -	ctx->pipe = pipe;
> +	mgr->drm_dev = ctx->drm_dev = drm_dev;
> +	mgr->pipe = ctx->pipe = priv->pipe++;
>  
>  	/*
>  	 * enable drm irq mode.
> @@ -803,8 +807,6 @@ static void fimd_dpms(struct exynos_drm_manager *mgr, int mode)
>  }
>  
>  static struct exynos_drm_manager_ops fimd_manager_ops = {
> -	.initialize = fimd_mgr_initialize,
> -	.remove = fimd_mgr_remove,
>  	.dpms = fimd_dpms,
>  	.mode_fixup = fimd_mode_fixup,
>  	.mode_set = fimd_mode_set,
> @@ -849,9 +851,10 @@ out:
>  	return IRQ_HANDLED;
>  }
>  
> -static int fimd_probe(struct platform_device *pdev)
> +static int fimd_bind(struct device *dev, struct device *master, void *data)
>  {
> -	struct device *dev = &pdev->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm_dev = data;
>  	struct fimd_context *ctx;
>  	struct resource *res;
>  	int win;
> @@ -910,11 +913,16 @@ static int fimd_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, &fimd_manager);
>  
>  	fimd_manager.ctx = ctx;
> -	exynos_drm_manager_register(&fimd_manager);
> +	fimd_mgr_initialize(&fimd_manager, drm_dev);
>  
> -	exynos_dpi_probe(ctx->dev);
> +	exynos_drm_crtc_create(&fimd_manager);
>  
> -	pm_runtime_enable(dev);
> +	/*
> +	 * It should be called after exynos_drm_crtc_create call because
> +	 * exynos_dpi_probe call will try to find same lcd type
> +	 * of manager to setup possible_crtcs.
> +	 */
> +	exynos_dpi_probe(drm_dev, dev);
>  
>  	for (win = 0; win < WINDOWS_NR; win++)
>  		fimd_clear_win(ctx, win);
> @@ -922,18 +930,56 @@ static int fimd_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int fimd_remove(struct platform_device *pdev)
> +static void fimd_unbind(struct device *dev, struct device *master,
> +			void *data)
>  {
> -	struct exynos_drm_manager *mgr = platform_get_drvdata(pdev);
> +	struct exynos_drm_manager *mgr = dev_get_drvdata(dev);
> +	struct drm_crtc *crtc = mgr->crtc;
> +
> +	fimd_dpms(mgr, DRM_MODE_DPMS_OFF);
>  
> -	exynos_dpi_remove(&pdev->dev);
> +	exynos_dpi_remove(mgr->drm_dev, dev);
>  
> -	exynos_drm_manager_unregister(&fimd_manager);
> +	fimd_mgr_remove(mgr);
>  
> -	fimd_dpms(mgr, DRM_MODE_DPMS_OFF);
> +	crtc->funcs->destroy(crtc);
> +}
> +
> +static const struct component_ops fimd_component_ops = {
> +	.bind	= fimd_bind,
> +	.unbind = fimd_unbind,
> +};
>  
> +static int fimd_probe(struct platform_device *pdev)
> +{
> +	struct device_node *dn;
> +
> +	/* Check if fimd node has port node. */
> +	dn = exynos_dpi_of_find_panel_node(&pdev->dev);
> +	if (dn) {
> +		struct drm_panel *panel;
> +
> +		/*
> +		 * Do not bind if there is the port node but a drm_panel
> +		 * isn't added to panel_list yet.
> +		 * In this case, fimd_probe will be called by defered probe
> +		 * again after the drm_panel is added to panel_list.
> +		 */
> +		panel = of_drm_find_panel(dn);
> +		if (!panel)
> +			return -EPROBE_DEFER;
> +	}

Wouldn't be better to leave it in exynos_dpi_probe? It should be called
in fimd_probe. It can return ERR_PTR(-EPROBE_DEFER) if the panel is
missing, NULL if there are no parallel bindings otherwise it will return
&exynos_dpi_display. fimd_bind will run exynos_drm_create_enc_conn on
returned pointer. In this case
in fimd_unbind connector and encoder should be removed and in
fimd_remove, exynos_dpi_remove should be conditionally called.

Regards
Andrzej

> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return component_add(&pdev->dev, &fimd_component_ops);
> +}
> +
> +static int fimd_remove(struct platform_device *pdev)
> +{
>  	pm_runtime_disable(&pdev->dev);
>  
> +	component_del(&pdev->dev, &fimd_component_ops);
>  	return 0;
>  }
>