Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Brian Starkey]

Hi Daniel,

On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@xxxxxxx> wrote:
> > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > index f4315bc..6e6fca2 100644
> > > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > @@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
> > > > tda998x_connector_helper_funcs = {
> > > >
> > > >  static void tda998x_connector_destroy(struct drm_connector *connector)
> > > >  {
> > > > -       drm_connector_unregister(connector);
> > > >         drm_connector_cleanup(connector);
> > > >  }
> > > >
> > > > @@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
> > > > struct device *master, void *data)
> > > >         if (ret)
> > > >                 goto err_connector;
> > > >
> > > > -       ret = drm_connector_register(&priv->connector);
> > > > -       if (ret)
> > > > -               goto err_sysfs;
> > > > -
> > >
> > >
> > > Instead of smashing all these patches into one, what about checking here
> > > for midlayer driver set with:
> > >
> > >         /* register here for drivers still using midlayer load/unload */
> > >         if (dev->driver->load)
> > >                 drm_connector_register(connector),
> > >
> > > Similar in other places. That way we wouldn't need to switch the world in
> > > one patch.
> >
> >
> > I don't think that helps. If we do that in isolation (first), then
> > mali-dp and hdlcd won't get their connectors registered because their
> > bind order is:
> >
> >         drm_dev_register();
> >         component_bind_all();
> >
> > If we change the mali-dp/hdlcd bind order first, then tda998x will
> > explode on drm_connector_register() until it's patched to remove that.
> >
> > As I mentioned in my mail to Russell, the only way I can see to avoid
> > patching all three drivers in one go is:
> >  1) Add (probably open-coded) drm_connector_register_all() to the end
> >     of bind in hdlcd and mali-dp
> >  2) Patch tda998x to remove drm_connector_register()
> >  3) Reorder hdlcd/mali-dp bind and remove the connector registration
> >     added in 1)
> >
> > We can do that, but it's extra churn for the same result, and none of
> > the 5 patches will really make sense in isolation anyway.
>
> I thought there's also armada to take care of, which this patch would
> break? Maybe even another driver, so the hack would still be useful
> for those other drivers.

OK it seems like this situation has got very confused. In short, this
patch does not break armada. Russell previously tested the tda998x
patch against armada and reported no issues.
Drivers with a ->load() callback don't need to register the connector
anymore, because drm_dev_register() does it for them.

As far as I know, this patch touching these three drivers is
sufficient to allow all current users of tda998x to continue using it,
whilst also allowing armada and tilcdc to be de-midlayered.

> And it would have been useful if malidp/hdlcd
> wouldn't have started out with the wrong init ordering ;-)

Yeah, believe me I know -_-
Hindsight is always 20/20 something something

Thanks,
-Brian

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch