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

[Jagan Teki]

Hi Linus,

On Sat, Dec 11, 2021 at 11:59 AM Jagan Teki <jagan at amarulasolutions.com> wrote:
>
> 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.

Any comments on this? I will try to send the next version based on it.

Thanks,
Jagan.