Message-ID: <20210302163505.2d42x364qsm26jo7@gilmour>
Date:   Tue, 2 Mar 2021 17:35:05 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Jagan Teki <jagan@...rulasolutions.com>
Cc:     Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-amarula <linux-amarula@...rulasolutions.com>
Subject: Re: [PATCH v3 6/7] drm: sun4i: dsi: Use drm_panel_bridge, connector
 API

On Fri, Feb 26, 2021 at 10:40:24PM +0530, Jagan Teki wrote:
> On Fri, Feb 26, 2021 at 10:27 PM Maxime Ripard <mripard@...nel.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 <jagan@...rulasolutions.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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists