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

[Russell King - ARM Linux]

On Tue, Oct 25, 2016 at 12:19:19PM +0200, Daniel Vetter wrote:
> On Tue, Oct 25, 2016 at 10:52:33AM +0100, Brian Starkey wrote:
> > 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.
> 
> Ah, makes sense. Should I apply this to drm-misc so it's in a shared tree?

I need the patch against v4.8.  I'm happy to pick it up and add it
to my drm-tda998x-devel branch, which you can then merge into
drm-misc if you wish.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.