| Message ID | 20220318160249.126431-1-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm: of: Properly try all possible cases for bridge/panel detection | expand |
On Fri, Mar 18, 2022 at 05:02:49PM +0100, 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. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") > --- > > 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 | 98 ++++++++++++++++++++-------------------- > 1 file changed, 49 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index 9d90cd75c457..63137c833b7a 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -219,6 +219,28 @@ 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; > + else > + *panel = NULL; You don't need the else branch here, we already cleared panel in drm_of_find_panel_or_bridge Looks good otherwise, thanks! Maxime
Hi Maxime, On Fri 18 Mar 22, 17:18, Maxime Ripard wrote: > On Fri, Mar 18, 2022 at 05:02:49PM +0100, 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. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") > > --- > > > > 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 | 98 ++++++++++++++++++++-------------------- > > 1 file changed, 49 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > > index 9d90cd75c457..63137c833b7a 100644 > > --- a/drivers/gpu/drm/drm_of.c > > +++ b/drivers/gpu/drm/drm_of.c > > @@ -219,6 +219,28 @@ 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; > > + else > > + *panel = NULL; > > You don't need the else branch here, we already cleared panel in > drm_of_find_panel_or_bridge I think we do, because of_drm_find_panel doesn't return NULL when an error happens but PTR_ERR(), so we need to clear it to keep up with previous expectations. > Looks good otherwise, thanks! Great, thanks for the review! Paul
On Fri, Mar 18, 2022 at 05:22:46PM +0100, Paul Kocialkowski wrote: > On Fri 18 Mar 22, 17:18, Maxime Ripard wrote: > > On Fri, Mar 18, 2022 at 05:02:49PM +0100, 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. > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") > > > --- > > > > > > 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 | 98 ++++++++++++++++++++-------------------- > > > 1 file changed, 49 insertions(+), 49 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > > > index 9d90cd75c457..63137c833b7a 100644 > > > --- a/drivers/gpu/drm/drm_of.c > > > +++ b/drivers/gpu/drm/drm_of.c > > > @@ -219,6 +219,28 @@ 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; > > > + else > > > + *panel = NULL; > > > > You don't need the else branch here, we already cleared panel in > > drm_of_find_panel_or_bridge > > I think we do, because of_drm_find_panel doesn't return NULL when an error > happens but PTR_ERR(), so we need to clear it to keep up with previous > expectations. Ah, right. We should remove the else then, checkpatch complains about it. Maxime
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 9d90cd75c457..63137c833b7a 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -219,6 +219,28 @@ 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; + else + *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 +263,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; - - if (panel) { - *panel = of_drm_find_panel(remote); - if (!IS_ERR(*panel)) - ret = 0; - else - *panel = NULL; - } + /* 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; - /* 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);
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. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Fixes: 80253168dbfd ("drm: of: Lookup if child node has panel or bridge") --- 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 | 98 ++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 49 deletions(-)