Re: [PATCH 02/13] drm/exynos: separated subdrv->probe call and encoder/connector creation.

From: Joonyoung Shim <jy0922.shim@samsung.com>
To: Inki Dae <inki.dae@samsung.com>
Cc: kyungmin.park@samsung.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 02/13] drm/exynos: separated subdrv->probe call and encoder/connector creation.
Date: Mon, 20 Aug 2012 10:11:01 +0900	[thread overview]
Message-ID: <50318EA5.8020505@samsung.com> (raw)
In-Reply-To: <1345197059-25583-3-git-send-email-inki.dae@samsung.com>

On 08/17/2012 06:50 PM, Inki Dae wrote:
> this patch separates sub driver's probe call and encoder/connector creation
> so that exynos drm core module can take exception when some operation was
> failed properly.

Which exceptions? I don't know this patch gives any benefit to us.

>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>   drivers/gpu/drm/exynos/exynos_drm_core.c |   93 +++++++++++++++++++++---------
>   1 files changed, 65 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
> index 84dd099..1c8d5fe 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
> @@ -34,30 +34,15 @@
>   
>   static LIST_HEAD(exynos_drm_subdrv_list);
>   
> -static int exynos_drm_subdrv_probe(struct drm_device *dev,
> +static int exynos_drm_create_enc_conn(struct drm_device *dev,
>   					struct exynos_drm_subdrv *subdrv)
>   {
>   	struct drm_encoder *encoder;
>   	struct drm_connector *connector;
> +	int ret;
>   
>   	DRM_DEBUG_DRIVER("%s\n", __FILE__);
>   
> -	if (subdrv->probe) {
> -		int ret;
> -
> -		/*
> -		 * this probe callback would be called by sub driver
> -		 * after setting of all resources to this sub driver,
> -		 * such as clock, irq and register map are done or by load()
> -		 * of exynos drm driver.
> -		 *
> -		 * P.S. note that this driver is considered for modularization.
> -		 */
> -		ret = subdrv->probe(dev, subdrv->dev);
> -		if (ret)
> -			return ret;
> -	}
> -
>   	if (!subdrv->manager)
>   		return 0;
>   
> @@ -78,24 +63,22 @@ static int exynos_drm_subdrv_probe(struct drm_device *dev,
>   	connector = exynos_drm_connector_create(dev, encoder);
>   	if (!connector) {
>   		DRM_ERROR("failed to create connector\n");
> -		encoder->funcs->destroy(encoder);
> -		return -EFAULT;
> +		ret = -EFAULT;
> +		goto err_destroy_encoder;
>   	}
>   
>   	subdrv->encoder = encoder;
>   	subdrv->connector = connector;
>   
>   	return 0;
> +
> +err_destroy_encoder:
> +	encoder->funcs->destroy(encoder);
> +	return ret;
>   }
>   
> -static void exynos_drm_subdrv_remove(struct drm_device *dev,
> -				      struct exynos_drm_subdrv *subdrv)
> +static void exynos_drm_destroy_enc_conn(struct exynos_drm_subdrv *subdrv)
>   {
> -	DRM_DEBUG_DRIVER("%s\n", __FILE__);
> -
> -	if (subdrv->remove)
> -		subdrv->remove(dev);
> -
>   	if (subdrv->encoder) {
>   		struct drm_encoder *encoder = subdrv->encoder;
>   		encoder->funcs->destroy(encoder);
> @@ -109,9 +92,48 @@ static void exynos_drm_subdrv_remove(struct drm_device *dev,
>   	}
>   }
>   
> +static int exynos_drm_subdrv_probe(struct drm_device *dev,
> +					struct exynos_drm_subdrv *subdrv)
> +{
> +	if (subdrv->probe) {
> +		int ret;
> +
> +		subdrv->drm_dev = dev;
> +
> +		/*
> +		 * this probe callback would be called by sub driver
> +		 * after setting of all resources to this sub driver,
> +		 * such as clock, irq and register map are done or by load()
> +		 * of exynos drm driver.
> +		 *
> +		 * P.S. note that this driver is considered for modularization.
> +		 */
> +		ret = subdrv->probe(dev, subdrv->dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!subdrv->manager)
> +		return -EINVAL;
> +
> +	subdrv->manager->dev = subdrv->dev;
> +
> +	return 0;
> +}
> +
> +static void exynos_drm_subdrv_remove(struct drm_device *dev,
> +				      struct exynos_drm_subdrv *subdrv)
> +{
> +	DRM_DEBUG_DRIVER("%s\n", __FILE__);
> +
> +	if (subdrv->remove)
> +		subdrv->remove(dev, subdrv->dev);
> +}
> +
>   int exynos_drm_device_register(struct drm_device *dev)
>   {
>   	struct exynos_drm_subdrv *subdrv, *n;
> +	unsigned int fine_cnt = 0;
>   	int err;
>   
>   	DRM_DEBUG_DRIVER("%s\n", __FILE__);
> @@ -120,14 +142,27 @@ int exynos_drm_device_register(struct drm_device *dev)
>   		return -EINVAL;
>   
>   	list_for_each_entry_safe(subdrv, n, &exynos_drm_subdrv_list, list) {
> -		subdrv->drm_dev = dev;
>   		err = exynos_drm_subdrv_probe(dev, subdrv);
>   		if (err) {
>   			DRM_DEBUG("exynos drm subdrv probe failed.\n");
>   			list_del(&subdrv->list);
> +			continue;
>   		}
> +
> +		err = exynos_drm_create_enc_conn(dev, subdrv);
> +		if (err) {
> +			DRM_DEBUG("failed to create encoder and connector.\n");
> +			exynos_drm_subdrv_remove(dev, subdrv);
> +			list_del(&subdrv->list);
> +			continue;
> +		}
> +
> +		fine_cnt++;
>   	}
>   
> +	if (!fine_cnt)
> +		return -EINVAL;
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(exynos_drm_device_register);
> @@ -143,8 +178,10 @@ int exynos_drm_device_unregister(struct drm_device *dev)
>   		return -EINVAL;
>   	}
>   
> -	list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list)
> +	list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) {
>   		exynos_drm_subdrv_remove(dev, subdrv);
> +		exynos_drm_destroy_enc_conn(subdrv);
> +	}
>   
>   	return 0;
>   }