drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.

The newer version of the RPi panel driver is going to be a combination of a bridge and a panel, but we should also support panels without a bridge, so the panel-bridge layer lets us do that cleanly.

v2: Drop "dev" argument.

Signed-off-by Eric Anholt <[email protected]>

Acked-by Daniel Vetter <[email protected]>

---
 drivers/gpu/drm/vc4/Kconfig   |   2 +-
 drivers/gpu/drm/vc4/vc4_dsi.c | 154 ++++++------------------------------------
 2 files changed, 21 insertions(+), 135 deletions(-)

drivers/gpu/drm/vc4/Kconfig +1 -1

index b16aefe4a8d3..4361bdcfd28a 100644
@@ -7,7 +7,7 @@ config DRM_VC4
 	select DRM_KMS_HELPER
 	select DRM_KMS_CMA_HELPER
 	select DRM_GEM_CMA_HELPER
-	select DRM_PANEL
+	select DRM_PANEL_BRIDGE
 	select SND_PCM
 	select SND_PCM_ELD
 	select SND_SOC_GENERIC_DMAENGINE_PCM

drivers/gpu/drm/vc4/vc4_dsi.c +19 -133

index deba62008fd0..fb54a9d10360 100644
@@ -503,8 +503,8 @@ struct vc4_dsi {
 
 	struct mipi_dsi_host dsi_host;
 	struct drm_encoder *encoder;
-	struct drm_connector *connector;
-	struct drm_panel *panel;
+	struct drm_bridge *bridge;
+	bool is_panel_bridge;
 
 	void __iomem *regs;
 

@@ -604,18 +604,6 @@ to_vc4_dsi_encoder(struct drm_encoder *encoder)
 	return container_of(encoder, struct vc4_dsi_encoder, base.base);
 }
 
-/* VC4 DSI connector KMS struct */
-struct vc4_dsi_connector {
-	struct drm_connector base;
-	struct vc4_dsi *dsi;
-};
-
-static inline struct vc4_dsi_connector *
-to_vc4_dsi_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct vc4_dsi_connector, base);
-}
-
 #define DSI_REG(reg) { reg, #reg }
 static const struct {
 	u32 reg;

@@ -723,79 +711,6 @@ int vc4_dsi_debugfs_regs(struct seq_file *m, void *unused)
 }
 #endif
 
-static enum drm_connector_status
-vc4_dsi_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct vc4_dsi_connector *vc4_connector =
-		to_vc4_dsi_connector(connector);
-	struct vc4_dsi *dsi = vc4_connector->dsi;
-
-	if (dsi->panel)
-		return connector_status_connected;
-	else
-		return connector_status_disconnected;
-}
-
-static void vc4_dsi_connector_destroy(struct drm_connector *connector)
-{
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-}
-
-static int vc4_dsi_connector_get_modes(struct drm_connector *connector)
-{
-	struct vc4_dsi_connector *vc4_connector =
-		to_vc4_dsi_connector(connector);
-	struct vc4_dsi *dsi = vc4_connector->dsi;
-
-	if (dsi->panel)
-		return drm_panel_get_modes(dsi->panel);
-
-	return 0;
-}
-
-static const struct drm_connector_funcs vc4_dsi_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.detect = vc4_dsi_connector_detect,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = vc4_dsi_connector_destroy,
-	.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 const struct drm_connector_helper_funcs vc4_dsi_connector_helper_funcs = {
-	.get_modes = vc4_dsi_connector_get_modes,
-};
-
-static struct drm_connector *vc4_dsi_connector_init(struct drm_device *dev,
-						    struct vc4_dsi *dsi)
-{
-	struct drm_connector *connector;
-	struct vc4_dsi_connector *dsi_connector;
-
-	dsi_connector = devm_kzalloc(dev->dev, sizeof(*dsi_connector),
-				     GFP_KERNEL);
-	if (!dsi_connector)
-		return ERR_PTR(-ENOMEM);
-
-	connector = &dsi_connector->base;
-
-	dsi_connector->dsi = dsi;
-
-	drm_connector_init(dev, connector, &vc4_dsi_connector_funcs,
-			   DRM_MODE_CONNECTOR_DSI);
-	drm_connector_helper_add(connector, &vc4_dsi_connector_helper_funcs);
-
-	connector->polled = 0;
-	connector->interlace_allowed = 0;
-	connector->doublescan_allowed = 0;
-
-	drm_mode_connector_attach_encoder(connector, dsi->encoder);
-
-	return connector;
-}
-
 static void vc4_dsi_encoder_destroy(struct drm_encoder *encoder)
 {
 	drm_encoder_cleanup(encoder);

@@ -893,12 +808,8 @@ static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
 	struct vc4_dsi *dsi = vc4_encoder->dsi;
 	struct device *dev = &dsi->pdev->dev;
 
-	drm_panel_disable(dsi->panel);
-
 	vc4_dsi_ulps(dsi, true);
 
-	drm_panel_unprepare(dsi->panel);
-
 	clk_disable_unprepare(dsi->pll_phy_clock);
 	clk_disable_unprepare(dsi->escape_clock);
 	clk_disable_unprepare(dsi->pixel_clock);

@@ -929,12 +840,6 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 		return;
 	}
 
-	ret = drm_panel_prepare(dsi->panel);
-	if (ret) {
-		DRM_ERROR("Panel failed to prepare\n");
-		return;
-	}
-
 	if (debug_dump_regs) {
 		DRM_INFO("DSI regs before:\n");
 		vc4_dsi_dump_regs(dsi);

@@ -1184,13 +1089,6 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 		DRM_INFO("DSI regs after:\n");
 		vc4_dsi_dump_regs(dsi);
 	}
-
-	ret = drm_panel_enable(dsi->panel);
-	if (ret) {
-		DRM_ERROR("Panel failed to enable\n");
-		drm_panel_unprepare(dsi->panel);
-		return;
-	}
 }
 
 static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,

@@ -1366,17 +1264,22 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
 		return 0;
 	}
 
-	dsi->panel = of_drm_find_panel(device->dev.of_node);
-	if (!dsi->panel)
-		return 0;
-
-	ret = drm_panel_attach(dsi->panel, dsi->connector);
-	if (ret != 0)
-		return ret;
+	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
+	if (!dsi->bridge) {
+		struct drm_panel *panel =
+			of_drm_find_panel(device->dev.of_node);
 
-	drm_helper_hpd_irq_event(dsi->connector->dev);
+		dsi->bridge = drm_panel_bridge_add(panel,
+						   DRM_MODE_CONNECTOR_DSI);
+		if (IS_ERR(dsi->bridge)) {
+			ret = PTR_ERR(dsi->bridge);
+			dsi->bridge = NULL;
+			return ret;
+		}
+		dsi->is_panel_bridge = true;
+	}
 
-	return 0;
+	return drm_bridge_attach(dsi->encoder, dsi->bridge, NULL);
 }
 
 static int vc4_dsi_host_detach(struct mipi_dsi_host *host,

@@ -1384,15 +1287,9 @@ static int vc4_dsi_host_detach(struct mipi_dsi_host *host,
 {
 	struct vc4_dsi *dsi = host_to_dsi(host);
 
-	if (dsi->panel) {
-		int ret = drm_panel_detach(dsi->panel);
-
-		if (ret)
-			return ret;
-
-		dsi->panel = NULL;
-
-		drm_helper_hpd_irq_event(dsi->connector->dev);
+	if (dsi->is_panel_bridge) {
+		drm_panel_bridge_remove(dsi->bridge);
+		dsi->bridge = NULL;
 	}
 
 	return 0;

@@ -1658,12 +1555,6 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 			 DRM_MODE_ENCODER_DSI, NULL);
 	drm_encoder_helper_add(dsi->encoder, &vc4_dsi_encoder_helper_funcs);
 
-	dsi->connector = vc4_dsi_connector_init(drm, dsi);
-	if (IS_ERR(dsi->connector)) {
-		ret = PTR_ERR(dsi->connector);
-		goto err_destroy_encoder;
-	}
-
 	dsi->dsi_host.ops = &vc4_dsi_host_ops;
 	dsi->dsi_host.dev = dev;
 

@@ -1674,11 +1565,6 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 	pm_runtime_enable(dev);
 
 	return 0;
-
-err_destroy_encoder:
-	vc4_dsi_encoder_destroy(dsi->encoder);
-
-	return ret;
 }
 
 static void vc4_dsi_unbind(struct device *dev, struct device *master,

Another 100 lines of boilerplate gone, while allowing for bridges to be connected in the display chain.

Signed-off-by Eric Anholt <[email protected]>

---
 drivers/gpu/drm/vc4/vc4_dpi.c | 164 ++++++++----------------------------------
 1 file changed, 30 insertions(+), 134 deletions(-)

drivers/gpu/drm/vc4/vc4_dpi.c +28 -131

index c6d703903fd9..98532e2cd2b7 100644
@@ -23,8 +23,10 @@
  */
 
 #include "drm_atomic_helper.h"
+#include "drm_bridge.h"
 #include "drm_crtc_helper.h"
 #include "drm_edid.h"
+#include "drm_of.h"
 #include "drm_panel.h"
 #include "linux/clk.h"
 #include "linux/component.h"

@@ -95,7 +97,8 @@ struct vc4_dpi {
 
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
-	struct drm_panel *panel;
+	struct drm_bridge *bridge;
+	bool is_panel_bridge;
 
 	void __iomem *regs;
 

@@ -118,24 +121,6 @@ to_vc4_dpi_encoder(struct drm_encoder *encoder)
 	return container_of(encoder, struct vc4_dpi_encoder, base.base);
 }
 
-/* VC4 DPI connector KMS struct */
-struct vc4_dpi_connector {
-	struct drm_connector base;
-	struct vc4_dpi *dpi;
-
-	/* Since the connector is attached to just the one encoder,
-	 * this is the reference to it so we can do the best_encoder()
-	 * hook.
-	 */
-	struct drm_encoder *encoder;
-};
-
-static inline struct vc4_dpi_connector *
-to_vc4_dpi_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct vc4_dpi_connector, base);
-}
-
 #define DPI_REG(reg) { reg, #reg }
 static const struct {
 	u32 reg;

@@ -167,80 +152,6 @@ int vc4_dpi_debugfs_regs(struct seq_file *m, void *unused)
 }
 #endif
 
-static enum drm_connector_status
-vc4_dpi_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct vc4_dpi_connector *vc4_connector =
-		to_vc4_dpi_connector(connector);
-	struct vc4_dpi *dpi = vc4_connector->dpi;
-
-	if (dpi->panel)
-		return connector_status_connected;
-	else
-		return connector_status_disconnected;
-}
-
-static void vc4_dpi_connector_destroy(struct drm_connector *connector)
-{
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-}
-
-static int vc4_dpi_connector_get_modes(struct drm_connector *connector)
-{
-	struct vc4_dpi_connector *vc4_connector =
-		to_vc4_dpi_connector(connector);
-	struct vc4_dpi *dpi = vc4_connector->dpi;
-
-	if (dpi->panel)
-		return drm_panel_get_modes(dpi->panel);
-
-	return 0;
-}
-
-static const struct drm_connector_funcs vc4_dpi_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.detect = vc4_dpi_connector_detect,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = vc4_dpi_connector_destroy,
-	.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 const struct drm_connector_helper_funcs vc4_dpi_connector_helper_funcs = {
-	.get_modes = vc4_dpi_connector_get_modes,
-};
-
-static struct drm_connector *vc4_dpi_connector_init(struct drm_device *dev,
-						    struct vc4_dpi *dpi)
-{
-	struct drm_connector *connector = NULL;
-	struct vc4_dpi_connector *dpi_connector;
-
-	dpi_connector = devm_kzalloc(dev->dev, sizeof(*dpi_connector),
-				     GFP_KERNEL);
-	if (!dpi_connector)
-		return ERR_PTR(-ENOMEM);
-
-	connector = &dpi_connector->base;
-
-	dpi_connector->encoder = dpi->encoder;
-	dpi_connector->dpi = dpi;
-
-	drm_connector_init(dev, connector, &vc4_dpi_connector_funcs,
-			   DRM_MODE_CONNECTOR_DPI);
-	drm_connector_helper_add(connector, &vc4_dpi_connector_helper_funcs);
-
-	connector->polled = 0;
-	connector->interlace_allowed = 0;
-	connector->doublescan_allowed = 0;
-
-	drm_mode_connector_attach_encoder(connector, dpi->encoder);
-
-	return connector;
-}
-
 static const struct drm_encoder_funcs vc4_dpi_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };

@@ -250,11 +161,7 @@ static void vc4_dpi_encoder_disable(struct drm_encoder *encoder)
 	struct vc4_dpi_encoder *vc4_encoder = to_vc4_dpi_encoder(encoder);
 	struct vc4_dpi *dpi = vc4_encoder->dpi;
 
-	drm_panel_disable(dpi->panel);
-
 	clk_disable_unprepare(dpi->pixel_clock);
-
-	drm_panel_unprepare(dpi->panel);
 }
 
 static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)

@@ -265,12 +172,6 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)
 	u32 dpi_c = DPI_ENABLE | DPI_OUTPUT_ENABLE_MODE;
 	int ret;
 
-	ret = drm_panel_prepare(dpi->panel);
-	if (ret) {
-		DRM_ERROR("Panel failed to prepare\n");
-		return;
-	}
-
 	if (dpi->connector->display_info.num_bus_formats) {
 		u32 bus_format = dpi->connector->display_info.bus_formats[0];
 

@@ -321,13 +222,6 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)
 	ret = clk_prepare_enable(dpi->pixel_clock);
 	if (ret)
 		DRM_ERROR("Failed to set clock rate: %d\n", ret);
-
-	ret = drm_panel_enable(dpi->panel);
-	if (ret) {
-		DRM_ERROR("Panel failed to enable\n");
-		drm_panel_unprepare(dpi->panel);
-		return;
-	}
 }
 
 static bool vc4_dpi_encoder_mode_fixup(struct drm_encoder *encoder,

@@ -351,24 +245,34 @@ static const struct of_device_id vc4_dpi_dt_match[] = {
 	{}
 };
 
-/* Walks the OF graph to find the panel node and then asks DRM to look
- * up the panel.
+/* Sets up the next link in the display chain, whether it's a panel or
+ * a bridge.
  */
-static struct drm_panel *vc4_dpi_get_panel(struct device *dev)
+static int vc4_dpi_init_bridge(struct vc4_dpi *dpi)
 {
-	struct device_node *panel_node;
-	struct device_node *np = dev->of_node;
+	struct device *dev = &dpi->pdev->dev;
 	struct drm_panel *panel;
+	int ret;
 
-	/* don't proceed if we have an endpoint but no panel_node tied to it */
-	panel_node = of_graph_get_remote_node(np, 0, 0);
-	if (!panel_node)
-		return NULL;
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
+					  &panel, &dpi->bridge);
+	if (ret) {
+		/* If nothing was connected in the DT, that's not an
+		 * error.
+		 */
+		if (ret == -ENODEV)
+			return 0;
+		else
+			return ret;
+	}
 
-	panel = of_drm_find_panel(panel_node);
-	of_node_put(panel_node);
+	if (panel) {
+		dpi->bridge = drm_panel_bridge_add(panel,
+						   DRM_MODE_CONNECTOR_DPI);
+		dpi->is_panel_bridge = true;
+	}
 
-	return panel;
+	return drm_bridge_attach(dpi->encoder, dpi->bridge, NULL);
 }
 
 static int vc4_dpi_bind(struct device *dev, struct device *master, void *data)

@@ -422,20 +326,13 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		DRM_ERROR("Failed to turn on core clock: %d\n", ret);
 
-	dpi->panel = vc4_dpi_get_panel(dev);
-
 	drm_encoder_init(drm, dpi->encoder, &vc4_dpi_encoder_funcs,
 			 DRM_MODE_ENCODER_DPI, NULL);
 	drm_encoder_helper_add(dpi->encoder, &vc4_dpi_encoder_helper_funcs);
 
-	dpi->connector = vc4_dpi_connector_init(drm, dpi);
-	if (IS_ERR(dpi->connector)) {
-		ret = PTR_ERR(dpi->connector);
+	ret = vc4_dpi_init_bridge(dpi);
+	if (ret)
 		goto err_destroy_encoder;
-	}
-
-	if (dpi->panel)
-		drm_panel_attach(dpi->panel, dpi->connector);
 
 	dev_set_drvdata(dev, dpi);
 

@@ -456,10 +353,9 @@ static void vc4_dpi_unbind(struct device *dev, struct device *master,
 	struct vc4_dev *vc4 = to_vc4_dev(drm);
 	struct vc4_dpi *dpi = dev_get_drvdata(dev);

Avoids a bunch of connector boilerplate. Note that this causes panel prepare() to be moved before mtk_dsi_poweron() and unprepare() to be after poweroff(). I think this is the expected usage of the panel API (enable should be when you do things that require the link to be brought up), but there may be issues here.

Signed-off-by Eric Anholt <[email protected]>

---

Note that I haven't tested this change, and am not committed to this patch. It's just an optional cleanup, if it works for you.

 drivers/gpu/drm/mediatek/mtk_dsi.c | 125 ++++---------------------------------
 1 file changed, 13 insertions(+), 112 deletions(-)

drivers/gpu/drm/mediatek/mtk_dsi.c +6 -112

index 808b995a990f..764bd8b9c256 100644
@@ -160,7 +160,6 @@ struct mtk_dsi {
 	struct device *dev;
 	struct mipi_dsi_host host;
 	struct drm_encoder encoder;
-	struct drm_connector conn;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 	struct phy *phy;

@@ -188,11 +187,6 @@ static inline struct mtk_dsi *encoder_to_dsi(struct drm_encoder *e)
 	return container_of(e, struct mtk_dsi, encoder);
 }
 
-static inline struct mtk_dsi *connector_to_dsi(struct drm_connector *c)
-{
-	return container_of(c, struct mtk_dsi, conn);
-}
-
 static inline struct mtk_dsi *host_to_dsi(struct mipi_dsi_host *h)
 {
 	return container_of(h, struct mtk_dsi, host);

@@ -603,16 +597,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
 	mtk_dsi_lane0_ulp_mode_leave(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 0);
 
-	if (dsi->panel) {
-		if (drm_panel_prepare(dsi->panel)) {
-			DRM_ERROR("failed to prepare the panel\n");
-			goto err_disable_digital_clk;
-		}
-	}
-
 	return 0;
-err_disable_digital_clk:
-	clk_disable_unprepare(dsi->digital_clk);
 err_disable_engine_clk:
 	clk_disable_unprepare(dsi->engine_clk);
 err_phy_power_off:

@@ -630,15 +615,7 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
 	if (--dsi->refcount != 0)
 		return;
 
-	if (!mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500)) {
-		if (dsi->panel) {
-			if (drm_panel_unprepare(dsi->panel)) {
-				DRM_ERROR("failed to unprepare the panel\n");
-				return;
-			}
-		}
-	}
-
+	mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
 	mtk_dsi_reset_engine(dsi);
 	mtk_dsi_lane0_ulp_mode_enter(dsi);
 	mtk_dsi_clk_ulp_mode_enter(dsi);

@@ -669,19 +646,9 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 
 	mtk_dsi_start(dsi);
 
-	if (dsi->panel) {
-		if (drm_panel_enable(dsi->panel)) {
-			DRM_ERROR("failed to enable the panel\n");
-			goto err_dsi_power_off;
-		}
-	}
-
 	dsi->enabled = true;
 
 	return;
-err_dsi_power_off:
-	mtk_dsi_stop(dsi);
-	mtk_dsi_poweroff(dsi);
 }
 
 static void mtk_output_dsi_disable(struct mtk_dsi *dsi)

@@ -689,13 +656,6 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi)
 	if (!dsi->enabled)
 		return;
 
-	if (dsi->panel) {
-		if (drm_panel_disable(dsi->panel)) {
-			DRM_ERROR("failed to disable the panel\n");
-			return;
-		}
-	}
-
 	mtk_dsi_stop(dsi);
 	mtk_dsi_poweroff(dsi);
 

@@ -750,13 +710,6 @@ static void mtk_dsi_encoder_enable(struct drm_encoder *encoder)
 	mtk_output_dsi_enable(dsi);
 }
 
-static int mtk_dsi_connector_get_modes(struct drm_connector *connector)
-{
-	struct mtk_dsi *dsi = connector_to_dsi(connector);
-
-	return drm_panel_get_modes(dsi->panel);
-}
-
 static const struct drm_encoder_helper_funcs mtk_dsi_encoder_helper_funcs = {
 	.mode_fixup = mtk_dsi_encoder_mode_fixup,
 	.mode_set = mtk_dsi_encoder_mode_set,

@@ -764,52 +717,7 @@ static const struct drm_encoder_helper_funcs mtk_dsi_encoder_helper_funcs = {
 	.enable = mtk_dsi_encoder_enable,
 };
 
-static const struct drm_connector_funcs mtk_dsi_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.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 const struct drm_connector_helper_funcs
-	mtk_dsi_connector_helper_funcs = {
-	.get_modes = mtk_dsi_connector_get_modes,
-};
-
-static int mtk_dsi_create_connector(struct drm_device *drm, struct mtk_dsi *dsi)
-{
-	int ret;
-
-	ret = drm_connector_init(drm, &dsi->conn, &mtk_dsi_connector_funcs,
-				 DRM_MODE_CONNECTOR_DSI);
-	if (ret) {
-		DRM_ERROR("Failed to connector init to drm\n");
-		return ret;
-	}
-
-	drm_connector_helper_add(&dsi->conn, &mtk_dsi_connector_helper_funcs);
-
-	dsi->conn.dpms = DRM_MODE_DPMS_OFF;
-	drm_mode_connector_attach_encoder(&dsi->conn, &dsi->encoder);
-
-	if (dsi->panel) {
-		ret = drm_panel_attach(dsi->panel, &dsi->conn);
-		if (ret) {
-			DRM_ERROR("Failed to attach panel to drm\n");
-			goto err_connector_cleanup;
-		}
-	}
-
-	return 0;
-
-err_connector_cleanup:
-	drm_connector_cleanup(&dsi->conn);
-	return ret;
-}
-
-static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi)
+static int mtk_dsi_create_enc(struct drm_device *drm, struct mtk_dsi *dsi)
 {
 	int ret;
 

@@ -827,15 +735,10 @@ static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi)
 	 */
 	dsi->encoder.possible_crtcs = 1;
 
-	/* If there's a bridge, attach to it and let it create the connector */
 	ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL);
 	if (ret) {
 		DRM_ERROR("Failed to attach bridge to drm\n");
-
-		/* Otherwise create our own connector and attach to a panel */
-		ret = mtk_dsi_create_connector(drm, dsi);
-		if (ret)
-			goto err_encoder_cleanup;
+		goto err_encoder_cleanup;
 	}
 
 	return 0;

@@ -848,9 +751,8 @@ static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi)
 static void mtk_dsi_destroy_conn_enc(struct mtk_dsi *dsi)
 {
 	drm_encoder_cleanup(&dsi->encoder);
-	/* Skip connector cleanup if creation was delegated to the bridge */
-	if (dsi->conn.dev)
-		drm_connector_cleanup(&dsi->conn);
+	if (dsi->panel)
+		drm_panel_bridge_remove(dsi->bridge);
 }
 
 static void mtk_dsi_ddp_start(struct mtk_ddp_comp *comp)

@@ -881,20 +783,12 @@ static int mtk_dsi_host_attach(struct mipi_dsi_host *host,
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
 
-	if (dsi->conn.dev)
-		drm_helper_hpd_irq_event(dsi->conn.dev);
-
 	return 0;
 }
 
 static int mtk_dsi_host_detach(struct mipi_dsi_host *host,
 			       struct mipi_dsi_device *device)
 {
-	struct mtk_dsi *dsi = host_to_dsi(host);
-
-	if (dsi->conn.dev)
-		drm_helper_hpd_irq_event(dsi->conn.dev);
-
 	return 0;
 }
 

@@ -1062,7 +956,7 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
 		goto err_ddp_comp_unregister;
 	}
 
-	ret = mtk_dsi_create_conn_enc(drm, dsi);
+	ret = mtk_dsi_create_enc(drm, dsi);
 	if (ret) {
 		DRM_ERROR("Encoder create failed with %d\n", ret);
 		goto err_unregister;

@@ -1114,6 +1008,13 @@ static int mtk_dsi_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 

Another 100 lines of boilerplate gone. Bridges aren't supported yet, but will be trivial to add later.

Signed-off-by Eric Anholt <[email protected]>

---

drivers/gpu/drm/stm/ltdc.c +0 -7

-
-		connector = ltdc_rgb_connector_create(ddev);
-		if (!connector) {
-			DRM_ERROR("Failed to create RGB connector\n");
-			ret = -EINVAL;
-			goto err;
-		}

Another 100 lines of boilerplate gone. Bridges aren't supported yet, but will be trivial to add later.

Signed-off-by Eric Anholt <[email protected]>

---

Also untested.

  drivers/gpu/drm/stm/ltdc.c | 128 +++++----------------------------------------
  drivers/gpu/drm/stm/ltdc.h |   2 +-
  2 files changed, 13 insertions(+), 117 deletions(-)

drivers/gpu/drm/stm/ltdc.c +12 -116

index a40418cda74a..41a1c5d68f5b 100644
@@ -269,11 +269,6 @@ static inline struct ltdc_device *encoder_to_ltdc(struct drm_encoder *enc)
  	return (struct ltdc_device *)enc->dev->dev_private;
  }
 
-static inline struct ltdc_device *connector_to_ltdc(struct drm_connector *con)
-{
-	return (struct ltdc_device *)con->dev->dev_private;
-}
-
  static inline enum ltdc_pix_fmt to_ltdc_pixelformat(u32 drm_fmt)
  {
  	enum ltdc_pix_fmt pf;

@@ -815,22 +810,12 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
 
  static void ltdc_rgb_encoder_enable(struct drm_encoder *encoder)
  {
-	struct ltdc_device *ldev = encoder_to_ltdc(encoder);
-
  	DRM_DEBUG_DRIVER("\n");
-
-	drm_panel_prepare(ldev->panel);
-	drm_panel_enable(ldev->panel);
  }
 
  static void ltdc_rgb_encoder_disable(struct drm_encoder *encoder)
  {
-	struct ltdc_device *ldev = encoder_to_ltdc(encoder);
-
  	DRM_DEBUG_DRIVER("\n");
-
-	drm_panel_disable(ldev->panel);
-	drm_panel_unprepare(ldev->panel);
  }
 
  static const struct drm_encoder_helper_funcs ltdc_rgb_encoder_helper_funcs = {

@@ -863,82 +848,6 @@ static struct drm_encoder *ltdc_rgb_encoder_create(struct drm_device *ddev)
  	return encoder;
  }
 
-/*
- * DRM_CONNECTOR
- */
-
-static int ltdc_rgb_connector_get_modes(struct drm_connector *connector)
-{
-	struct drm_device *ddev = connector->dev;
-	struct ltdc_device *ldev = ddev->dev_private;
-	int ret = 0;
-
-	DRM_DEBUG_DRIVER("\n");
-
-	if (ldev->panel)
-		ret = drm_panel_get_modes(ldev->panel);
-
-	return ret < 0 ? 0 : ret;
-}
-
-static struct drm_connector_helper_funcs ltdc_rgb_connector_helper_funcs = {
-	.get_modes = ltdc_rgb_connector_get_modes,
-};
-
-static enum drm_connector_status
-ltdc_rgb_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct ltdc_device *ldev = connector_to_ltdc(connector);
-
-	return ldev->panel ? connector_status_connected :
-	       connector_status_disconnected;
-}
-
-static void ltdc_rgb_connector_destroy(struct drm_connector *connector)
-{
-	DRM_DEBUG_DRIVER("\n");
-
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-}
-
-static const struct drm_connector_funcs ltdc_rgb_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.detect = ltdc_rgb_connector_detect,
-	.destroy = ltdc_rgb_connector_destroy,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-struct drm_connector *ltdc_rgb_connector_create(struct drm_device *ddev)
-{
-	struct drm_connector *connector;
-	int err;
-
-	connector = devm_kzalloc(ddev->dev, sizeof(*connector), GFP_KERNEL);
-	if (!connector) {
-		DRM_ERROR("Failed to allocate connector\n");
-		return NULL;
-	}
-
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
-
-	err = drm_connector_init(ddev, connector, &ltdc_rgb_connector_funcs,
-				 DRM_MODE_CONNECTOR_DPI);
-	if (err) {
-		DRM_ERROR("Failed to initialize connector\n");
-		return NULL;
-	}
-
-	drm_connector_helper_add(connector, &ltdc_rgb_connector_helper_funcs);
-
-	DRM_DEBUG_DRIVER("RGB connector:%d created\n", connector->base.id);
-
-	return connector;
-}
-
  static int ltdc_get_caps(struct drm_device *ddev)
  {
  	struct ltdc_device *ldev = ddev->dev_private;

@@ -972,7 +881,7 @@ static int ltdc_get_caps(struct drm_device *ddev)
  	return 0;
  }
 
-static struct drm_panel *ltdc_get_panel(struct drm_device *ddev)
+static struct drm_bridge *ltdc_get_bridge(struct drm_device *ddev)
  {
  	struct device *dev = ddev->dev;
  	struct device_node *np = dev->of_node;

@@ -1004,7 +913,10 @@ static struct drm_panel *ltdc_get_panel(struct drm_device *ddev)
  		}
  	}
 
-	return panel;
+	if (!panel)
+		return ERR_PTR(-ENODEV);
+
+	return drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DPI);
  }
 
  int ltdc_load(struct drm_device *ddev)

@@ -1014,7 +926,6 @@ int ltdc_load(struct drm_device *ddev)
  	struct device *dev = ddev->dev;
  	struct device_node *np = dev->of_node;
  	struct drm_encoder *encoder;
-	struct drm_connector *connector = NULL;
  	struct drm_crtc *crtc;
  	struct reset_control *rstc;
  	struct resource res;

@@ -1022,8 +933,8 @@ int ltdc_load(struct drm_device *ddev)
 
  	DRM_DEBUG_DRIVER("\n");
 
-	ldev->panel = ltdc_get_panel(ddev);
-	if (!ldev->panel)
+	ldev->bridge = ltdc_get_bridge(ddev);
+	if (!ldev->bridge)
  		return -EPROBE_DEFER;
 
  	rstc = of_reset_control_get(np, NULL);

@@ -1082,28 +993,13 @@ int ltdc_load(struct drm_device *ddev)
 
  	DRM_INFO("ltdc hw version 0x%08x - ready\n", ldev->caps.hw_version);
 
-	if (ldev->panel) {
+	if (ldev->bridge) {
  		encoder = ltdc_rgb_encoder_create(ddev);
  		if (!encoder) {
  			DRM_ERROR("Failed to create RGB encoder\n");
  			ret = -EINVAL;
  			goto err;
  		}
-
-		connector = ltdc_rgb_connector_create(ddev);
-		if (!connector) {
-			DRM_ERROR("Failed to create RGB connector\n");
-			ret = -EINVAL;
-			goto err;
-		}
-
-		ret = drm_mode_connector_attach_encoder(connector, encoder);
-		if (ret) {
-			DRM_ERROR("Failed to attach connector to encoder\n");
-			goto err;
-		}
-
-		drm_panel_attach(ldev->panel, connector);
  	}
 
  	crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);

@@ -1130,8 +1026,8 @@ int ltdc_load(struct drm_device *ddev)
 
  	return 0;
  err:
-	if (ldev->panel)
-		drm_panel_detach(ldev->panel);
+	if (ldev->bridge)
+		drm_panel_bridge_remove(ldev->bridge);
 
  	clk_disable_unprepare(ldev->pixel_clk);
 

@@ -1146,8 +1042,8 @@ void ltdc_unload(struct drm_device *ddev)
 
  	drm_vblank_cleanup(ddev);
 
-	if (ldev->panel)
-		drm_panel_detach(ldev->panel);
+	if (ldev->bridge)
+		drm_panel_bridge_remove(ldev->bridge);
 
  	clk_disable_unprepare(ldev->pixel_clk);
  }

drivers/gpu/drm/stm/ltdc.h +1 -1

index d7a9c736ac1e..d78cb0dd3200 100644
@@ -24,7 +24,7 @@ struct ltdc_device {
  	struct drm_fbdev_cma *fbdev;
  	void __iomem *regs;
  	struct clk *pixel_clk;	/* lcd pixel clock */
-	struct drm_panel *panel;
+	struct drm_bridge *bridge;
  	struct mutex err_lock;	/* protecting error_status */
  	struct ltdc_caps caps;
  	u32 clut[256];		/* color look up table */

Hi Eric,

I took your patch for the panel-bridge and it works perfectly in both DPI mode (panel RGB //) and DSI mode (bridge dw mipi dsi), bravo :-)

~160 lines have been removed from ltdc.c thanks to your panel-bridge code!

Many thanks
Philippe

Quoted 240 lines · Eric Anholt

Another 100 lines of boilerplate gone. Bridges aren't supported yet, but will be trivial to add later.

Signed-off-by Eric Anholt <[email protected]>

---

Also untested.

  drivers/gpu/drm/stm/ltdc.c | 128 +++++----------------------------------------
  drivers/gpu/drm/stm/ltdc.h |   2 +-
  2 files changed, 13 insertions(+), 117 deletions(-)

drivers/gpu/drm/stm/ltdc.c +12 -116

index a40418cda74a..41a1c5d68f5b 100644
@@ -269,11 +269,6 @@ static inline struct ltdc_device *encoder_to_ltdc(struct drm_encoder *enc)
  	return (struct ltdc_device *)enc->dev->dev_private;
  }
 
-static inline struct ltdc_device *connector_to_ltdc(struct drm_connector *con)
-{
-	return (struct ltdc_device *)con->dev->dev_private;
-}
-
  static inline enum ltdc_pix_fmt to_ltdc_pixelformat(u32 drm_fmt)
  {
  	enum ltdc_pix_fmt pf;

@@ -815,22 +810,12 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
 
  static void ltdc_rgb_encoder_enable(struct drm_encoder *encoder)
  {
-	struct ltdc_device *ldev = encoder_to_ltdc(encoder);
-
  	DRM_DEBUG_DRIVER("\n");
-
-	drm_panel_prepare(ldev->panel);
-	drm_panel_enable(ldev->panel);
  }
 
  static void ltdc_rgb_encoder_disable(struct drm_encoder *encoder)
  {
-	struct ltdc_device *ldev = encoder_to_ltdc(encoder);
-
  	DRM_DEBUG_DRIVER("\n");
-
-	drm_panel_disable(ldev->panel);
-	drm_panel_unprepare(ldev->panel);
  }
 
  static const struct drm_encoder_helper_funcs ltdc_rgb_encoder_helper_funcs = {

@@ -863,82 +848,6 @@ static struct drm_encoder *ltdc_rgb_encoder_create(struct drm_device *ddev)
  	return encoder;
  }
 
-/*
- * DRM_CONNECTOR
- */
-
-static int ltdc_rgb_connector_get_modes(struct drm_connector *connector)
-{
-	struct drm_device *ddev = connector->dev;
-	struct ltdc_device *ldev = ddev->dev_private;
-	int ret = 0;
-
-	DRM_DEBUG_DRIVER("\n");
-
-	if (ldev->panel)
-		ret = drm_panel_get_modes(ldev->panel);
-
-	return ret < 0 ? 0 : ret;
-}
-
-static struct drm_connector_helper_funcs ltdc_rgb_connector_helper_funcs = {
-	.get_modes = ltdc_rgb_connector_get_modes,
-};
-
-static enum drm_connector_status
-ltdc_rgb_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct ltdc_device *ldev = connector_to_ltdc(connector);
-
-	return ldev->panel ? connector_status_connected :
-	       connector_status_disconnected;
-}
-
-static void ltdc_rgb_connector_destroy(struct drm_connector *connector)
-{
-	DRM_DEBUG_DRIVER("\n");
-
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-}
-
-static const struct drm_connector_funcs ltdc_rgb_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.detect = ltdc_rgb_connector_detect,
-	.destroy = ltdc_rgb_connector_destroy,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-struct drm_connector *ltdc_rgb_connector_create(struct drm_device *ddev)
-{
-	struct drm_connector *connector;
-	int err;
-
-	connector = devm_kzalloc(ddev->dev, sizeof(*connector), GFP_KERNEL);
-	if (!connector) {
-		DRM_ERROR("Failed to allocate connector\n");
-		return NULL;
-	}
-
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
-
-	err = drm_connector_init(ddev, connector, &ltdc_rgb_connector_funcs,
-				 DRM_MODE_CONNECTOR_DPI);
-	if (err) {
-		DRM_ERROR("Failed to initialize connector\n");
-		return NULL;
-	}
-
-	drm_connector_helper_add(connector, &ltdc_rgb_connector_helper_funcs);
-
-	DRM_DEBUG_DRIVER("RGB connector:%d created\n", connector->base.id);
-
-	return connector;
-}
-
  static int ltdc_get_caps(struct drm_device *ddev)
  {
  	struct ltdc_device *ldev = ddev->dev_private;

@@ -972,7 +881,7 @@ static int ltdc_get_caps(struct drm_device *ddev)
  	return 0;
  }
 
-static struct drm_panel *ltdc_get_panel(struct drm_device *ddev)
+static struct drm_bridge *ltdc_get_bridge(struct drm_device *ddev)
  {
  	struct device *dev = ddev->dev;
  	struct device_node *np = dev->of_node;

@@ -1004,7 +913,10 @@ static struct drm_panel *ltdc_get_panel(struct drm_device *ddev)
  		}
  	}
 
-	return panel;
+	if (!panel)
+		return ERR_PTR(-ENODEV);
+
+	return drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DPI);
  }
 
  int ltdc_load(struct drm_device *ddev)

@@ -1014,7 +926,6 @@ int ltdc_load(struct drm_device *ddev)
  	struct device *dev = ddev->dev;
  	struct device_node *np = dev->of_node;
  	struct drm_encoder *encoder;
-	struct drm_connector *connector = NULL;
  	struct drm_crtc *crtc;
  	struct reset_control *rstc;
  	struct resource res;

@@ -1022,8 +933,8 @@ int ltdc_load(struct drm_device *ddev)
 
  	DRM_DEBUG_DRIVER("\n");
 
-	ldev->panel = ltdc_get_panel(ddev);
-	if (!ldev->panel)
+	ldev->bridge = ltdc_get_bridge(ddev);
+	if (!ldev->bridge)
  		return -EPROBE_DEFER;
 
  	rstc = of_reset_control_get(np, NULL);

@@ -1082,28 +993,13 @@ int ltdc_load(struct drm_device *ddev)
 
  	DRM_INFO("ltdc hw version 0x%08x - ready\n", ldev->caps.hw_version);
 
-	if (ldev->panel) {
+	if (ldev->bridge) {
  		encoder = ltdc_rgb_encoder_create(ddev);
  		if (!encoder) {
  			DRM_ERROR("Failed to create RGB encoder\n");
  			ret = -EINVAL;
  			goto err;
  		}
-
-		connector = ltdc_rgb_connector_create(ddev);
-		if (!connector) {
-			DRM_ERROR("Failed to create RGB connector\n");
-			ret = -EINVAL;
-			goto err;
-		}
-
-		ret = drm_mode_connector_attach_encoder(connector, encoder);
-		if (ret) {
-			DRM_ERROR("Failed to attach connector to encoder\n");
-			goto err;
-		}
-
-		drm_panel_attach(ldev->panel, connector);
  	}
 
  	crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);

@@ -1130,8 +1026,8 @@ int ltdc_load(struct drm_device *ddev)
 
  	return 0;
  err:
-	if (ldev->panel)
-		drm_panel_detach(ldev->panel);
+	if (ldev->bridge)
+		drm_panel_bridge_remove(ldev->bridge);
 
  	clk_disable_unprepare(ldev->pixel_clk);
 

@@ -1146,8 +1042,8 @@ void ltdc_unload(struct drm_device *ddev)
 
  	drm_vblank_cleanup(ddev);
 
-	if (ldev->panel)
-		drm_panel_detach(ldev->panel);
+	if (ldev->bridge)
+		drm_panel_bridge_remove(ldev->bridge);
 
  	clk_disable_unprepare(ldev->pixel_clk);
  }

drivers/gpu/drm/stm/ltdc.h +1 -1

index d7a9c736ac1e..d78cb0dd3200 100644
@@ -24,7 +24,7 @@ struct ltdc_device {
  	struct drm_fbdev_cma *fbdev;
  	void __iomem *regs;
  	struct clk *pixel_clk;	/* lcd pixel clock */
-	struct drm_panel *panel;
+	struct drm_bridge *bridge;
  	struct mutex err_lock;	/* protecting error_status */
  	struct ltdc_caps caps;
  	u32 clut[256];		/* color look up table */

Hi Philippe,

Quoted 2 lines · Philippe CORNU

Hi Eric,

Quoted 2 lines

I took your patch for the panel-bridge and it works perfectly in both DPI mode (panel RGB //) and DSI mode (bridge dw mipi dsi), bravo :-)

I still don't understand how it can work without a call to drm_bridge_attach() (which is used to link the RGB encoder to the DPI connector). I'm probably missing something obvious. Maybe someone can point it out :-).

Thanks,

Boris

Quoted 247 lines

~160 lines have been removed from ltdc.c thanks to your panel-bridge code!

Many thanks
Philippe

Quoted 240 lines · Eric Anholt

Another 100 lines of boilerplate gone. Bridges aren't supported yet, but will be trivial to add later.

Signed-off-by Eric Anholt <[email protected]>

---

Also untested.

  drivers/gpu/drm/stm/ltdc.c | 128 +++++----------------------------------------
  drivers/gpu/drm/stm/ltdc.h |   2 +-
  2 files changed, 13 insertions(+), 117 deletions(-)

drivers/gpu/drm/stm/ltdc.c +12 -116

index a40418cda74a..41a1c5d68f5b 100644
@@ -269,11 +269,6 @@ static inline struct ltdc_device *encoder_to_ltdc(struct drm_encoder *enc)
  	return (struct ltdc_device *)enc->dev->dev_private;
  }
 
-static inline struct ltdc_device *connector_to_ltdc(struct drm_connector *con)
-{
-	return (struct ltdc_device *)con->dev->dev_private;
-}
-
  static inline enum ltdc_pix_fmt to_ltdc_pixelformat(u32 drm_fmt)
  {
  	enum ltdc_pix_fmt pf;

@@ -815,22 +810,12 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
 
  static void ltdc_rgb_encoder_enable(struct drm_encoder *encoder)
  {
-	struct ltdc_device *ldev = encoder_to_ltdc(encoder);
-
  	DRM_DEBUG_DRIVER("\n");
-
-	drm_panel_prepare(ldev->panel);
-	drm_panel_enable(ldev->panel);
  }
 
  static void ltdc_rgb_encoder_disable(struct drm_encoder *encoder)
  {
-	struct ltdc_device *ldev = encoder_to_ltdc(encoder);
-
  	DRM_DEBUG_DRIVER("\n");
-
-	drm_panel_disable(ldev->panel);
-	drm_panel_unprepare(ldev->panel);
  }
 
  static const struct drm_encoder_helper_funcs ltdc_rgb_encoder_helper_funcs = {

@@ -863,82 +848,6 @@ static struct drm_encoder *ltdc_rgb_encoder_create(struct drm_device *ddev)
  	return encoder;
  }
 
-/*
- * DRM_CONNECTOR
- */
-
-static int ltdc_rgb_connector_get_modes(struct drm_connector *connector)
-{
-	struct drm_device *ddev = connector->dev;
-	struct ltdc_device *ldev = ddev->dev_private;
-	int ret = 0;
-
-	DRM_DEBUG_DRIVER("\n");
-
-	if (ldev->panel)
-		ret = drm_panel_get_modes(ldev->panel);
-
-	return ret < 0 ? 0 : ret;
-}
-
-static struct drm_connector_helper_funcs ltdc_rgb_connector_helper_funcs = {
-	.get_modes = ltdc_rgb_connector_get_modes,
-};
-
-static enum drm_connector_status
-ltdc_rgb_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct ltdc_device *ldev = connector_to_ltdc(connector);
-
-	return ldev->panel ? connector_status_connected :
-	       connector_status_disconnected;
-}
-
-static void ltdc_rgb_connector_destroy(struct drm_connector *connector)
-{
-	DRM_DEBUG_DRIVER("\n");
-
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-}
-
-static const struct drm_connector_funcs ltdc_rgb_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.detect = ltdc_rgb_connector_detect,
-	.destroy = ltdc_rgb_connector_destroy,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-struct drm_connector *ltdc_rgb_connector_create(struct drm_device *ddev)
-{
-	struct drm_connector *connector;
-	int err;
-
-	connector = devm_kzalloc(ddev->dev, sizeof(*connector), GFP_KERNEL);
-	if (!connector) {
-		DRM_ERROR("Failed to allocate connector\n");
-		return NULL;
-	}
-
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
-
-	err = drm_connector_init(ddev, connector, &ltdc_rgb_connector_funcs,
-				 DRM_MODE_CONNECTOR_DPI);
-	if (err) {
-		DRM_ERROR("Failed to initialize connector\n");
-		return NULL;
-	}
-
-	drm_connector_helper_add(connector, &ltdc_rgb_connector_helper_funcs);
-
-	DRM_DEBUG_DRIVER("RGB connector:%d created\n", connector->base.id);
-
-	return connector;
-}
-
  static int ltdc_get_caps(struct drm_device *ddev)
  {
  	struct ltdc_device *ldev = ddev->dev_private;

@@ -972,7 +881,7 @@ static int ltdc_get_caps(struct drm_device *ddev)
  	return 0;
  }
 
-static struct drm_panel *ltdc_get_panel(struct drm_device *ddev)
+static struct drm_bridge *ltdc_get_bridge(struct drm_device *ddev)
  {
  	struct device *dev = ddev->dev;
  	struct device_node *np = dev->of_node;

@@ -1004,7 +913,10 @@ static struct drm_panel *ltdc_get_panel(struct drm_device *ddev)
  		}
  	}
 
-	return panel;
+	if (!panel)
+		return ERR_PTR(-ENODEV);
+
+	return drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DPI);
  }
 
  int ltdc_load(struct drm_device *ddev)

@@ -1014,7 +926,6 @@ int ltdc_load(struct drm_device *ddev)
  	struct device *dev = ddev->dev;
  	struct device_node *np = dev->of_node;
  	struct drm_encoder *encoder;
-	struct drm_connector *connector = NULL;
  	struct drm_crtc *crtc;
  	struct reset_control *rstc;
  	struct resource res;

@@ -1022,8 +933,8 @@ int ltdc_load(struct drm_device *ddev)
 
  	DRM_DEBUG_DRIVER("\n");
 
-	ldev->panel = ltdc_get_panel(ddev);
-	if (!ldev->panel)
+	ldev->bridge = ltdc_get_bridge(ddev);
+	if (!ldev->bridge)
  		return -EPROBE_DEFER;
 
  	rstc = of_reset_control_get(np, NULL);

@@ -1082,28 +993,13 @@ int ltdc_load(struct drm_device *ddev)
 
  	DRM_INFO("ltdc hw version 0x%08x - ready\n", ldev->caps.hw_version);
 
-	if (ldev->panel) {
+	if (ldev->bridge) {
  		encoder = ltdc_rgb_encoder_create(ddev);
  		if (!encoder) {
  			DRM_ERROR("Failed to create RGB encoder\n");
  			ret = -EINVAL;
  			goto err;
  		}
-
-		connector = ltdc_rgb_connector_create(ddev);
-		if (!connector) {
-			DRM_ERROR("Failed to create RGB connector\n");
-			ret = -EINVAL;
-			goto err;
-		}
-
-		ret = drm_mode_connector_attach_encoder(connector, encoder);
-		if (ret) {
-			DRM_ERROR("Failed to attach connector to encoder\n");
-			goto err;
-		}
-
-		drm_panel_attach(ldev->panel, connector);
  	}
 
  	crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);

@@ -1130,8 +1026,8 @@ int ltdc_load(struct drm_device *ddev)
 
  	return 0;
  err:
-	if (ldev->panel)
-		drm_panel_detach(ldev->panel);
+	if (ldev->bridge)
+		drm_panel_bridge_remove(ldev->bridge);
 
  	clk_disable_unprepare(ldev->pixel_clk);
 

@@ -1146,8 +1042,8 @@ void ltdc_unload(struct drm_device *ddev)
 
  	drm_vblank_cleanup(ddev);
 
-	if (ldev->panel)
-		drm_panel_detach(ldev->panel);
+	if (ldev->bridge)
+		drm_panel_bridge_remove(ldev->bridge);
 
  	clk_disable_unprepare(ldev->pixel_clk);
  }

drivers/gpu/drm/stm/ltdc.h +1 -1

index d7a9c736ac1e..d78cb0dd3200 100644
@@ -24,7 +24,7 @@ struct ltdc_device {
  	struct drm_fbdev_cma *fbdev;
  	void __iomem *regs;
  	struct clk *pixel_clk;	/* lcd pixel clock */
-	struct drm_panel *panel;
+	struct drm_bridge *bridge;
  	struct mutex err_lock;	/* protecting error_status */
  	struct ltdc_caps caps;
  	u32 clut[256];		/* color look up table */

Hi Boris,

Quoted 7 lines · Boris Brezillon

Hi Philippe,

Quoted 2 lines · Philippe CORNU

Hi Eric,

Quoted 7 lines

Quoted 2 lines

I took your patch for the panel-bridge and it works perfectly in both DPI mode (panel RGB //) and DSI mode (bridge dw mipi dsi), bravo :-)

I still don't understand how it can work without a call to drm_bridge_attach() (which is used to link the RGB encoder to the DPI connector). I'm probably missing something obvious. Maybe someone can point it out :-).

I think the expectation is that there will be a follow-up patch that would add bridge support. Philippe had posted a patch for adding bridges to ltdc before:

https://patchwork.kernel.org/patch/9724835/

He can now redo his patch over Eric's panel-bridge patch-set.

Hi Archit,

Quoted 24 lines · Archit Taneja

Hi Boris,

Quoted 7 lines · Boris Brezillon

Hi Philippe,

Quoted 2 lines · Philippe CORNU

Hi Eric,

Quoted 7 lines

Quoted 2 lines

I took your patch for the panel-bridge and it works perfectly in both DPI mode (panel RGB //) and DSI mode (bridge dw mipi dsi), bravo :-)

I still don't understand how it can work without a call to drm_bridge_attach() (which is used to link the RGB encoder to the DPI connector). I'm probably missing something obvious. Maybe someone can point it out :-).

I think the expectation is that there will be a follow-up patch that would add bridge support. Philippe had posted a patch for adding bridges to ltdc before:

https://patchwork.kernel.org/patch/9724835/

Quoted 1 line

He can now redo his patch over Eric's panel-bridge patch-set.

But by doing that you're breaking bisectability, or is it working when applying only Eric's patchset?

If applying this patch breaks the driver, why not applying it after bridge support has been added to this driver?

Regards,

Boris

Quoted 29 lines · Boris Brezillon

Hi Archit,

Quoted 24 lines · Archit Taneja

Hi Boris,

Quoted 7 lines · Boris Brezillon

Hi Philippe,

Quoted 2 lines · Philippe CORNU

Hi Eric,

Quoted 7 lines

Quoted 2 lines

I took your patch for the panel-bridge and it works perfectly in both DPI mode (panel RGB //) and DSI mode (bridge dw mipi dsi), bravo :-)

I still don't understand how it can work without a call to drm_bridge_attach() (which is used to link the RGB encoder to the DPI connector). I'm probably missing something obvious. Maybe someone can point it out :-).

I think the expectation is that there will be a follow-up patch that would add bridge support. Philippe had posted a patch for adding bridges to ltdc before:

https://patchwork.kernel.org/patch/9724835/

Quoted 7 lines

Quoted 1 line

He can now redo his patch over Eric's panel-bridge patch-set.

But by doing that you're breaking bisectability, or is it working when applying only Eric's patchset?

If applying this patch breaks the driver, why not applying it after bridge support has been added to this driver?

Yeah, it would break the driver. Philippe can create a patch that adds bridge support using the panel-bridge glue layer using Eric's patch as a reference. In the end, there should only be a single patch.

Thanks,
Archit

Quoted 5 lines

Regards,

Boris

drm_encoder_cleanup() finishes with memsetting it to 0, already.

Signed-off-by Eric Anholt <[email protected]>

---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +0 -2

index 65a3bd7a0c00..4b2cfbd0d43f 100644
@@ -83,14 +83,8 @@ static const struct drm_encoder_helper_funcs atmel_hlcdc_panel_encoder_helper_fu
 	.enable = atmel_hlcdc_rgb_encoder_enable,
 };
 
-static void atmel_hlcdc_rgb_encoder_destroy(struct drm_encoder *encoder)
-{

This cuts 135 lines of boilerplate, at the cost of losing the filtering of get_modes() using atmel_hlcdc_dc_mode_valid(). The atomic check will still check that we don't set an invalid mode, though.

Signed-off-by Eric Anholt <[email protected]>

---

This patch is just a proposal for atmel-hlcdc if you like it -- maybe you still want the filtering of get_modes(). I'm not sure from a userspace API perspective if we should really have the encoder (your CRTC's limits) filtering modes from a connector, but I'm also pretty sure it doesn't actually matter.

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 163 ++---------------------
 1 file changed, 14 insertions(+), 149 deletions(-)

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +3 -120

index 4b2cfbd0d43f..340ef962aa81 100644
@@ -23,191 +23,56 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_of.h>
+#include <drm/drm_bridge.h>
 
 #include "atmel_hlcdc_dc.h"
 
-/**
- * Atmel HLCDC RGB connector structure
- *
- * This structure stores RGB slave device information.
- *
- * @connector: DRM connector
- * @encoder: DRM encoder
- * @dc: pointer to the atmel_hlcdc_dc structure
- * @panel: panel connected on the RGB output
- */
-struct atmel_hlcdc_rgb_output {
-	struct drm_connector connector;
-	struct drm_encoder encoder;
-	struct atmel_hlcdc_dc *dc;
-	struct drm_panel *panel;
-};
-
-static inline struct atmel_hlcdc_rgb_output *
-drm_connector_to_atmel_hlcdc_rgb_output(struct drm_connector *connector)
-{
-	return container_of(connector, struct atmel_hlcdc_rgb_output,
-			    connector);
-}
-
-static inline struct atmel_hlcdc_rgb_output *
-drm_encoder_to_atmel_hlcdc_rgb_output(struct drm_encoder *encoder)
-{
-	return container_of(encoder, struct atmel_hlcdc_rgb_output, encoder);
-}
-
-static void atmel_hlcdc_rgb_encoder_enable(struct drm_encoder *encoder)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_encoder_to_atmel_hlcdc_rgb_output(encoder);
-
-	if (rgb->panel) {
-		drm_panel_prepare(rgb->panel);
-		drm_panel_enable(rgb->panel);
-	}
-}
-
-static void atmel_hlcdc_rgb_encoder_disable(struct drm_encoder *encoder)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_encoder_to_atmel_hlcdc_rgb_output(encoder);
-
-	if (rgb->panel) {
-		drm_panel_disable(rgb->panel);
-		drm_panel_unprepare(rgb->panel);
-	}
-}
-
-static const struct drm_encoder_helper_funcs atmel_hlcdc_panel_encoder_helper_funcs = {
-	.disable = atmel_hlcdc_rgb_encoder_disable,
-	.enable = atmel_hlcdc_rgb_encoder_enable,
-};
-
 static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
-static int atmel_hlcdc_panel_get_modes(struct drm_connector *connector)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	if (rgb->panel)
-		return rgb->panel->funcs->get_modes(rgb->panel);
-
-	return 0;
-}
-
-static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
-				      struct drm_display_mode *mode)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	return atmel_hlcdc_dc_mode_valid(rgb->dc, mode);
-}
-
-static const struct drm_connector_helper_funcs atmel_hlcdc_panel_connector_helper_funcs = {
-	.get_modes = atmel_hlcdc_panel_get_modes,
-	.mode_valid = atmel_hlcdc_rgb_mode_valid,
-};
-
-static enum drm_connector_status
-atmel_hlcdc_panel_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	if (rgb->panel)
-		return connector_status_connected;
-
-	return connector_status_disconnected;
-}
-
-static void
-atmel_hlcdc_panel_connector_destroy(struct drm_connector *connector)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	if (rgb->panel)
-		drm_panel_detach(rgb->panel);
-
-	drm_connector_cleanup(connector);
-}
-
-static const struct drm_connector_funcs atmel_hlcdc_panel_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.detect = atmel_hlcdc_panel_connector_detect,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = atmel_hlcdc_panel_connector_destroy,
-	.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 atmel_hlcdc_attach_endpoint(struct drm_device *dev,
 				       const struct device_node *np)
 {
-	struct atmel_hlcdc_dc *dc = dev->dev_private;
-	struct atmel_hlcdc_rgb_output *output;
+	struct drm_encoder *encoder;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 	int ret;
 
-	output = devm_kzalloc(dev->dev, sizeof(*output), GFP_KERNEL);
-	if (!output)
+	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +1 -14

 			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret)
 		return ret;
 
-	output->encoder.possible_crtcs = 0x1;
+	encoder->possible_crtcs = 0x1;
 
 	ret = drm_of_find_panel_or_bridge(np, 0, 0, &panel, &bridge);
 	if (ret)
 		return ret;
 
 	if (panel) {
-		output->connector.dpms = DRM_MODE_DPMS_OFF;
-		output->connector.polled = DRM_CONNECTOR_POLL_CONNECT;
-		drm_connector_helper_add(&output->connector,
-				&atmel_hlcdc_panel_connector_helper_funcs);
-		ret = drm_connector_init(dev, &output->connector,
-					 &atmel_hlcdc_panel_connector_funcs,
-					 DRM_MODE_CONNECTOR_Unknown);
-		if (ret)
-			goto err_encoder_cleanup;
-
-		drm_mode_connector_attach_encoder(&output->connector,
-						  &output->encoder);
-

Hi Eric,

Quoted 4 lines · On Thu, 11 May 2017 11:31:28 -0700 Eric Anholt <[email protected]> wrote:

This cuts 135 lines of boilerplate, at the cost of losing the filtering of get_modes() using atmel_hlcdc_dc_mode_valid(). The atomic check will still check that we don't set an invalid mode, though.

Nice.

Quoted 2 lines

Signed-off-by Eric Anholt <[email protected]>

Acked-by Boris Brezillon <[email protected]>

A few comments below (no need to address them, those are just minor things that can be fixed later on, or things I'm not comfortable with but cannot be addressed easily).

Quoted 7 lines

---

This patch is just a proposal for atmel-hlcdc if you like it -- maybe you still want the filtering of get_modes(). I'm not sure from a userspace API perspective if we should really have the encoder (your CRTC's limits) filtering modes from a connector, but I'm also pretty sure it doesn't actually matter.

Actually I added this check because someone suggested it :-). I don't think it's a real problem, and that should definitely be checked at the CRTC level, not here.

Quoted 150 lines

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 163 ++---------------------
 1 file changed, 14 insertions(+), 149 deletions(-)

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +3 -120

index 4b2cfbd0d43f..340ef962aa81 100644
@@ -23,191 +23,56 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_of.h>
+#include <drm/drm_bridge.h>
 
 #include "atmel_hlcdc_dc.h"
 
-/**
- * Atmel HLCDC RGB connector structure
- *
- * This structure stores RGB slave device information.
- *
- * @connector: DRM connector
- * @encoder: DRM encoder
- * @dc: pointer to the atmel_hlcdc_dc structure
- * @panel: panel connected on the RGB output
- */
-struct atmel_hlcdc_rgb_output {
-	struct drm_connector connector;
-	struct drm_encoder encoder;
-	struct atmel_hlcdc_dc *dc;
-	struct drm_panel *panel;
-};
-
-static inline struct atmel_hlcdc_rgb_output *
-drm_connector_to_atmel_hlcdc_rgb_output(struct drm_connector *connector)
-{
-	return container_of(connector, struct atmel_hlcdc_rgb_output,
-			    connector);
-}
-
-static inline struct atmel_hlcdc_rgb_output *
-drm_encoder_to_atmel_hlcdc_rgb_output(struct drm_encoder *encoder)
-{
-	return container_of(encoder, struct atmel_hlcdc_rgb_output, encoder);
-}
-
-static void atmel_hlcdc_rgb_encoder_enable(struct drm_encoder *encoder)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_encoder_to_atmel_hlcdc_rgb_output(encoder);
-
-	if (rgb->panel) {
-		drm_panel_prepare(rgb->panel);
-		drm_panel_enable(rgb->panel);
-	}
-}
-
-static void atmel_hlcdc_rgb_encoder_disable(struct drm_encoder *encoder)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_encoder_to_atmel_hlcdc_rgb_output(encoder);
-
-	if (rgb->panel) {
-		drm_panel_disable(rgb->panel);
-		drm_panel_unprepare(rgb->panel);
-	}
-}
-
-static const struct drm_encoder_helper_funcs atmel_hlcdc_panel_encoder_helper_funcs = {
-	.disable = atmel_hlcdc_rgb_encoder_disable,
-	.enable = atmel_hlcdc_rgb_encoder_enable,
-};
-
 static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
-static int atmel_hlcdc_panel_get_modes(struct drm_connector *connector)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	if (rgb->panel)
-		return rgb->panel->funcs->get_modes(rgb->panel);
-
-	return 0;
-}
-
-static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
-				      struct drm_display_mode *mode)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	return atmel_hlcdc_dc_mode_valid(rgb->dc, mode);
-}
-
-static const struct drm_connector_helper_funcs atmel_hlcdc_panel_connector_helper_funcs = {
-	.get_modes = atmel_hlcdc_panel_get_modes,
-	.mode_valid = atmel_hlcdc_rgb_mode_valid,
-};
-
-static enum drm_connector_status
-atmel_hlcdc_panel_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	if (rgb->panel)
-		return connector_status_connected;
-
-	return connector_status_disconnected;
-}
-
-static void
-atmel_hlcdc_panel_connector_destroy(struct drm_connector *connector)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	if (rgb->panel)
-		drm_panel_detach(rgb->panel);
-
-	drm_connector_cleanup(connector);
-}
-
-static const struct drm_connector_funcs atmel_hlcdc_panel_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.detect = atmel_hlcdc_panel_connector_detect,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = atmel_hlcdc_panel_connector_destroy,
-	.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 atmel_hlcdc_attach_endpoint(struct drm_device *dev,
 				       const struct device_node *np)
 {
-	struct atmel_hlcdc_dc *dc = dev->dev_private;
-	struct atmel_hlcdc_rgb_output *output;
+	struct drm_encoder *encoder;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 	int ret;
 
-	output = devm_kzalloc(dev->dev, sizeof(*output), GFP_KERNEL);
-	if (!output)
+	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c line 47

Quoted 10 lines

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +2 -5

+	if (!encoder)
 		return -EINVAL;
 
-	output->dc = dc;
-
-	drm_encoder_helper_add(&output->encoder,
-			       &atmel_hlcdc_panel_encoder_helper_funcs);
-	ret = drm_encoder_init(dev, &output->encoder,
+	ret = drm_encoder_init(dev, encoder,
 			       &atmel_hlcdc_panel_encoder_funcs,

atmel_hlcdc_panel_encoder_funcs should probably be renamed atmel_hlcdc_dpi_encoder_funcs, but it's just a detail.

Quoted 25 lines

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +1 -14

 			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret)
 		return ret;
 
-	output->encoder.possible_crtcs = 0x1;
+	encoder->possible_crtcs = 0x1;
 
 	ret = drm_of_find_panel_or_bridge(np, 0, 0, &panel, &bridge);
 	if (ret)
 		return ret;
 
 	if (panel) {
-		output->connector.dpms = DRM_MODE_DPMS_OFF;
-		output->connector.polled = DRM_CONNECTOR_POLL_CONNECT;
-		drm_connector_helper_add(&output->connector,
-				&atmel_hlcdc_panel_connector_helper_funcs);
-		ret = drm_connector_init(dev, &output->connector,
-					 &atmel_hlcdc_panel_connector_funcs,
-					 DRM_MODE_CONNECTOR_Unknown);
-		if (ret)
-			goto err_encoder_cleanup;
-
-		drm_mode_connector_attach_encoder(&output->connector,
-						  &output->encoder);
-

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c line 61

Quoted 10 lines

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +3 -7

-		ret = drm_panel_attach(panel, &output->connector);
-		if (ret) {
-			drm_connector_cleanup(&output->connector);
-			goto err_encoder_cleanup;
-		}
-
-		output->panel = panel;
+		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
+		if (IS_ERR(bridge))
+			return PTR_ERR(bridge);

It would even be simpler if panels were directly registering themselves to the bridge infrastructure so that they can be found with drm_of_find_bridge() and we don't have to explicitly create this bride->panel wrapper, but I remember that Thierry was not a big fan of this idea.

Also, I keep thinking that we're abusing objects. A bridge is supposed to convert a pixel stream in a given format/encoding into a different format/encoding. Here, there's no conversion taking place in this panel_bridge, it's just a way to expose the panel as a generic element that can be easily connected to many display controller drivers. To be perfectly clear, I really like this idea of providing a generic wrapper, I'm just unsure exposing this wrapper as a bridge is such a good idea, unless we clarify the meaning/goal of a 'drm_bridge' object.

The more I look at it, the more I think the drm_bridge is turning into a generic element that is taking a stream of pixel in a specific format and either consuming it of transforming it before forwarding it to the next element in the chain.

Regards,

Boris
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

This cuts 135 lines of boilerplate, at the cost of losing the filtering of get_modes() using atmel_hlcdc_dc_mode_valid(). The atomic check will still check that we don't set an invalid mode, though.

Signed-off-by Eric Anholt <[email protected]>

---

This patch is just a proposal for atmel-hlcdc if you like it -- maybe you still want the filtering of get_modes(). I'm not sure from a userspace API perspective if we should really have the encoder (your CRTC's limits) filtering modes from a connector, but I'm also pretty sure it doesn't actually matter.

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 163 ++---------------------
 1 file changed, 14 insertions(+), 149 deletions(-)

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +14 -149

index 4b2cfbd0d43f..340ef962aa81 100644
@@ -23,191 +23,56 @@
 
 #include <drm/drmP.h>
 #include <drm/drm_of.h>
+#include <drm/drm_bridge.h>
 
 #include "atmel_hlcdc_dc.h"
 
-/**
- * Atmel HLCDC RGB connector structure
- *
- * This structure stores RGB slave device information.
- *
- * @connector: DRM connector
- * @encoder: DRM encoder
- * @dc: pointer to the atmel_hlcdc_dc structure
- * @panel: panel connected on the RGB output
- */
-struct atmel_hlcdc_rgb_output {
-	struct drm_connector connector;
-	struct drm_encoder encoder;
-	struct atmel_hlcdc_dc *dc;
-	struct drm_panel *panel;
-};
-
-static inline struct atmel_hlcdc_rgb_output *
-drm_connector_to_atmel_hlcdc_rgb_output(struct drm_connector *connector)
-{
-	return container_of(connector, struct atmel_hlcdc_rgb_output,
-			    connector);
-}
-
-static inline struct atmel_hlcdc_rgb_output *
-drm_encoder_to_atmel_hlcdc_rgb_output(struct drm_encoder *encoder)
-{
-	return container_of(encoder, struct atmel_hlcdc_rgb_output, encoder);
-}
-
-static void atmel_hlcdc_rgb_encoder_enable(struct drm_encoder *encoder)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_encoder_to_atmel_hlcdc_rgb_output(encoder);
-
-	if (rgb->panel) {
-		drm_panel_prepare(rgb->panel);
-		drm_panel_enable(rgb->panel);
-	}
-}
-
-static void atmel_hlcdc_rgb_encoder_disable(struct drm_encoder *encoder)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_encoder_to_atmel_hlcdc_rgb_output(encoder);
-
-	if (rgb->panel) {
-		drm_panel_disable(rgb->panel);
-		drm_panel_unprepare(rgb->panel);
-	}
-}
-
-static const struct drm_encoder_helper_funcs atmel_hlcdc_panel_encoder_helper_funcs = {
-	.disable = atmel_hlcdc_rgb_encoder_disable,
-	.enable = atmel_hlcdc_rgb_encoder_enable,
-};
-
 static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
-static int atmel_hlcdc_panel_get_modes(struct drm_connector *connector)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	if (rgb->panel)
-		return rgb->panel->funcs->get_modes(rgb->panel);
-
-	return 0;
-}
-
-static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
-				      struct drm_display_mode *mode)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	return atmel_hlcdc_dc_mode_valid(rgb->dc, mode);
-}
-
-static const struct drm_connector_helper_funcs atmel_hlcdc_panel_connector_helper_funcs = {
-	.get_modes = atmel_hlcdc_panel_get_modes,
-	.mode_valid = atmel_hlcdc_rgb_mode_valid,
-};
-
-static enum drm_connector_status
-atmel_hlcdc_panel_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	if (rgb->panel)
-		return connector_status_connected;
-
-	return connector_status_disconnected;
-}
-
-static void
-atmel_hlcdc_panel_connector_destroy(struct drm_connector *connector)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	if (rgb->panel)
-		drm_panel_detach(rgb->panel);
-
-	drm_connector_cleanup(connector);
-}
-
-static const struct drm_connector_funcs atmel_hlcdc_panel_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.detect = atmel_hlcdc_panel_connector_detect,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = atmel_hlcdc_panel_connector_destroy,
-	.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 atmel_hlcdc_attach_endpoint(struct drm_device *dev,
 				       const struct device_node *np)
 {
-	struct atmel_hlcdc_dc *dc = dev->dev_private;
-	struct atmel_hlcdc_rgb_output *output;
+	struct drm_encoder *encoder;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 	int ret;
 
-	output = devm_kzalloc(dev->dev, sizeof(*output), GFP_KERNEL);
-	if (!output)
+	encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);
+	if (!encoder)
 		return -EINVAL;
 
-	output->dc = dc;
-
-	drm_encoder_helper_add(&output->encoder,
-			       &atmel_hlcdc_panel_encoder_helper_funcs);
-	ret = drm_encoder_init(dev, &output->encoder,
+	ret = drm_encoder_init(dev, encoder,
 			       &atmel_hlcdc_panel_encoder_funcs,
 			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret)
 		return ret;
 
-	output->encoder.possible_crtcs = 0x1;
+	encoder->possible_crtcs = 0x1;
 
 	ret = drm_of_find_panel_or_bridge(np, 0, 0, &panel, &bridge);
 	if (ret)
 		return ret;
 
 	if (panel) {
-		output->connector.dpms = DRM_MODE_DPMS_OFF;
-		output->connector.polled = DRM_CONNECTOR_POLL_CONNECT;
-		drm_connector_helper_add(&output->connector,
-				&atmel_hlcdc_panel_connector_helper_funcs);
-		ret = drm_connector_init(dev, &output->connector,
-					 &atmel_hlcdc_panel_connector_funcs,
-					 DRM_MODE_CONNECTOR_Unknown);
-		if (ret)
-			goto err_encoder_cleanup;
-
-		drm_mode_connector_attach_encoder(&output->connector,
-						  &output->encoder);
-
-		ret = drm_panel_attach(panel, &output->connector);
-		if (ret) {
-			drm_connector_cleanup(&output->connector);
-			goto err_encoder_cleanup;
-		}
-
-		output->panel = panel;
+		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
+		if (IS_ERR(bridge))
+			return PTR_ERR(bridge);
 
 		return 0;
 	}
 
 	if (bridge) {
-		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
+		ret = drm_bridge_attach(encoder, bridge, NULL);
 		if (!ret)
 			return 0;
+
+		if (panel)
+			drm_panel_bridge_remove(bridge);
 	}
 
-err_encoder_cleanup:
-	drm_encoder_cleanup(&output->encoder);
+	drm_encoder_cleanup(encoder);
 
 	return ret;
 }

Hi,

When pushing to drm-misc-next, I got a minor merge conflict when dim tried to rebuild drm-tip.

The reason for the conflict was because the following commit was present in drm-tip but not in drm-misc:

https://cgit.freedesktop.org/drm-tip/commit/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c?id=6bee9b78a7a5ea257b24d93974538938c82b1169

I fixed the conflict such that drm-tip is rebuit successfully. I'd however like if Eric and Boris could have a look at atmel_hlcdc_output.c to check if things are okay.

Quoted 25 lines

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +3 -20

 	if (panel) {
-		output->connector.dpms = DRM_MODE_DPMS_OFF;
-		output->connector.polled = DRM_CONNECTOR_POLL_CONNECT;
-		drm_connector_helper_add(&output->connector,
-				&atmel_hlcdc_panel_connector_helper_funcs);
-		ret = drm_connector_init(dev, &output->connector,
-					 &atmel_hlcdc_panel_connector_funcs,
-					 DRM_MODE_CONNECTOR_Unknown);
-		if (ret)
-			goto err_encoder_cleanup;
-
-		drm_mode_connector_attach_encoder(&output->connector,
-						  &output->encoder);
-
-		ret = drm_panel_attach(panel, &output->connector);
-		if (ret) {
-			drm_connector_cleanup(&output->connector);
-			goto err_encoder_cleanup;
-		}
-
-		output->panel = panel;
+		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
+		if (IS_ERR(bridge))
+			return PTR_ERR(bridge);

Quoted 1 line

return 0;

Quoted 12 lines

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +4 -1

 	}
 
 	if (bridge) {
-		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
+		ret = drm_bridge_attach(encoder, bridge, NULL);
 		if (!ret)
 			return 0;
+
+		if (panel)
+			drm_panel_bridge_remove(bridge);
 	}

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c line 78

Quoted 7 lines

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +1 -2

-err_encoder_cleanup:
-	drm_encoder_cleanup(&output->encoder);
+	drm_encoder_cleanup(encoder);
 
 	return ret;
 }

Hi Archit,

Quoted 8 lines · On Mon, 5 Jun 2017 21:06:20 +0530 Archit Taneja <[email protected]> wrote:

Hi,

When pushing to drm-misc-next, I got a minor merge conflict when dim tried to rebuild drm-tip.

The reason for the conflict was because the following commit was present in drm-tip but not in drm-misc:

Quoted 5 lines

https://cgit.freedesktop.org/drm-tip/commit/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c?id=6bee9b78a7a5ea257b24d93974538938c82b1169

I fixed the conflict such that drm-tip is rebuit successfully. I'd however like if Eric and Boris could have a look at atmel_hlcdc_output.c to check if things are okay.

Where can I find the fix you made to fix this problem?

[...]

Quoted 25 lines

Quoted 25 lines

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +3 -20

 	if (panel) {
-		output->connector.dpms = DRM_MODE_DPMS_OFF;
-		output->connector.polled = DRM_CONNECTOR_POLL_CONNECT;
-		drm_connector_helper_add(&output->connector,
-				&atmel_hlcdc_panel_connector_helper_funcs);
-		ret = drm_connector_init(dev, &output->connector,
-					 &atmel_hlcdc_panel_connector_funcs,
-					 DRM_MODE_CONNECTOR_Unknown);
-		if (ret)
-			goto err_encoder_cleanup;
-
-		drm_mode_connector_attach_encoder(&output->connector,
-						  &output->encoder);
-
-		ret = drm_panel_attach(panel, &output->connector);
-		if (ret) {
-			drm_connector_cleanup(&output->connector);
-			goto err_encoder_cleanup;
-		}
-
-		output->panel = panel;
+		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_Unknown);
+		if (IS_ERR(bridge))
+			return PTR_ERR(bridge);

Quoted 1 line

Quoted 1 line

return 0;

Just noticed that this is wrong. We should not return here, otherwise the panel bridge is never attached to the encoder.

Quoted 20 lines

Quoted 19 lines

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c +5 -3

 	}
 
 	if (bridge) {
-		ret = drm_bridge_attach(&output->encoder, bridge, NULL);
+		ret = drm_bridge_attach(encoder, bridge, NULL);
 		if (!ret)
 			return 0;
+
+		if (panel)
+			drm_panel_bridge_remove(bridge);
 	}
 
-err_encoder_cleanup:
-	drm_encoder_cleanup(&output->encoder);
+	drm_encoder_cleanup(encoder);
 
 	return ret;
 }

Many DRM drivers have common code to make a stub connector implementation that wraps a drm_panel. By wrapping the panel in a DRM bridge, all of the connector code (including calls during encoder enable/disable) goes away.

v2: Fix build with CONFIG_DRM=m, drop "dev" argument that should just
be the panel's dev, move kerneldoc up a level and document _remove().

Signed-off-by Eric Anholt <[email protected]>

Acked-by Daniel Vetter <[email protected]>

---
 Documentation/gpu/drm-kms-helpers.rst |   6 ++
 drivers/gpu/drm/Makefile              |   1 +
 drivers/gpu/drm/bridge/Kconfig        |  11 +-
 drivers/gpu/drm/bridge/lvds-encoder.c | 157 ++++-----------------------
 drivers/gpu/drm/bridge/panel.c        | 197 ++++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h              |   7 ++
 6 files changed, 238 insertions(+), 141 deletions(-)
 create mode 100644 drivers/gpu/drm/bridge/panel.c

Documentation/gpu/drm-kms-helpers.rst +6 -0

index c075aadd7078..7c5e2549a58a 100644
@@ -143,6 +143,12 @@ Bridge Helper Reference
 .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
    :export:
 
+Panel-Bridge Helper Reference
+-----------------------------
+
+.. kernel-doc:: drivers/gpu/drm/bridge/panel.c
+   :export:
+
 .. _drm_panel_helper:
 
 Panel Helper Reference

drivers/gpu/drm/Makefile +1 -0

index c156fecfb362..4cc9c02cc3f2 100644
@@ -24,6 +24,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 drm-$(CONFIG_PCI) += ati_pcigart.o
 drm-$(CONFIG_DRM_PANEL) += drm_panel.o
+drm-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
 drm-$(CONFIG_OF) += drm_of.o
 drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o

drivers/gpu/drm/bridge/Kconfig +9 -2

index f6968d3b4b41..c4daca38743c 100644
@@ -4,6 +4,14 @@ config DRM_BRIDGE
 	help
 	  Bridge registration and lookup framework.
 
+config DRM_PANEL_BRIDGE
+	def_bool y
+	depends on DRM_BRIDGE
+	select DRM_KMS_HELPER
+	select DRM_PANEL
+	help
+	  DRM bridge wrapper of DRM panels
+
 menu "Display Interface Bridges"
 	depends on DRM && DRM_BRIDGE
 

@@ -27,8 +35,7 @@ config DRM_DUMB_VGA_DAC
 config DRM_LVDS_ENCODER
 	tristate "Transparent parallel to LVDS encoder support"
 	depends on OF
-	select DRM_KMS_HELPER
-	select DRM_PANEL
+	select DRM_PANEL_BRIDGE
 	help
 	  Support for transparent parallel to LVDS encoders that don't require
 	  any configuration.

drivers/gpu/drm/bridge/lvds-encoder.c +18 -139

index f1f67a279426..0903ba574f61 100644
@@ -8,144 +8,18 @@
  */
 
 #include <drm/drmP.h>
-#include <drm/drm_atomic_helper.h>
-#include <drm/drm_connector.h>
-#include <drm/drm_crtc_helper.h>
-#include <drm/drm_encoder.h>
-#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_panel.h>
 
 #include <linux/of_graph.h>
 
-struct lvds_encoder {
-	struct device *dev;
-
-	struct drm_bridge bridge;
-	struct drm_connector connector;
-	struct drm_panel *panel;
-};
-
-static inline struct lvds_encoder *
-drm_bridge_to_lvds_encoder(struct drm_bridge *bridge)
-{
-	return container_of(bridge, struct lvds_encoder, bridge);
-}
-
-static inline struct lvds_encoder *
-drm_connector_to_lvds_encoder(struct drm_connector *connector)
-{
-	return container_of(connector, struct lvds_encoder, connector);
-}
-
-static int lvds_connector_get_modes(struct drm_connector *connector)
-{
-	struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector);
-
-	return drm_panel_get_modes(lvds->panel);
-}
-
-static const struct drm_connector_helper_funcs lvds_connector_helper_funcs = {
-	.get_modes = lvds_connector_get_modes,
-};
-
-static const struct drm_connector_funcs lvds_connector_funcs = {
-	.dpms = drm_atomic_helper_connector_dpms,
-	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static int lvds_encoder_attach(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
-	struct drm_connector *connector = &lvds->connector;
-	int ret;
-
-	if (!bridge->encoder) {
-		DRM_ERROR("Missing encoder\n");
-		return -ENODEV;
-	}
-
-	drm_connector_helper_add(connector, &lvds_connector_helper_funcs);
-
-	ret = drm_connector_init(bridge->dev, connector, &lvds_connector_funcs,
-				 DRM_MODE_CONNECTOR_LVDS);
-	if (ret) {
-		DRM_ERROR("Failed to initialize connector\n");
-		return ret;
-	}
-
-	drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder);
-
-	ret = drm_panel_attach(lvds->panel, &lvds->connector);
-	if (ret < 0)
-		return ret;
-
-	return 0;
-}
-
-static void lvds_encoder_detach(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
-
-	drm_panel_detach(lvds->panel);
-}
-
-static void lvds_encoder_pre_enable(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
-
-	drm_panel_prepare(lvds->panel);
-}
-
-static void lvds_encoder_enable(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
-
-	drm_panel_enable(lvds->panel);
-}
-
-static void lvds_encoder_disable(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
-
-	drm_panel_disable(lvds->panel);
-}
-
-static void lvds_encoder_post_disable(struct drm_bridge *bridge)
-{
-	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
-
-	drm_panel_unprepare(lvds->panel);
-}
-
-static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = {
-	.attach = lvds_encoder_attach,
-	.detach = lvds_encoder_detach,
-	.pre_enable = lvds_encoder_pre_enable,
-	.enable = lvds_encoder_enable,
-	.disable = lvds_encoder_disable,
-	.post_disable = lvds_encoder_post_disable,
-};
-
 static int lvds_encoder_probe(struct platform_device *pdev)
 {
-	struct lvds_encoder *lvds;
 	struct device_node *port;
 	struct device_node *endpoint;
-	struct device_node *panel;
-
-	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
-	if (!lvds)
-		return -ENOMEM;
-
-	lvds->dev = &pdev->dev;
-	platform_set_drvdata(pdev, lvds);
-
-	lvds->bridge.funcs = &lvds_encoder_bridge_funcs;
-	lvds->bridge.of_node = pdev->dev.of_node;
+	struct device_node *panel_node;
+	struct drm_panel *panel;
+	struct drm_bridge *bridge;
 
 	/* Locate the panel DT node. */
 	port = of_graph_get_port_by_id(pdev->dev.of_node, 1);

@@ -161,29 +35,34 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	panel = of_graph_get_remote_port_parent(endpoint);
+	panel_node = of_graph_get_remote_port_parent(endpoint);
 	of_node_put(endpoint);
-	if (!panel) {
+	if (!panel_node) {
 		dev_dbg(&pdev->dev, "no remote endpoint for port 1\n");
 		return -ENXIO;
 	}
 
-	lvds->panel = of_drm_find_panel(panel);
-	of_node_put(panel);
-	if (!lvds->panel) {
+	panel = of_drm_find_panel(panel_node);
+	of_node_put(panel_node);
+	if (!panel) {
 		dev_dbg(&pdev->dev, "panel not found, deferring probe\n");
 		return -EPROBE_DEFER;
 	}
 
-	/* Register the bridge. */
-	return drm_bridge_add(&lvds->bridge);
+	bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_LVDS);
+	if (IS_ERR(bridge))
+		return PTR_ERR(bridge);
+
+	platform_set_drvdata(pdev, bridge);
+
+	return 0;
 }
 
 static int lvds_encoder_remove(struct platform_device *pdev)
 {
-	struct lvds_encoder *encoder = platform_get_drvdata(pdev);
+	struct drm_bridge *bridge = platform_get_drvdata(pdev);
 
-	drm_bridge_remove(&encoder->bridge);
+	drm_bridge_remove(bridge);
 
 	return 0;
 }

drivers/gpu/drm/bridge/panel.c new file +197 -0

new file mode 100644
index 000000000000..fd1a78cb1205
@@ -0,0 +1,197 @@
+/*
+ * Copyright (C) 2016 Laurent Pinchart <[email protected]>
+ * Copyright (C) 2017 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_panel.h>
+
+struct panel_bridge {
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct drm_panel *panel;
+	u32 connector_type;
+};
+
+static inline struct panel_bridge *
+drm_bridge_to_panel_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct panel_bridge, bridge);
+}
+
+static inline struct panel_bridge *
+drm_connector_to_panel_bridge(struct drm_connector *connector)
+{
+	return container_of(connector, struct panel_bridge, connector);
+}
+
+static int panel_bridge_connector_get_modes(struct drm_connector *connector)
+{
+	struct panel_bridge *panel_bridge =
+		drm_connector_to_panel_bridge(connector);
+
+	return drm_panel_get_modes(panel_bridge->panel);
+}
+
+static const struct drm_connector_helper_funcs panel_bridge_connector_helper_funcs = {
+	.get_modes = panel_bridge_connector_get_modes,
+};
+
+static const struct drm_connector_funcs panel_bridge_connector_funcs = {
+	.dpms = drm_atomic_helper_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int panel_bridge_attach(struct drm_bridge *bridge)
+{
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+	struct drm_connector *connector = &panel_bridge->connector;
+	int ret;
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Missing encoder\n");
+		return -ENODEV;
+	}
+
+	drm_connector_helper_add(connector,
+				 &panel_bridge_connector_helper_funcs);
+
+	ret = drm_connector_init(bridge->dev, connector,
+				 &panel_bridge_connector_funcs,
+				 panel_bridge->connector_type);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector\n");
+		return ret;
+	}
+
+	drm_mode_connector_attach_encoder(&panel_bridge->connector,
+					  bridge->encoder);
+
+	ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static void panel_bridge_detach(struct drm_bridge *bridge)
+{
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+
+	drm_panel_detach(panel_bridge->panel);
+}
+
+static void panel_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+
+	drm_panel_prepare(panel_bridge->panel);
+}
+
+static void panel_bridge_enable(struct drm_bridge *bridge)
+{
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+
+	drm_panel_enable(panel_bridge->panel);
+}
+
+static void panel_bridge_disable(struct drm_bridge *bridge)
+{
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+
+	drm_panel_disable(panel_bridge->panel);
+}
+
+static void panel_bridge_post_disable(struct drm_bridge *bridge)
+{
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+
+	drm_panel_unprepare(panel_bridge->panel);
+}
+
+static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
+	.attach = panel_bridge_attach,
+	.detach = panel_bridge_detach,
+	.pre_enable = panel_bridge_pre_enable,
+	.enable = panel_bridge_enable,
+	.disable = panel_bridge_disable,
+	.post_disable = panel_bridge_post_disable,
+};
+
+/**
+ * drm_panel_bridge_add - Creates a drm_bridge and drm_connector that
+ * just calls the appropriate functions from drm_panel.
+ *
+ * @panel: The drm_panel being wrapped.  Must be non-NULL.
+ * @connector_type: The DRM_MODE_CONNECTOR_* for the connector to be
+ * created.
+ *
+ * For drivers converting from directly using drm_panel: The expected
+ * usage pattern is that during either encoder module probe or DSI
+ * host attach, a drm_panel will be looked up through
+ * drm_of_find_panel_or_bridge().  drm_panel_bridge_add() is used to
+ * wrap that panel in the new bridge, and the result can then be
+ * passed to drm_bridge_attach().  The drm_panel_prepare() and related
+ * functions can be dropped from the encoder driver (they're now
+ * called by the KMS helpers before calling into the encoder), along
+ * with connector creation.  When done with the bridge,
+ * drm_bridge_detach() should be called as normal, then
+ * drm_panel_bridge_remove() to free it.
+ */
+struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
+					u32 connector_type)
+{
+	struct panel_bridge *panel_bridge;
+	int ret;
+
+	if (!panel)
+		return ERR_PTR(EINVAL);
+
+	panel_bridge = devm_kzalloc(panel->dev, sizeof(*panel_bridge),
+				    GFP_KERNEL);
+	if (!panel_bridge)
+		return ERR_PTR(-ENOMEM);
+
+	panel_bridge->connector_type = connector_type;
+	panel_bridge->panel = panel;
+
+	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
+	panel_bridge->bridge.of_node = panel->dev->of_node;
+
+	ret = drm_bridge_add(&panel_bridge->bridge);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &panel_bridge->bridge;
+}
+EXPORT_SYMBOL(drm_panel_bridge_add);
+
+/**
+ * drm_panel_bridge_remove - Unregisters and frees a drm_bridge
+ * created by drm_panel_bridge_add().
+ *
+ * @bridge: The drm_bridge being freed.
+ */
+void drm_panel_bridge_remove(struct drm_bridge *bridge)
+{
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+
+	drm_bridge_remove(bridge);
+	devm_kfree(panel_bridge->panel->dev, bridge);
+}
+EXPORT_SYMBOL(drm_panel_bridge_remove);

include/drm/drm_bridge.h +1 -0

index fdd82fcbf168..bad2178ea3e0 100644
@@ -29,6 +29,7 @@
 #include <drm/drm_modes.h>
 
 struct drm_bridge;
+struct drm_panel;
 
 /**
  * struct drm_bridge_funcs - drm_bridge control functions