Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965569AbcJYKTZ (ORCPT ); Tue, 25 Oct 2016 06:19:25 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33514 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933028AbcJYKTX (ORCPT ); Tue, 25 Oct 2016 06:19:23 -0400 Date: Tue, 25 Oct 2016 12:19:19 +0200 From: Daniel Vetter To: Brian Starkey Cc: Daniel Vetter , Russell King , dri-devel , Linux Kernel Mailing List Subject: Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration Message-ID: <20161025101919.35c53zoyf435urp3@phenom.ffwll.local> Mail-Followup-To: Brian Starkey , Russell King , dri-devel , Linux Kernel Mailing List References: <20161024142312.GA1988@e106950-lin.cambridge.arm.com> <1477319279-21726-1-git-send-email-brian.starkey@arm.com> <20161024143627.GT20761@phenom.ffwll.local> <20161024145202.GB1988@e106950-lin.cambridge.arm.com> <20161025095233.GD1988@e106950-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161025095233.GD1988@e106950-lin.cambridge.arm.com> X-Operating-System: Linux phenom 4.6.0-1-amd64 User-Agent: NeoMutt/20161014 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3447 Lines: 83 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 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? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch