Re: [PATCH v10 3/4] drm/i915: Set aux.dev to the drm_connector device, instead of drm_device.

[Imre Deak <imre.deak@intel.com>]

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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx