[PATCH 12/23] drm/exynos: Split manager/display/subdrv

[Inki Dae]

> -----Original Message-----
> From: Sean Paul [mailto:seanpaul at chromium.org]
> Sent: Saturday, October 12, 2013 11:07 AM
> To: Inki Dae
> Cc: DRI mailing list; Stéphane Marchesin; Dave Airlie; Tomasz Figa
> Subject: Re: [PATCH 12/23] drm/exynos: Split manager/display/subdrv
> 
> On Fri, Oct 11, 2013 at 8:42 PM, Inki Dae <inki.dae at samsung.com> wrote:
> >>  static int exynos_drm_create_enc_conn(struct drm_device *dev,
> >> -                                       struct exynos_drm_subdrv
*subdrv)
> >> +                                       struct exynos_drm_display
*display)
> >>  {
> >>         struct drm_encoder *encoder;
> >>         struct drm_connector *connector;
> >> +       struct exynos_drm_manager *manager;
> >>         int ret;
> >> +       unsigned long possible_crtcs = 0;
> >>
> >> -       subdrv->manager->dev = subdrv->dev;
> >> +       /* Find possible crtcs for this display */
> >> +       list_for_each_entry(manager, &exynos_drm_manager_list, list)
> >> +               if (manager->type == display->type)
> >> +                       possible_crtcs |= 1 << manager->pipe;
> >>
> >>         /* create and initialize a encoder for this sub driver. */
> >> -       encoder = exynos_drm_encoder_create(dev, subdrv->manager,
> >> -                       (1 << MAX_CRTC) - 1);
> >> +       encoder = exynos_drm_encoder_create(dev, display,
> possible_crtcs);
> >>         if (!encoder) {
> >>                 DRM_ERROR("failed to create encoder\n");
> >>                 return -EFAULT;
> >>         }
> >> -       subdrv->encoder = encoder;
> >> +       display->encoder = encoder;
> >>
> >> -       if (subdrv->manager->display_ops->type ==
> EXYNOS_DISPLAY_TYPE_LCD) {
> >> +       if (display->type == EXYNOS_DISPLAY_TYPE_LCD) {
> >>                 ret = exynos_drm_attach_lcd_bridge(dev, encoder)
> >
> > Plz, _do_not_base_ on top of your lvds bridge patch set. As I
> > mentioned before, the tricky codes are not good. For this, let's find
> > a better way after your refactoring patch set are reviewed enough, and
> > if merged.
> >
> 
> [+adding back the people you removed from CC]
> 

Hm.. I  didn't really remove the people from CC. See the your original
email. :)


> I'm happy to change how that works, but I've yet to find a better
> solution. Instead of just dismissing something as "tricky codes", can
> you suggest a concrete solution that is better than what I've got?
> 

I think I mentioned already at previous email. First of all, let's have a
thorough review to your refactoring patch set, and then let's find a better
way.

I believe that we can find a better way with your refactoring patch set: now
we can implement encoder and connector directly.


> I'm also a bit confused as to why you think it's so tricky, it's a

First, That was your comment, "Right, so this is kind of tricky". And also
it seems like tricky codes to me.

> synchronous initialization call to a driver; that's about as simple as
> it gets.

As you mentioned before, ptn3460 driver is not depended on SoC, and can be
used for other SoC. Let you try to think about lcd class and backlight
class. I think we need a interfacing layer between common driver and
specific something for connecting SoC specific DRM framework.

> 
> Sean
> 
> 
> 
> >>                 if (ret)
> >>                         return 0;
> >> @@ -91,7 +98,7 @@ static int exynos_drm_create_enc_conn(struct
> drm_device *dev,
> >>                 goto err_destroy_encoder;
> >>         }
> >>
> >> -       subdrv->connector = connector;
> >> +       display->connector = connector;
> >>
> >>         return 0;
> >>
> >> @@ -100,21 +107,6 @@ err_destroy_encoder:
> >>         return ret;
> >>  }
> >>