Hi Stephen,
Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index d7a19d6..055681a 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c
Some nitpicks
Reviewed-by: Stephen Boyd swboyd@chromium.org
@@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp_hpd_event_setup(dp);
dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
if (!dp_display->is_edp)dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);Did it turn out that in fact DP isn't ready still to setup even after delaying the irq?
The host_init, config_hpd, phy_init and enable_irq are happening in modeset_init already for eDP. So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying the delay for DP.
}
void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor) @@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct
msm_dp *dp_display, struct drm_minor *minor)
}}
+static int dp_display_get_next_bridge(struct msm_dp *dp) {
int rc;struct dp_display_private *dp_priv;struct device_node *aux_bus;struct device *dev;dp_priv = container_of(dp, struct dp_display_private, dp_display);dev = &dp_priv->pdev->dev;aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");if (aux_bus && dp->is_edp) {dp_display_host_init(dp_priv);dp_catalog_ctrl_hpd_config(dp_priv->catalog);dp_display_host_phy_init(dp_priv);enable_irq(dp_priv->irq);/** The code below assumes that the panel will finish probing* by the time devm_of_dp_aux_populate_ep_devices() returns.* This isn't a great assumption since it will fail if the* panel driver is probed asynchronously but is the best we* can do without a bigger driver reorganization.*/rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);of_node_put(aux_bus);if (rc)goto error;} else if (dp->is_edp) {DRM_ERROR("eDP aux_bus not found\n");return -ENODEV;}/** External bridges are mandatory for eDP interfaces: one has to* provide at least an eDP panel (which gets wrapped into panel-bridge).
** For DisplayPort interfaces external bridges are optional, so* silently ignore an error if one is not present (-ENODEV).*/rc = dp_parser_find_next_bridge(dp_priv->parser);if (!dp->is_edp && rc == -ENODEV)return 0;else if (rc)Just an if because there's a return above.
Okay
goto error;dp->next_bridge = dp_priv->parser->next_bridge;In which case it almost feels like it could be written
if (!dp->is_edp && rc == -ENODEV) return 0; if (!rc) { dp->next_bridge = dp_priv->parser->next_bridge; return 0; } error: if (dp->is_edp) {
but I'm not worried either way, besides removing the else from the else-if.
Okay
return 0;+error:
if (dp->is_edp) {disable_irq(dp_priv->irq);dp_display_host_phy_exit(dp_priv);dp_display_host_deinit(dp_priv);}return rc;+}
int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
*dev,
struct drm_encoder *encoder) { diff --gita/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h index d371bae..950416c 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.h +++ b/drivers/gpu/drm/msm/dp/dp_parser.h @@ -125,7 +125,7 @@ struct dp_parser { u32 max_dp_lanes; struct drm_bridge *next_bridge;
int (*parse)(struct dp_parser *parser, int connector_type);
int (*parse)(struct dp_parser *parser);};
/** @@ -141,4 +141,15 @@ struct dp_parser { */ struct dp_parser *dp_parser_get(struct platform_device *pdev);
+/**
- dp_parser_find_next_bridge() - find an additional bridge to DP
- @parser: dp_parser data from client
- return: 0 if able to get the bridge else return an error code
return comes after the description below. Also should be capitalized. You can check this by compiling with W=1 I believe, or run the kernel doc format script on the file..
Okay
- This function is used to find any additional bridge attached to
- the DP controller. The eDP interface requires a panel bridge.
Return: 0 if able to get the bridge, otherwise negative errno for failure
Okay
- */
+int dp_parser_find_next_bridge(struct dp_parser *parser);