Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1449428 > unrolled thread

[PATCH] drm/i2c: tda998x: don't register the connector

Back to article view | Back to linux.kernel

Contents

  [PATCH] drm/i2c: tda998x: don't register the connector Brian Starkey <brian.starkey@arm.com> - 2016-07-25 13:00 +0200
    Re: [PATCH] drm/i2c: tda998x: don't register the connector Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-07-25 14:30 +0200
      Re: [PATCH] drm/i2c: tda998x: don't register the connector Brian Starkey <brian.starkey@arm.com> - 2016-07-25 15:00 +0200
        Re: [PATCH] drm/i2c: tda998x: don't register the connector Daniel Vetter <daniel@ffwll.ch> - 2016-07-25 17:10 +0200

#1449428 — [PATCH] drm/i2c: tda998x: don't register the connector

The connector shouldn't be registered until the rest of the whole device
is set up, so that consistent state is presented to userspace.

As drm_dev_register() now registers all of the connectors anyway,
there's no need to explicitly do it in individual drivers so remove
the calls to drm_connector_register()/drm_connector_unregister().

This allows componentised drivers to use tda998x without having racy
initialisation.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c |    8 --------
 1 file changed, 8 deletions(-)

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;
-
 	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

[toc] | [next] | [standalone]

#1449445

On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
> The connector shouldn't be registered until the rest of the whole device
> is set up, so that consistent state is presented to userspace.
> 
> As drm_dev_register() now registers all of the connectors anyway,
> there's no need to explicitly do it in individual drivers so remove
> the calls to drm_connector_register()/drm_connector_unregister().
> 
> This allows componentised drivers to use tda998x without having racy
> initialisation.

Is there a corresponding patch for armada-drm so that the cubox doesn't
regress?  Has it already been merged?

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

[toc] | [prev] | [next] | [standalone]

#1449459

Hi Russell,

On Mon, Jul 25, 2016 at 01:25:04PM +0100, Russell King - ARM Linux wrote:
>On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
>> The connector shouldn't be registered until the rest of the whole device
>> is set up, so that consistent state is presented to userspace.
>>
>> As drm_dev_register() now registers all of the connectors anyway,
>> there's no need to explicitly do it in individual drivers so remove
>> the calls to drm_connector_register()/drm_connector_unregister().
>>
>> This allows componentised drivers to use tda998x without having racy
>> initialisation.
>
>Is there a corresponding patch for armada-drm so that the cubox doesn't
>regress?  Has it already been merged?
>

A patch for armada-drm to do what?

I should perhaps have explicitly mentioned that this change depends
on e28cd4d0a223: "drm: Automatically register/unregister all
connectors", which is in drm-next.

Like my commit message says - after the above commit, all connectors
are automatically registered in drm_dev_register() - so I don't
anticipate any regression, but I don't have a cubox to test.

armada-drm seems to be doing effectively the same thing as arm/hdlcd,
which works fine after this patch with no other changes.

Let me know if I've missed something; or if you are able to test on
cubox that would be great.

Thanks,
Brian

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

[toc] | [prev] | [next] | [standalone]

#1449559

On Mon, Jul 25, 2016 at 01:54:06PM +0100, Brian Starkey wrote:
> Hi Russell,
> 
> On Mon, Jul 25, 2016 at 01:25:04PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
> > > The connector shouldn't be registered until the rest of the whole device
> > > is set up, so that consistent state is presented to userspace.
> > > 
> > > As drm_dev_register() now registers all of the connectors anyway,
> > > there's no need to explicitly do it in individual drivers so remove
> > > the calls to drm_connector_register()/drm_connector_unregister().
> > > 
> > > This allows componentised drivers to use tda998x without having racy
> > > initialisation.
> > 
> > Is there a corresponding patch for armada-drm so that the cubox doesn't
> > regress?  Has it already been merged?
> > 
> 
> A patch for armada-drm to do what?
> 
> I should perhaps have explicitly mentioned that this change depends
> on e28cd4d0a223: "drm: Automatically register/unregister all
> connectors", which is in drm-next.
> 
> Like my commit message says - after the above commit, all connectors
> are automatically registered in drm_dev_register() - so I don't
> anticipate any regression, but I don't have a cubox to test.
> 
> armada-drm seems to be doing effectively the same thing as arm/hdlcd,
> which works fine after this patch with no other changes.
> 
> Let me know if I've missed something; or if you are able to test on
> cubox that would be great.

Ack from my side on generally nuking drm_connector_register() from
everywhere except truely hotplugged connectors like dp mst. It should keep
working for everyone. Only exception is if there's a driver which calls
drm_dev_register too early (before all connectors are probed), which would
be a bug anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web