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

From: Brian Starkey
Date: Tue Oct 25 2016 - 05:52:43 EST


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