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

[Jagan Teki]

Hi Linus,

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

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.

Thanks,
Jagan.