Hi Doug,
On Wed, Mar 24, 2021 at 03:44:39PM -0700, Doug Anderson wrote:
> On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart 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@xxxxxxxxxxxxxxxx>
> > ---
> > 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)?
Sure, I'll expand the comment.
> > * @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?
Good point, I'll fix that.
> > @@ -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/
Would using the drm_bridge_connector help ? It's a helper that creates a
connector based on a chain of bridges. It implements the .get_modes()
connector operation (see drm_bridge_connector_get_modes()), based on the
.get_edid() operation provided by the bridges. As it has a full view of
the chain, it could enable bridges prior to reading the EDID, and then
power them off, including the panel-bridge.
--
Regards,
Laurent Pinchart
