Re: [PATCH v5 09/10] drm/msm/dp: turn link_ready into plugged

[Konrad Dybcio]

On 3/14/26 2:09 AM, Dmitry Baryshkov wrote:
> Tracking when the DP link is ready isn't that useful from the driver
> point of view. It doesn't provide a direct information if the device
> should be suspended, etc. Replace it with the 'plugged' boolean, which
> is set when the driver knows that there is DPRX plugged.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> ---

[...]

> -static void msm_dp_display_host_phy_init(struct msm_dp_display_private *dp)
> +static bool msm_dp_display_host_phy_init(struct msm_dp_display_private *dp)
>  {
>  	drm_dbg_dp(dp->drm_dev, "type=%d core_init=%d phy_init=%d\n",
>  		dp->msm_dp_display.connector_type, dp->core_initialized,
> @@ -312,7 +313,10 @@ static void msm_dp_display_host_phy_init(struct msm_dp_display_private *dp)
>  	if (!dp->phy_initialized) {
>  		msm_dp_ctrl_phy_init(dp->ctrl);
>  		dp->phy_initialized = true;
> +		return true;
>  	}
> +
> +	return false;
>  }

Please either rename this function or add kerneldoc - it's not obvious
what "msm_dp_display_host_phy_init() == true" is supposed to mean


>  
>  static void msm_dp_display_host_phy_exit(struct msm_dp_display_private *dp)
> @@ -366,14 +370,6 @@ static int msm_dp_display_handle_irq_hpd(struct msm_dp_display_private *dp)
>  	u32 sink_request = dp->link->sink_request;
>  
>  	drm_dbg_dp(dp->drm_dev, "%d\n", sink_request);
> -	if (!dp->msm_dp_display.link_ready) {
> -		if (sink_request & DP_LINK_STATUS_UPDATED) {
> -			drm_dbg_dp(dp->drm_dev, "Disconnected sink_request: %d\n",
> -							sink_request);
> -			DRM_ERROR("Disconnected, no DP_LINK_STATUS_UPDATED\n");
> -			return -EINVAL;
> -		}
> -	}
>  
>  	msm_dp_ctrl_handle_sink_request(dp->ctrl);
>  
> @@ -392,11 +388,11 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
>  			dp->msm_dp_display.connector_type,
>  			dp->link->sink_count);
>  
> -	if (dp->msm_dp_display.link_ready)
> -		return 0;
> +	mutex_lock(&dp->plugged_lock);

guard(mutex)(&dp->plugged_lock)

[...]

>  static void msm_dp_display_handle_plugged_change(struct msm_dp *msm_dp_display,
> @@ -446,8 +440,12 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
>  			dp->msm_dp_display.connector_type,
>  			dp->link->sink_count);
>  
> -	if (!dp->msm_dp_display.link_ready)
> +	mutex_lock(&dp->plugged_lock);

likewise

[...]

>  end:
> -	pm_runtime_put_sync(&dp->pdev->dev);
> +	/*
> +	 * If we detected the DPRX, leave the controller on so that it doesn't
> +	 * loose the state.

loose -> lose

> +	 */
> +	if (!priv->plugged) {
> +		if (phy_deinit) {
> +			msm_dp_aux_enable_xfers(priv->aux, false);
> +			msm_dp_display_host_phy_exit(priv);

Should msm_dp_aux_enable_xfers() logically be called anywhere outside
phy_init/exit()?

> +		}
> +
> +		pm_runtime_put_sync(&dp->pdev->dev);
> +	}
> +
> +	mutex_unlock(&priv->plugged_lock);
> +
>  	return status;
>  }
>  
> @@ -1123,6 +1145,8 @@ static int msm_dp_display_probe(struct platform_device *pdev)
>  		(dp->msm_dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
>  	dp->hpd_isr_status = 0;
>  
> +	mutex_init(&dp->plugged_lock);

devm/drmm?

Konrad