Hi Linus, On Sat, Dec 11, 2021 at 11:59 AM Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote: > > Hi Linus, > > On Sat, Dec 11, 2021 at 5:37 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > > > > On Fri, Dec 10, 2021 at 6:49 PM Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > devm_drm_of_get_bridge is capable of looking up the downstream > > > bridge and panel and trying to add a panel bridge if the panel > > > is found. > > > > > > Replace explicit finding calls with devm_drm_of_get_bridge. > > > > > > Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > > > Cc: Chun-Kuang Hu <chunkuang.hu@xxxxxxxxxx> > > > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx> > > > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> > > > > Nice overall! > > > > > - /* Look for a panel as a child to this node */ > > > - for_each_available_child_of_node(dev->of_node, child) { > > > - panel = of_drm_find_panel(child); > > > - if (IS_ERR(panel)) { > > > - dev_err(dev, "failed to find panel try bridge (%ld)\n", > > > - PTR_ERR(panel)); > > > - panel = NULL; > > > - > > > - bridge = of_drm_find_bridge(child); > > > - if (!bridge) { > > > - dev_err(dev, "failed to find bridge\n"); > > > - return -EINVAL; > > > - } > > > - } > > > - } > > > - if (panel) { > > > - bridge = drm_panel_bridge_add_typed(panel, > > > - DRM_MODE_CONNECTOR_DSI); > > > > And we are guaranteed that the right type of connector will be > > used here? (Just checking.) > > Yes. Each panel driver initialised the connector_type via > drm_panel_init and it will check during devm_drm_of_get_bridge. > > > > > > - if (IS_ERR(bridge)) { > > > - dev_err(dev, "error adding panel bridge\n"); > > > - return PTR_ERR(bridge); > > > - } > > > - dev_info(dev, "connected to panel\n"); > > > - d->panel = panel; > > > > How does this assignment happen after your patch? > > I'm using that... > > > > devm_drm_of_get_bridge() needs some more argument right? > > Yes, but I think we don't need to preserve the panel here. Yes I > didn't add that change, will take care in v2. > > > > > > - } else if (bridge) { > > > - /* TODO: AV8100 HDMI encoder goes here for example */ > > > - dev_info(dev, "connected to non-panel bridge (unsupported)\n"); > > > - return -ENODEV; > > > - } else { > > > - dev_err(dev, "no panel or bridge\n"); > > > - return -ENODEV; > > > + bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0); > > > + if (IS_ERR(bridge)) { > > > + dev_err(dev, "error to get bridge\n"); > > > + return PTR_ERR(bridge); > > > > I'm gonna want to test this somehow on the hardware. But the TODO comment > > there wasn't supposed to be deleted if I will still need to take some special > > action whether this is a panel bridge or some other bridge. > > Agreed. Even I do like to add this prints, since > devm_drm_of_get_bridge is not able to return differentiated bridge so > it it not possible now. May be we can discuss this point. Any comments on this? I will try to send the next version based on it. Thanks, Jagan.