On 24-03-22 09:30:21, Bjorn Andersson wrote: > On Fri, Mar 22, 2024 at 03:22:22PM +0200, Abel Vesa wrote: > > Instead of relying on different compatibles for eDP and DP, lookup > > the panel node in devicetree to figure out the connector type and > > then pass on that information to the PHY. External DP is not described > > in DT, therefore, assume it's eDP if panel node is present. > > > > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx> > > --- > > drivers/gpu/drm/msm/dp/dp_display.c | 43 +++++++++++++++++++++++++++++++++---- > > 1 file changed, 39 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > > index c4cb82af5c2f..c9763f77c832 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > @@ -726,6 +726,14 @@ static int dp_init_sub_modules(struct dp_display_private *dp) > > if (IS_ERR(phy)) > > return PTR_ERR(phy); > > > > + rc = phy_set_mode_ext(phy, PHY_MODE_DP, > > + dp->dp_display.is_edp ? PHY_SUBMODE_EDP : PHY_SUBMODE_DP); > > + if (rc) { > > + DRM_ERROR("failed to set phy submode, rc = %d\n", rc); > > + dp->catalog = NULL; > > + goto error; > > + } > > + > > dp->catalog = dp_catalog_get(dev); > > if (IS_ERR(dp->catalog)) { > > rc = PTR_ERR(dp->catalog); > > @@ -734,9 +742,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp) > > goto error; > > } > > > > - dp->aux = dp_aux_get(dev, dp->catalog, > > - phy, > > - dp->dp_display.is_edp); > > + dp->aux = dp_aux_get(dev, dp->catalog, phy, dp->dp_display.is_edp); > > if (IS_ERR(dp->aux)) { > > rc = PTR_ERR(dp->aux); > > DRM_ERROR("failed to initialize aux, rc = %d\n", rc); > > @@ -1241,6 +1247,35 @@ static int dp_auxbus_done_probe(struct drm_dp_aux *aux) > > return dp_display_probe_tail(aux->dev); > > } > > > > +static int dp_display_get_connector_type(struct platform_device *pdev, > > + const struct msm_dp_desc *desc) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *aux_bus; > > + struct device_node *panel; > > + int ret = DRM_MODE_CONNECTOR_DisplayPort; > > + > > + /* legacy platforms specify connector type in match data */ > > + if (desc->connector_type == DRM_MODE_CONNECTOR_eDP || > > + desc->connector_type == DRM_MODE_CONNECTOR_DisplayPort) > > + return desc->connector_type; > > + > > + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); > > + if (!aux_bus) > > + goto out; > > My compiler warns that if we take this code path, then you will > of_node_put(<uninitialized panel>) below. > > > + > > + panel = of_get_child_by_name(aux_bus, "panel"); > > + if (!panel) > > + goto out; > > + > > + ret = DRM_MODE_CONNECTOR_eDP; > > My brain read this function as: > check something > if (error) > bailout! > > check something > if (error) > bailout! > > ret should be edp > > I then have to scan the code again to figure out what ret is otherwise, > and convince myself that the error path is never an error, but a totally > normal case. > > > If you instead rely on the fact that both of_get_child_by_name() and > of_node_put() can be passed NULL, you can write this as: > > static int fn(..) { > aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); > panel = of_get_child_by_name(aux_bus, "panel"); > > if (panel) > connector_type = DRM_MODE_CONNECTOR_eDP; > else > connector_type = DRM_MODE_CONNECTOR_DisplayPort; > > of_node_put(panel); > of_node_put(aux_bus); > > return connector_type; > } > > Much easier to read, and you don't even have to zero-initialize panel to > avoid that compiler warning. > Fair enough, will do that instead. > Regards, > Bjorn > > > + > > +out: > > + of_node_put(panel); > > + of_node_put(aux_bus); > > + return ret; > > +} > > + > > static int dp_display_probe(struct platform_device *pdev) > > { > > int rc = 0; > > @@ -1263,7 +1298,7 @@ static int dp_display_probe(struct platform_device *pdev) > > dp->dp_display.pdev = pdev; > > dp->name = "drm_dp"; > > dp->id = desc->id; > > - dp->dp_display.connector_type = desc->connector_type; > > + dp->dp_display.connector_type = dp_display_get_connector_type(pdev, desc); > > dp->wide_bus_supported = desc->wide_bus_supported; > > dp->dp_display.is_edp = > > (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP); > > > > -- > > 2.34.1 > >