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

[Daniel Vetter]

On Mon, Oct 24, 2016 at 03:27:59PM +0100, Brian Starkey wrote:
> Connectors shouldn't be registered until the rest of the whole device
> is set up, so that consistent state is presented to userspace.
> 
> As such, remove the calls to drm_connector_register() and
> drm_connector_unregister() from tda998x, as these are now handled by
> drm_dev_(un)register() itself.
> 
> To work with this change, the mali-dp and hdlcd bind and unbind
> sequences have to be reordered, to ensure that the componentised
> encoder/connector is bound before drm_dev_register() registers all
> connectors. Similarly, the device must be unregistered before the
> component is unbound.
> 
> Altogether, this allows other drivers using tda998x to be
> de-midlayered, and to have less racy initialisation of their components.
> 
> Splitting this commit into three (one per driver) isn't possible without
> intermediate breakage, so it is all squashed together here.
> 
> Suggested-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
> Signed-off-by: Brian Starkey <brian.starkey@xxxxxxx>
> Reviewed-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>

> 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.
-Daniel

>  	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
>  
>  	return 0;
>  
> -err_sysfs:
> -	drm_connector_cleanup(&priv->connector);
>  err_connector:
>  	drm_encoder_cleanup(&priv->encoder);
>  err_encoder:
> @@ -1463,7 +1456,6 @@ static void tda998x_unbind(struct device *dev, struct device *master,
>  {
>  	struct tda998x_priv *priv = dev_get_drvdata(dev);
>  
> -	drm_connector_unregister(&priv->connector);
>  	drm_connector_cleanup(&priv->connector);
>  	drm_encoder_cleanup(&priv->encoder);
>  	tda998x_destroy(priv);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch