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.
Thanks,
Jagan.
