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

[Joonyoung Shim <jy0922.shim@samsung.com>]

On 08/20/2012 01:31 PM, InKi Dae wrote:
> 2012/8/20 Joonyoung Shim <jy0922.shim@samsung.com>:
>> On 08/20/2012 10:52 AM, InKi Dae wrote:
>>> 2012/8/20 Joonyoung Shim <jy0922.shim@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.
>>>>
>>> previous code didn't take exception when exynos_drm_encoder_create()
>>> is falied.
>>
>> No, it is considered.
>>
> where is subdrv->remove() called when exynos_drm_encoder_create() is
> failed? I think if failed then subdrv->remove() should be called and
> if exynos_drm_connector_create() is failed then not only
> encoder->funcs->destory() but also subdrv->remove()

OK, but just add subdrv->remove(). Then if it needs, please split function.

>>> and for now, exynos_drm_encoder/connector_create functions
>>> was called at exynos_drm_subdrv_probe() so know that this patch is for
>>> code clean by separating them into two parts also.
>>
>> It's ok, but it just splitting.
>>
>>
>>>>> 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;
>>>>>     }
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel