On Tue 29 Mar 06:27 PDT 2022, Paul Kocialkowski wrote: > While bridge/panel detection was initially relying on the usual > port/ports-based of graph detection, it was recently changed to > perform the lookup on any child node that is not port/ports > instead when such a node is available, with no fallback on the > usual way. > > This results in breaking detection when a child node is present > but does not contain any panel or bridge node, even when the > usual port/ports-based of graph is there. > > In order to support both situations properly, this commit reworks > the logic to try both options and not just one of the two: it will > only return -EPROBE_DEFER when both have failed. > Thanks for your patch Paul, it fixed a regression on a device where I have a eDP bridge with an of_graph and a aux-bus defined. But unfortunately it does not resolve the regression I have for the USB based DisplayPort setup described below. In the Qualcomm DisplayPort driver We're calling: devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); and with the following DT snippet the behavior changed: displayport-controller@ae90000 { compatible = "qcom,sc8180x-dp"; ... operating-points-v2 = <&dp0_opp_table>; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; dp0_in: endpoint { remote-endpoint = <&display_driver>; }; }; }; dp0_opp_table: opp-table { ...; }; }; Prior to the introduction of 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") this would return -ENODEV, so we could differentiate the case when we have a statically defined eDP panel from that of a dynamically attached (over USB) DP panel. Prior to your change, above case without the opp-table node would have still returned -ENODEV. But now this will just return -EPROBE_DEFER in both cases. I thought the appropriate method of referencing the dsi panel was to actually reference that using the of_graph, even though it's a child of the dsi controller - that's at least how we've done it in e.g. [1]. I find this to be much nicer than to just blindly define that all children of any sort of display controller must be a bridge or a panel. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845-mtp.dts#n436 Regards, Bjorn > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx> > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") > --- > > Changes since v2: > - Removed unnecessary else statement and added a comment about > clearing the panel pointer on error. > > Changes since v1: > - Renamed remote to node; > - Renamed helper to find_panel_or_bridge; > - Cleared bridge pointer early; > - Returned early to make the code more concise; > > --- > drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++-------------------- > 1 file changed, 50 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index 9d90cd75c457..8716da6369a6 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -219,6 +219,29 @@ int drm_of_encoder_active_endpoint(struct device_node *node, > } > EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); > > +static int find_panel_or_bridge(struct device_node *node, > + struct drm_panel **panel, > + struct drm_bridge **bridge) > +{ > + if (panel) { > + *panel = of_drm_find_panel(node); > + if (!IS_ERR(*panel)) > + return 0; > + > + /* Clear the panel pointer in case of error. */ > + *panel = NULL; > + } > + > + /* No panel found yet, check for a bridge next. */ > + if (bridge) { > + *bridge = of_drm_find_bridge(node); > + if (*bridge) > + return 0; > + } > + > + return -EPROBE_DEFER; > +} > + > /** > * drm_of_find_panel_or_bridge - return connected panel or bridge device > * @np: device tree node containing encoder output ports > @@ -241,66 +264,44 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > struct drm_panel **panel, > struct drm_bridge **bridge) > { > - int ret = -EPROBE_DEFER; > - struct device_node *remote; > + struct device_node *node; > + int ret; > > if (!panel && !bridge) > return -EINVAL; > + > if (panel) > *panel = NULL; > - > - /** > - * Devices can also be child nodes when we also control that device > - * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). > - * > - * Lookup for a child node of the given parent that isn't either port > - * or ports. > - */ > - for_each_available_child_of_node(np, remote) { > - if (of_node_name_eq(remote, "port") || > - of_node_name_eq(remote, "ports")) > - continue; > - > - goto of_find_panel_or_bridge; > + if (bridge) > + *bridge = NULL; > + > + /* Check for a graph on the device node first. */ > + if (of_graph_is_present(np)) { > + node = of_graph_get_remote_node(np, port, endpoint); > + if (node) { > + ret = find_panel_or_bridge(node, panel, bridge); > + of_node_put(node); > + > + if (!ret) > + return 0; > + } > } > > - /* > - * of_graph_get_remote_node() produces a noisy error message if port > - * node isn't found and the absence of the port is a legit case here, > - * so at first we silently check whether graph presents in the > - * device-tree node. > - */ > - if (!of_graph_is_present(np)) > - return -ENODEV; > - > - remote = of_graph_get_remote_node(np, port, endpoint); > - > -of_find_panel_or_bridge: > - if (!remote) > - return -ENODEV; > + /* Otherwise check for any child node other than port/ports. */ > + for_each_available_child_of_node(np, node) { > + if (of_node_name_eq(node, "port") || > + of_node_name_eq(node, "ports")) > + continue; > > - if (panel) { > - *panel = of_drm_find_panel(remote); > - if (!IS_ERR(*panel)) > - ret = 0; > - else > - *panel = NULL; > - } > - > - /* No panel found yet, check for a bridge next. */ > - if (bridge) { > - if (ret) { > - *bridge = of_drm_find_bridge(remote); > - if (*bridge) > - ret = 0; > - } else { > - *bridge = NULL; > - } > + ret = find_panel_or_bridge(node, panel, bridge); > + of_node_put(node); > > + /* Stop at the first found occurrence. */ > + if (!ret) > + return 0; > } > > - of_node_put(remote); > - return ret; > + return -EPROBE_DEFER; > } > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); > > -- > 2.35.1 >