Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

From: Daniel Vetter <daniel@ffwll.ch>
To: Lukas Wunner <lukas@wunner.de>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
Date: Tue, 29 Sep 2015 17:04:03 +0200	[thread overview]
Message-ID: <20150929150403.GN3383@phenom.ffwll.local> (raw)
In-Reply-To: <20150929124920.GA13481@wunner.de>

On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote:
> Hi Rafael,
> 
> On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote:
> > This is useful to determine which connector owns this AUX channel.
> 
> WTF? I posted a patch in August which does exactly that:
> http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html
> 
> Can also be pulled in from this git repo:
> https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6
> 
> My patch has the advantage that it updates all the drivers which use
> drm_dp_aux to fill that attribute. Yours only updates i915.
> 
> Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux,
> quote:
> 
> "That will also clear up the confusion with drm_dp_aux, adding a
> drm_connector there feels wrong since not every dp_aux line has a
> connector (e.g. for dp mst). If we can lift this relation out into drivers
> (where this is known) that seems cleaner."
> 
> So now Intel itself does precisely what Daniel criticized? Confusing!
> 
> Source:
> http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html

Critism is still valid, and thinking about this again a cleaner solution
would be to just have a correct parent/child relationship in the device
hirarchy. I.e. add a struct device *parent to the aux channel structure
which should point to the right connector.

Thanks for pointing out that I missed properly delayering this.
-Daniel

> 
> 
> Best regards,
> 
> Lukas
> 
> 
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 1 +
> >  include/drm/drm_dp_helper.h     | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 77f7330..f90439d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
> >  
> >  	intel_dp->aux.name = name;
> >  	intel_dp->aux.dev = dev->dev;
> > +	intel_dp->aux.connector = connector->base.kdev;
> >  	intel_dp->aux.transfer = intel_dp_aux_transfer;
> >  
> >  	DRM_DEBUG_KMS("registering %s bus for %s\n", name,
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 9ec4716..e009b5d 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -702,6 +702,7 @@ struct drm_dp_aux {
> >  	const char *name;
> >  	struct i2c_adapter ddc;
> >  	struct device *dev;
> > +	struct device *connector;
> >  	struct mutex hw_mutex;
> >  	ssize_t (*transfer)(struct drm_dp_aux *aux,
> >  			    struct drm_dp_aux_msg *msg);
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel