Re: [PATCH 3/3] drm: bridge: Switch to devm_drm_of_get_bridge

On Fri, Dec 10, 2021 at 6:49 PM Jagan Teki jagan@amarulasolutions.com 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@pengutronix.de Cc: Chun-Kuang Hu chunkuang.hu@kernel.org Cc: Linus Walleij linus.walleij@linaro.org Signed-off-by: Jagan Teki jagan@amarulasolutions.com

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.)

•           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?

•   } 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.

Yours, Linus Walleij