On Wed, 11 Nov 2015, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Use kasprintf() to generate the "DPDDC-<port>" name for the aux helper.
>
> To deal with errors properly make intel_dp_aux_init() return something,
> and adjust the caller to match. It seems we were also missing a
> intel_dp_mst_encoder_cleanup() call on edp (non-port A) init failures,
> so add that too.
>
> The whole error/cleanup ordering doesn't feel entirely sane to me, but
> I'll leave that part alone for now.
>
> v2: Use kasprintf() instead of a table, reorder patches (Chis)
>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 75 +++++++++++++++++++++++++----------------
> 1 file changed, 46 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 891a7f8..df2a2d2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1009,6 +1009,13 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> }
>
> static void
> +intel_dp_aux_fini(struct intel_dp *intel_dp)
> +{
> + drm_dp_aux_unregister(&intel_dp->aux);
> + kfree(intel_dp->aux.name);
> +}
> +
> +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);
> @@ -1016,7 +1023,6 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> enum port port = intel_dig_port->port;
> struct ddi_vbt_port_info *info = &dev_priv->vbt.ddi_port_info[port];
> - const char *name = NULL;
> uint32_t porte_aux_ctl_reg = DPA_AUX_CH_CTL;
> int ret;
>
> @@ -1043,23 +1049,18 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
> switch (port) {
> case PORT_A:
> intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
> - name = "DPDDC-A";
> break;
> case PORT_B:
> intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL;
> - name = "DPDDC-B";
> break;
> case PORT_C:
> intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL;
> - name = "DPDDC-C";
> break;
> case PORT_D:
> intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL;
> - name = "DPDDC-D";
> break;
> case PORT_E:
> intel_dp->aux_ch_ctl_reg = porte_aux_ctl_reg;
> - name = "DPDDC-E";
> break;
> default:
> BUG();
> @@ -1077,27 +1078,36 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
> if (!IS_HASWELL(dev) && !IS_BROADWELL(dev) && port != PORT_E)
> intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
>
> - intel_dp->aux.name = name;
> + intel_dp->aux.name = kasprintf(GFP_KERNEL, "DPDDC-%c", port_name(port));
> + if (!intel_dp->aux.name)
> + return -ENOMEM;
> +
> intel_dp->aux.dev = dev->dev;
> intel_dp->aux.transfer = intel_dp_aux_transfer;
>
> - DRM_DEBUG_KMS("registering %s bus for %s\n", name,
> + DRM_DEBUG_KMS("registering %s bus for %s\n",
> + intel_dp->aux.name,
> connector->base.kdev->kobj.name);
>
> ret = drm_dp_aux_register(&intel_dp->aux);
> if (ret < 0) {
> DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
> - name, ret);
> - return;
> + intel_dp->aux.name, ret);
> + kfree(intel_dp->aux.name);
> + 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", name, ret);
> - drm_dp_aux_unregister(&intel_dp->aux);
> + 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
> @@ -4771,7 +4781,7 @@ 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;
>
> - drm_dp_aux_unregister(&intel_dp->aux);
> + 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);
> @@ -5752,7 +5762,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> struct drm_device *dev = intel_encoder->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> enum port port = intel_dig_port->port;
> - int type;
> + int type, ret;
>
> intel_dp->pps_pipe = INVALID_PIPE;
>
> @@ -5853,7 +5863,9 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> pps_unlock(intel_dp);
> }
>
> - intel_dp_aux_init(intel_dp, intel_connector);
> + ret = intel_dp_aux_init(intel_dp, intel_connector);
> + if (ret)
> + goto fail;
>
> /* init MST on ports that can support it */
> if (HAS_DP_MST(dev) &&
> @@ -5862,20 +5874,9 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> intel_connector->base.base.id);
>
> if (!intel_edp_init_connector(intel_dp, intel_connector)) {
> - drm_dp_aux_unregister(&intel_dp->aux);
> - if (is_edp(intel_dp)) {
> - cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> - /*
> - * vdd might still be enabled do to the delayed vdd off.
> - * Make sure vdd is actually turned off here.
> - */
> - pps_lock(intel_dp);
> - edp_panel_vdd_off_sync(intel_dp);
> - pps_unlock(intel_dp);
> - }
> - drm_connector_unregister(connector);
> - drm_connector_cleanup(connector);
> - return false;
> + intel_dp_aux_fini(intel_dp);
> + intel_dp_mst_encoder_cleanup(intel_dig_port);
> + goto fail;
> }
>
> intel_dp_add_properties(intel_dp, connector);
> @@ -5892,6 +5893,22 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> i915_debugfs_connector_add(connector);
>
> return true;
> +
> +fail:
> + if (is_edp(intel_dp)) {
> + cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> + /*
> + * vdd might still be enabled do to the delayed vdd off.
> + * Make sure vdd is actually turned off here.
> + */
> + pps_lock(intel_dp);
> + edp_panel_vdd_off_sync(intel_dp);
> + pps_unlock(intel_dp);
> + }
A follow-up might abstract this sequence as there's three copies now.
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> + drm_connector_unregister(connector);
> + drm_connector_cleanup(connector);
> +
> + return false;
> }
>
> void
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
