| Message ID | 20211210174819.2250178-3-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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
On Sat, Dec 11, 2021 at 1:07 AM Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Dec 10, 2021 at 6:49 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > - 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? Actually it is only used in unbind right below: if (d->panel) drm_panel_bridge_remove(d->bridge_out); If it is not needed at all after your patch (because devm removes the bridge) then also delete this code, and delete the struct drm_panel *panel from struct mcde_dsi at the top and possibly also drop the header #include <drm/drm_panel.h> entirely. Yours, Linus Walleij
Hi Linus, On Sat, Dec 11, 2021 at 5:37 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > 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.) 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.
Hi Linus, On Sat, Dec 11, 2021 at 11:59 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > Hi Linus, > > On Sat, Dec 11, 2021 at 5:37 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > 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.) > > 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.
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c index 001fb39d9919..065cc3b041dd 100644 --- a/drivers/gpu/drm/bridge/analogix/anx7625.c +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c @@ -1333,8 +1333,6 @@ static int anx7625_parse_dt(struct device *dev, struct anx7625_platform_data *pdata) { struct device_node *np = dev->of_node, *ep0; - struct drm_panel *panel; - int ret; int bus_type, mipi_lanes; anx7625_get_swing_setting(dev, pdata); @@ -1371,16 +1369,7 @@ static int anx7625_parse_dt(struct device *dev, if (of_property_read_bool(np, "analogix,audio-enable")) pdata->audio_en = 1; - ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, NULL); - if (ret < 0) { - if (ret == -ENODEV) - return 0; - return ret; - } - if (!panel) - return -ENODEV; - - pdata->panel_bridge = devm_drm_panel_bridge_add(dev, panel); + pdata->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); if (IS_ERR(pdata->panel_bridge)) return PTR_ERR(pdata->panel_bridge); DRM_DEV_DEBUG_DRIVER(dev, "get panel node.\n"); diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c index a6151db95586..23c34039ac48 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -178,7 +178,6 @@ static const struct drm_bridge_funcs chipone_bridge_funcs = { static int chipone_parse_dt(struct chipone *icn) { struct device *dev = icn->dev; - struct drm_panel *panel; int ret; icn->vdd1 = devm_regulator_get_optional(dev, "vdd1"); @@ -214,11 +213,7 @@ static int chipone_parse_dt(struct chipone *icn) return PTR_ERR(icn->enable_gpio); } - ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); - if (ret) - return ret; - - icn->panel_bridge = devm_drm_panel_bridge_add(dev, panel); + icn->panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); if (IS_ERR(icn->panel_bridge)) return PTR_ERR(icn->panel_bridge); diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c index 6becdcdc99fe..f6859dfa6d36 100644 --- a/drivers/gpu/drm/bridge/nwl-dsi.c +++ b/drivers/gpu/drm/bridge/nwl-dsi.c @@ -910,22 +910,10 @@ static int nwl_dsi_bridge_attach(struct drm_bridge *bridge, { struct nwl_dsi *dsi = bridge_to_dsi(bridge); struct drm_bridge *panel_bridge; - struct drm_panel *panel; - int ret; - - ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 1, 0, &panel, - &panel_bridge); - if (ret) - return ret; - - if (panel) { - panel_bridge = drm_panel_bridge_add(panel); - if (IS_ERR(panel_bridge)) - return PTR_ERR(panel_bridge); - } - if (!panel_bridge) - return -EPROBE_DEFER; + panel_bridge = devm_drm_of_get_bridge(dsi->dev, dsi->dev->of_node, 1, 0); + if (IS_ERR(panel_bridge)) + return PTR_ERR(panel_bridge); return drm_bridge_attach(bridge->encoder, panel_bridge, bridge, flags); } diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index e941c1132598..1ab91f4e057b 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -263,7 +263,6 @@ static int ptn3460_probe(struct i2c_client *client, struct device *dev = &client->dev; struct ptn3460_bridge *ptn_bridge; struct drm_bridge *panel_bridge; - struct drm_panel *panel; int ret; ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL); @@ -271,11 +270,7 @@ static int ptn3460_probe(struct i2c_client *client, return -ENOMEM; } - ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL); - if (ret) - return ret; - - panel_bridge = devm_drm_panel_bridge_add(dev, panel); + panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0); if (IS_ERR(panel_bridge)) return PTR_ERR(panel_bridge); diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c index 614b19f0f1b7..37b308850b4e 100644 --- a/drivers/gpu/drm/bridge/parade-ps8622.c +++ b/drivers/gpu/drm/bridge/parade-ps8622.c @@ -452,18 +452,13 @@ static int ps8622_probe(struct i2c_client *client, struct device *dev = &client->dev; struct ps8622_bridge *ps8622; struct drm_bridge *panel_bridge; - struct drm_panel *panel; int ret; ps8622 = devm_kzalloc(dev, sizeof(*ps8622), GFP_KERNEL); if (!ps8622) return -ENOMEM; - ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL); - if (ret) - return ret; - - panel_bridge = devm_drm_panel_bridge_add(dev, panel); + panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0); if (IS_ERR(panel_bridge)) return PTR_ERR(panel_bridge); diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 7900da1d4325..eafd1e5e6852 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -315,7 +315,6 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host, struct dw_mipi_dsi *dsi = host_to_dsi(host); const struct dw_mipi_dsi_plat_data *pdata = dsi->plat_data; struct drm_bridge *bridge; - struct drm_panel *panel; int ret; if (device->lanes > dsi->plat_data->max_data_lanes) { @@ -329,17 +328,9 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host, dsi->format = device->format; dsi->mode_flags = device->mode_flags; - ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, - &panel, &bridge); - if (ret) - return ret; - - if (panel) { - bridge = drm_panel_bridge_add_typed(panel, - DRM_MODE_CONNECTOR_DSI); - if (IS_ERR(bridge)) - return PTR_ERR(bridge); - } + bridge = devm_drm_of_get_bridge(dsi->dev, dsi->dev->of_node, 1, 0); + if (IS_ERR(bridge)) + return PTR_ERR(bridge); dsi->panel_bridge = bridge; diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c index 5651734ce977..9371349b8b25 100644 --- a/drivers/gpu/drm/mcde/mcde_dsi.c +++ b/drivers/gpu/drm/mcde/mcde_dsi.c @@ -1073,9 +1073,7 @@ static int mcde_dsi_bind(struct device *dev, struct device *master, struct drm_device *drm = data; struct mcde *mcde = to_mcde(drm); struct mcde_dsi *d = dev_get_drvdata(dev); - struct device_node *child; - struct drm_panel *panel = NULL; - struct drm_bridge *bridge = NULL; + struct drm_bridge *bridge; if (!of_get_available_child_count(dev->of_node)) { dev_info(dev, "unused DSI interface\n"); @@ -1100,37 +1098,10 @@ static int mcde_dsi_bind(struct device *dev, struct device *master, return PTR_ERR(d->lp_clk); } - /* 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); - 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; - } 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); } d->bridge_out = bridge; diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 5d90d2eb0019..a1b3e1f4b497 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -1004,7 +1004,6 @@ static int mtk_dsi_probe(struct platform_device *pdev) { struct mtk_dsi *dsi; struct device *dev = &pdev->dev; - struct drm_panel *panel; struct resource *regs; int irq_num; int ret; @@ -1021,17 +1020,10 @@ static int mtk_dsi_probe(struct platform_device *pdev) return ret; } - ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, - &panel, &dsi->next_bridge); - if (ret) + dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0); + if (IS_ERR(dsi->next_bridge)) { + ret = PTR_ERR(dsi->next_bridge); goto err_unregister_host; - - if (panel) { - dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); - if (IS_ERR(dsi->next_bridge)) { - ret = PTR_ERR(dsi->next_bridge); - goto err_unregister_host; - } } dsi->driver_data = of_device_get_match_data(dev);
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> --- Note: for mcde_dsi child lookups has dependecy with https://patchwork.kernel.org/project/dri-devel/cover/20211207054747.461029-1-jagan@amarulasolutions.com/ drivers/gpu/drm/bridge/analogix/anx7625.c | 13 +------ drivers/gpu/drm/bridge/chipone-icn6211.c | 7 +--- drivers/gpu/drm/bridge/nwl-dsi.c | 18 ++------- drivers/gpu/drm/bridge/nxp-ptn3460.c | 7 +--- drivers/gpu/drm/bridge/parade-ps8622.c | 7 +--- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 15 ++----- drivers/gpu/drm/mcde/mcde_dsi.c | 39 +++---------------- drivers/gpu/drm/mediatek/mtk_dsi.c | 14 ++----- 8 files changed, 18 insertions(+), 102 deletions(-)