Re: [RFC PATCH 05/11] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

[Doug Anderson <dianders@chromium.org>]

Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> To simplify interfacing with the panel, wrap it in a panel-bridge and
> let the DRM bridge helpers handle chaining of operations.
>
> This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> requires all components in the display pipeline to be represented by
> bridges.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 +++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 1d1be791d5ba..c21a7f7d452b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -124,6 +124,7 @@
>   * @edid:         Detected EDID of eDP panel.
>   * @refclk:       Our reference clock.
>   * @panel:        Our panel.
> + * @next_bridge:  The next bridge.

To make it easier for folks who don't work with DRM all day, could you
somehow clarify which direction "next" is talking about. AKA the next
"outward" (towards the sink) or the next "inward" (toward the source)?


>   * @enable_gpio:  The GPIO we toggle to enable the bridge.
>   * @supplies:     Data for bulk enabling/disabling our regulators.
>   * @dp_lanes:     Count of dp_lanes we're using.
> @@ -152,6 +153,7 @@ struct ti_sn_bridge {
>         struct mipi_dsi_device          *dsi;
>         struct clk                      *refclk;
>         struct drm_panel                *panel;
> +       struct drm_bridge               *next_bridge;

There's no reason to store the "panel" pointer anymore, right? It can
just be a local variable in probe?


> @@ -850,8 +856,6 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>          */
>         regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
>                            HPD_DISABLE);
> -
> -       drm_panel_prepare(pdata->panel);

Ugh, I guess conflicts with my EDID patch [1] which assumes that this
function will directly turn the panel on. I'll see if I can find some
solution...

[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/