Hi Doug, Thank you for the patch.
On Fri, Feb 04, 2022 at 04:13:40PM -0800, Douglas Anderson wrote: > The ti-sn65dsi86 driver shouldn't hand-roll its own bridge > connector. It should use the normal drm_bridge_connector. Let's switch > to do that, removing all of the custom code. > > NOTE: this still _doesn't_ implement DRM_BRIDGE_ATTACH_NO_CONNECTOR > support for ti-sn65dsi86 and that would still be a useful thing to do > in the future. It was attempted in the past [1] but put on the back > burner. However, unless we instantly change ti-sn65dsi86 fully from > not supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR at all to _only_ > supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR then we'll still need a bit > of time when we support both. This is a better way to support the old > way where the driver hand rolls things itself. > > A new notes about the implementation here: > * When using the drm_bridge_connector the connector should be created > after all the bridges, so we change the ordering a bit. > * I'm reasonably certain that we don't need to do anything to "free" > the new drm_bridge_connector. If drm_bridge_connector_init() returns > success then we know drm_connector_init() was called with the > `drm_bridge_connector_funcs`. The `drm_bridge_connector_funcs` has a > .destroy() that does all the cleanup. drm_connector_init() calls > __drm_mode_object_add() with a drm_connector_free() that will call > the .destroy(). > * I'm also reasonably certain that I don't need to "undo" the > drm_bridge_attach() if drm_bridge_connector_init() fails. The > "detach" function is private and other similar code doesn't try to > undo the drm_bridge_attach() in error cases. There's also a comment > indicating the lack of balance at the top of drm_bridge_attach(). > > [1] https://lore.kernel.org/r/20210920225801.227211-4-robdcl...@gmail.com > > Signed-off-by: Douglas Anderson <diand...@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > --- > > Changes in v2: > - ("ti-sn65dsi86: Use drm_bridge_connector") new for v2. > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 72 ++++++--------------------- > 1 file changed, 14 insertions(+), 58 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index ba136a188be7..38616aab12ac 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -26,6 +26,7 @@ > #include <drm/drm_atomic.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_bridge.h> > +#include <drm/drm_bridge_connector.h> > #include <drm/dp/drm_dp_aux_bus.h> > #include <drm/dp/drm_dp_helper.h> > #include <drm/drm_mipi_dsi.h> > @@ -174,7 +175,7 @@ struct ti_sn65dsi86 { > struct regmap *regmap; > struct drm_dp_aux aux; > struct drm_bridge bridge; > - struct drm_connector connector; > + struct drm_connector *connector; > struct device_node *host_node; > struct mipi_dsi_device *dsi; > struct clk *refclk; > @@ -646,54 +647,6 @@ static struct auxiliary_driver ti_sn_aux_driver = { > .id_table = ti_sn_aux_id_table, > }; > > -/* > ----------------------------------------------------------------------------- > - * DRM Connector Operations > - */ > - > -static struct ti_sn65dsi86 * > -connector_to_ti_sn65dsi86(struct drm_connector *connector) > -{ > - return container_of(connector, struct ti_sn65dsi86, connector); > -} > - > -static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) > -{ > - struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector); > - > - return drm_bridge_get_modes(pdata->next_bridge, connector); > -} > - > -static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs > = { > - .get_modes = ti_sn_bridge_connector_get_modes, > -}; > - > -static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = { > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = drm_connector_cleanup, > - .reset = drm_atomic_helper_connector_reset, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > -static int ti_sn_bridge_connector_init(struct ti_sn65dsi86 *pdata) > -{ > - int ret; > - > - ret = drm_connector_init(pdata->bridge.dev, &pdata->connector, > - &ti_sn_bridge_connector_funcs, > - DRM_MODE_CONNECTOR_eDP); > - if (ret) { > - DRM_ERROR("Failed to initialize connector with drm\n"); > - return ret; > - } > - > - drm_connector_helper_add(&pdata->connector, > - &ti_sn_bridge_connector_helper_funcs); > - drm_connector_attach_encoder(&pdata->connector, pdata->bridge.encoder); > - > - return 0; > -} > - > > /*------------------------------------------------------------------------------ > * DRM Bridge > */ > @@ -757,10 +710,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, > return ret; > } > > - ret = ti_sn_bridge_connector_init(pdata); > - if (ret < 0) > - goto err_conn_init; > - > /* We never want the next bridge to *also* create a connector: */ > flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR; > > @@ -768,13 +717,20 @@ static int ti_sn_bridge_attach(struct drm_bridge > *bridge, > ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge, > &pdata->bridge, flags); > if (ret < 0) > - goto err_dsi_host; > + goto err_initted_aux; > + > + pdata->connector = drm_bridge_connector_init(pdata->bridge.dev, > + pdata->bridge.encoder); > + if (IS_ERR(pdata->connector)) { > + ret = PTR_ERR(pdata->connector); > + goto err_initted_aux; > + } > + > + drm_connector_attach_encoder(pdata->connector, pdata->bridge.encoder); > > return 0; > > -err_dsi_host: > - drm_connector_cleanup(&pdata->connector); > -err_conn_init: > +err_initted_aux: > drm_dp_aux_unregister(&pdata->aux); > return ret; > } > @@ -824,7 +780,7 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 > *pdata) > > static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) > { > - if (pdata->connector.display_info.bpc <= 6) > + if (pdata->connector->display_info.bpc <= 6) > return 18; > else > return 24; -- Regards, Laurent Pinchart
