| Message ID | 1453417821-2811-4-git-send-email-rafael.antognolli@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-01-21 at 15:10 -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. Yes, having the i2c-dev only at one place under the connector is more logical and what we want imo. It's an ABI change but if other drivers already have it in this way then I assume it's fine. For HDMI connectors we still have them under the drm_device (and there is no symlink for them under the connector device) but this was inconsistent already before this patch and can be fixed up later. Adding Jani, in case he has comments on this. Below one more comment: > v9: > - 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. > > v10: > - move aux_fini() to connector_unregister(), instead of moving > drm_dp_aux_unregister() outside of connector_register(). > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 18 ++---------------- > 1 file changed, 2 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index e2bea710..da704c6 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1188,7 +1188,6 @@ intel_dp_aux_fini(struct intel_dp *intel_dp) > 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; > @@ -1199,7 +1198,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", > @@ -1214,16 +1213,6 @@ 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; > } > > @@ -1232,9 +1221,7 @@ 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); > + intel_dp_aux_fini(intel_dp); Hrm, the mst_port check seems to have been misplaced at some point, this function isn't called for virtual MST connectors. So I think it's fine to remove it. The patch looks ok: Reviewed-by: Imre Deak <imre.deak@intel.com> > intel_connector_unregister(intel_connector); > } > > @@ -4868,7 +4855,6 @@ void intel_dp_encoder_destroy(struct > drm_encoder *encoder) > struct intel_digital_port *intel_dig_port = > enc_to_dig_port(encoder); > struct intel_dp *intel_dp = &intel_dig_port->dp; > > - intel_dp_aux_fini(intel_dp); > intel_dp_mst_encoder_cleanup(intel_dig_port); > if (is_edp(intel_dp)) { > cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
On Fri, Feb 12, 2016 at 10:28:28PM +0200, Imre Deak wrote: > On Thu, 2016-01-21 at 15:10 -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. > > Yes, having the i2c-dev only at one place under the connector is more > logical and what we want imo. It's an ABI change but if other drivers > already have it in this way then I assume it's fine. For HDMI > connectors we still have them under the drm_device (and there is no > symlink for them under the connector device) but this was inconsistent > already before this patch and can be fixed up later. > > Adding Jani, in case he has comments on this. > > Below one more comment: > > > v9: > > - 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. > > > > v10: > > - move aux_fini() to connector_unregister(), instead of moving > > drm_dp_aux_unregister() outside of connector_register(). > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 18 ++---------------- > > 1 file changed, 2 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index e2bea710..da704c6 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1188,7 +1188,6 @@ intel_dp_aux_fini(struct intel_dp *intel_dp) > > 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; > > @@ -1199,7 +1198,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", > > @@ -1214,16 +1213,6 @@ 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; > > } > > > > @@ -1232,9 +1221,7 @@ 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); > > + intel_dp_aux_fini(intel_dp); > > Hrm, the mst_port check seems to have been misplaced at some point, > this function isn't called for virtual MST connectors. So I think it's > fine to remove it. The patch looks ok: > > Reviewed-by: Imre Deak <imre.deak@intel.com> Added to drm-misc, thanks. -Daniel > > > intel_connector_unregister(intel_connector); > > } > > > > @@ -4868,7 +4855,6 @@ void intel_dp_encoder_destroy(struct > > drm_encoder *encoder) > > struct intel_digital_port *intel_dig_port = > > enc_to_dig_port(encoder); > > struct intel_dp *intel_dp = &intel_dig_port->dp; > > > > - intel_dp_aux_fini(intel_dp); > > intel_dp_mst_encoder_cleanup(intel_dig_port); > > if (is_edp(intel_dp)) { > > cancel_delayed_work_sync(&intel_dp->panel_vdd_work); > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e2bea710..da704c6 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1188,7 +1188,6 @@ intel_dp_aux_fini(struct intel_dp *intel_dp) 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; @@ -1199,7 +1198,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", @@ -1214,16 +1213,6 @@ 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; } @@ -1232,9 +1221,7 @@ 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); + intel_dp_aux_fini(intel_dp); intel_connector_unregister(intel_connector); } @@ -4868,7 +4855,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); struct intel_dp *intel_dp = &intel_dig_port->dp; - intel_dp_aux_fini(intel_dp); intel_dp_mst_encoder_cleanup(intel_dig_port); if (is_edp(intel_dp)) { cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
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. v9: - 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. v10: - move aux_fini() to connector_unregister(), instead of moving drm_dp_aux_unregister() outside of connector_register(). Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)