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

[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.

> 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