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

[Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>]

On 08/20/2012 01:31 PM, InKi Dae wrote:
> 2012/8/20 Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>:
> > On 08/20/2012 10:52 AM, InKi Dae wrote:
> > > 2012/8/20 Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>:
> > > > 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@xxxxxxxxxxx>
> > > > > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > > > > ---
> > > > >     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@xxxxxxxxxxxxxxxxxxxxx
> > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel