Hi Lukas, Sorry for the late answer, I went on vacation and then was busy with something else. Anyway, I tried to address all comments (yours and Daniel's) on a new version. See some comments below. On Sun, Dec 06, 2015 at 05:08:49PM +0100, Lukas Wunner wrote: > Hi Rafael, > > On Thu, Dec 03, 2015 at 02:54:02PM -0800, Rafael Antognolli wrote: > > So far, the i915 driver and some other drivers set it to the drm_device, > > which doesn't allow one to know which DP a given aux channel is related > > to. Changing this to be the drm_connector provides proper nesting, still > > allowing one to get the drm_device from it. Some drivers already set it > > to the drm_connector. > > > > This also removes the need to add a sysfs link for the i2c device under > > the connector, as it will already be there. > > > > v2: > > Two minor nits, I think this should be "v9" instead of "v2" because > this was newly added since v8, and the subject should be prefixed > "drm/i915" (or "drm/i915/dp" or whatever) instead of "drm/dp" because > this only touches i915 and not drm core. > > > > - As a side effect, drm_dp_aux_unregister() must be called before > > intel_connector_unregister(), as both the aux.dev and the i2c adapter > > dev are children of the drm_connector device now. Calling > > drm_dp_aux_unregister() before prevents them from being destroyed > > twice. > > I haven't verified what Ville wrote (that there are WARNs because of > this ordering issue), but if this is true then we need to make sure > all other drivers get the order right, otherwise they'd spew WARNs > as well once this gets merged. > > I've checked that for every driver and the only one affected is radeon. > A fix is now in my GitHub repo, feel free to include the commit in your > series if you want to. I haven't been able to actually test it though > as I don't have a radeon card: > https://github.com/l1k/linux/commit/485e197a7cac88e24348150526988149428feeaa > Nice, I added it to the series, though I also don't have a radeon to test it. > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 22 ++++------------------ > > 1 file changed, 4 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index f2bfca0..515893c 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp) > > static void > > intel_dp_aux_fini(struct intel_dp *intel_dp) > > { > > - drm_dp_aux_unregister(&intel_dp->aux); > > kfree(intel_dp->aux.name); > > } > > Hm, instead of moving the single call of drm_dp_aux_unregister() > to intel_dp_connector_unregister() I think it would be more clear > to call intel_dp_aux_fini() from intel_dp_connector_unregister() > and remove its invocation from intel_dp_encoder_destroy(). > > Ville introduced intel_dp_aux_fini() very recently with a121f4e5fae5, > he should probably have a say on how to solve this. I makes sense to me, so I did what you suggest. Thanks for the review, Rafael > > > > static int > > intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > { > > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > enum port port = intel_dig_port->port; > > int ret; > > @@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > if (!intel_dp->aux.name) > > return -ENOMEM; > > > > - intel_dp->aux.dev = dev->dev; > > + intel_dp->aux.dev = connector->base.kdev; > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", > > @@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > return ret; > > } > > > > - ret = sysfs_create_link(&connector->base.kdev->kobj, > > - &intel_dp->aux.ddc.dev.kobj, > > - intel_dp->aux.ddc.dev.kobj.name); > > - if (ret < 0) { > > - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", > > - intel_dp->aux.name, ret); > > - intel_dp_aux_fini(intel_dp); > > - return ret; > > - } > > - > > return 0; > > } > > > > static void > > intel_dp_connector_unregister(struct intel_connector *intel_connector) > > { > > - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); > > - > > - if (!intel_connector->mst_port) > > - sysfs_remove_link(&intel_connector->base.kdev->kobj, > > - intel_dp->aux.ddc.dev.kobj.name); > > + struct intel_dp *intel_dp = > > + enc_to_intel_dp(&intel_connector->encoder->base); > > + drm_dp_aux_unregister(&intel_dp->aux); > > intel_connector_unregister(intel_connector); > > } > > > > -- > > 2.4.3 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
