On Fri, Feb 26, 2021 at 10:40:24PM +0530, Jagan Teki wrote: > On Fri, Feb 26, 2021 at 10:27 PM Maxime Ripard <mrip...@kernel.org> wrote: > > > > Hi, > > > > On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote: > > > Use drm_panel_bridge to replace manual panel handling code. > > > > > > This simplifies the driver to allows all components in the > > > display pipeline to be treated as bridges, paving the way > > > to generic connector handling. > > > > > > Use drm_bridge_connector_init to create a connector for display > > > pipelines that use drm_bridge. > > > > > > This allows splitting connector operations across multiple bridges > > > when necessary, instead of having the last bridge in the chain > > > creating the connector and handling all connector operations > > > internally. > > > > > > Signed-off-by: Jagan Teki <ja...@amarulasolutions.com> > > > > Most of the code removed in that patch was actually introduced earlier > > which feels a bit weird. Is there a reason we can't do that one first, > > and then introduce the bridge support? > > This patch adds new bridge API's which requires the driver has to > support the bridge first.
I'm not sure what you're saying, you can definitely have a bridge
without support for a downstream bridge.
Anyway, my point is that:
> ---
> Changes for v3:
> - new patch
>
> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 108 +++++++------------------
> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 7 --
> 2 files changed, 27 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 3cdc14daf25c..5e5d3789b3df 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -20,6 +20,7 @@
> #include <linux/slab.h>
>
> #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge_connector.h>
> #include <drm/drm_mipi_dsi.h>
> #include <drm/drm_of.h>
> #include <drm/drm_panel.h>
> @@ -769,12 +770,6 @@ static void sun6i_dsi_bridge_pre_enable(struct
> drm_bridge *bridge)
> phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
> phy_configure(dsi->dphy, &opts);
> phy_power_on(dsi->dphy);
> -
> - if (dsi->panel)
> - drm_panel_prepare(dsi->panel);
> -
> - if (dsi->panel_bridge)
> - dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge);
This is added in patch 2
> }
>
> static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge)
> @@ -793,12 +788,6 @@ static void sun6i_dsi_bridge_enable(struct drm_bridge
> *bridge)
> * ordering on the panels I've tested it with, so I guess this
> * will do for now, until that IP is better understood.
> */
> - if (dsi->panel)
> - drm_panel_enable(dsi->panel);
> -
> - if (dsi->panel_bridge)
> - dsi->panel_bridge->funcs->enable(dsi->panel_bridge);
> -
This is added in patch 2
> sun6i_dsi_start(dsi, DSI_START_HSC);
>
> udelay(1000);
> @@ -812,14 +801,6 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge
> *bridge)
>
> DRM_DEBUG_DRIVER("Disabling DSI output\n");
>
> - if (dsi->panel) {
> - drm_panel_disable(dsi->panel);
> - drm_panel_unprepare(dsi->panel);
> - } else if (dsi->panel_bridge) {
> - dsi->panel_bridge->funcs->disable(dsi->panel_bridge);
> - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
> - }
> -
This is added in patch 2
> phy_power_off(dsi->dphy);
> phy_exit(dsi->dphy);
>
> @@ -828,63 +809,13 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge
> *bridge)
> regulator_disable(dsi->regulator);
> }
>
> -static int sun6i_dsi_get_modes(struct drm_connector *connector)
> -{
> - struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
> -
> - return drm_panel_get_modes(dsi->panel, connector);
> -}
> -
> -static const struct drm_connector_helper_funcs
> sun6i_dsi_connector_helper_funcs = {
> - .get_modes = sun6i_dsi_get_modes,
> -};
> -
> -static enum drm_connector_status
> -sun6i_dsi_connector_detect(struct drm_connector *connector, bool force)
> -{
> - struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
> -
> - return dsi->panel ? connector_status_connected :
> - connector_status_disconnected;
> -}
> -
> -static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
> - .detect = sun6i_dsi_connector_detect,
> - .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 sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
> - int ret;
> -
> - if (dsi->panel_bridge)
> - return drm_bridge_attach(bridge->encoder, dsi->panel_bridge,
> NULL, 0);
This is added in patch 2
> - if (dsi->panel) {
> - drm_connector_helper_add(&dsi->connector,
> - &sun6i_dsi_connector_helper_funcs);
> - ret = drm_connector_init(bridge->dev, &dsi->connector,
> - &sun6i_dsi_connector_funcs,
> - DRM_MODE_CONNECTOR_DSI);
> - if (ret) {
> - dev_err(dsi->dev, "Couldn't initialise the DSI
> connector\n");
> - goto err_cleanup_connector;
> - }
> -
> - drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
> - }
This has been added (through a rework) in patch 3
Surely we can do better?
Maxime
signature.asc
Description: PGP signature
