[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

The 'slave encoder' structure of the tda998x driver asks for glue
between the DRM driver and the encoder/connector structures.

This patch changes the driver to a normal DRM encoder/connector
thanks to the infrastructure for componentised subsystems.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 323 ++++++++++++++++++++++----------------
 1 file changed, 188 insertions(+), 135 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index fd6751c..1c25e40 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,11 +20,12 @@
 #include <linux/hdmi.h>
 #include <linux/module.h>
 #include <linux/irq.h>
+#include <linux/of_platform.h>
+#include <linux/component.h>
 #include <sound/asoundef.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
-#include <drm/drm_encoder_slave.h>
 #include <drm/drm_edid.h>
 #include <drm/i2c/tda998x.h>
 
@@ -44,10 +45,14 @@ struct tda998x_priv {
 
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
-	struct drm_encoder *encoder;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
 };
 
-#define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
+#define connector_priv(e) \
+		container_of(connector, struct tda998x_priv, connector)
+#define encoder_priv(e) \
+		container_of(encoder, struct tda998x_priv, encoder)
 
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
@@ -559,9 +564,8 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
 	if ((flag2 & INT_FLAGS_2_EDID_BLK_RD) && priv->wq_edid_wait) {
 		priv->wq_edid_wait = 0;
 		wake_up(&priv->wq_edid);
-	} else if (cec != 0) {			/* HPD change */
-		if (priv->encoder && priv->encoder->dev)
-			drm_helper_hpd_irq_event(priv->encoder->dev);
+	} else if (cec != 0 && priv->encoder.dev) {	/* HPD change */
+		drm_helper_hpd_irq_event(priv->encoder.dev);
 	}
 	return IRQ_HANDLED;
 }
@@ -731,9 +735,8 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 /* DRM encoder functions */
 
 static void
-tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
+tda998x_encoder_set_config(struct tda998x_priv *priv, void *params)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	struct tda998x_encoder_params *p = params;
 
 	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
@@ -755,7 +758,7 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
 static void
 tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
+	struct tda998x_priv *priv = encoder_priv(encoder);
 
 	/* we only care about on or off: */
 	if (mode != DRM_MODE_DPMS_ON)
@@ -786,18 +789,6 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 	priv->dpms = mode;
 }
 
-static void
-tda998x_encoder_save(struct drm_encoder *encoder)
-{
-	DBG("");
-}
-
-static void
-tda998x_encoder_restore(struct drm_encoder *encoder)
-{
-	DBG("");
-}
-
 static bool
 tda998x_encoder_mode_fixup(struct drm_encoder *encoder,
 			  const struct drm_display_mode *mode,
@@ -806,11 +797,14 @@ tda998x_encoder_mode_fixup(struct drm_encoder *encoder,
 	return true;
 }
 
-static int
-tda998x_encoder_mode_valid(struct drm_encoder *encoder,
-			  struct drm_display_mode *mode)
+static void tda998x_encoder_prepare(struct drm_encoder *encoder)
 {
-	return MODE_OK;
+	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
+}
+
+static void tda998x_encoder_commit(struct drm_encoder *encoder)
+{
+	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
 }
 
 static void
@@ -818,7 +812,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			struct drm_display_mode *mode,
 			struct drm_display_mode *adjusted_mode)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
+	struct tda998x_priv *priv = encoder_priv(encoder);
 	uint16_t ref_pix, ref_line, n_pix, n_line;
 	uint16_t hs_pix_s, hs_pix_e;
 	uint16_t vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1006,10 +1000,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 }
 
 static enum drm_connector_status
-tda998x_encoder_detect(struct drm_encoder *encoder,
-		      struct drm_connector *connector)
+tda998x_connector_detect(struct drm_connector *connector, bool force)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
+	struct tda998x_priv *priv = connector_priv(connector);
 	uint8_t val = cec_read(priv, REG_CEC_RXSHPDLEV);
 
 	return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
@@ -1017,9 +1010,8 @@ tda998x_encoder_detect(struct drm_encoder *encoder,
 }
 
 static int
-read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
+read_edid_block(struct tda998x_priv *priv, uint8_t *buf, int blk)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	uint8_t offset, segptr;
 	int ret, i;
 
@@ -1073,10 +1065,8 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 	return 0;
 }
 
-static uint8_t *
-do_get_edid(struct drm_encoder *encoder)
+static uint8_t *do_get_edid(struct tda998x_priv *priv)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	int j, valid_extensions = 0;
 	uint8_t *block, *new;
 	bool print_bad_edid = drm_debug & DRM_UT_KMS;
@@ -1088,7 +1078,7 @@ do_get_edid(struct drm_encoder *encoder)
 		reg_clear(priv, REG_TX4, TX4_PD_RAM);
 
 	/* base block fetch */
-	if (read_edid_block(encoder, block, 0))
+	if (read_edid_block(priv, block, 0))
 		goto fail;
 
 	if (!drm_edid_block_valid(block, 0, print_bad_edid))
@@ -1105,7 +1095,7 @@ do_get_edid(struct drm_encoder *encoder)
 
 	for (j = 1; j <= block[0x7e]; j++) {
 		uint8_t *ext_block = block + (valid_extensions + 1) * EDID_LENGTH;
-		if (read_edid_block(encoder, ext_block, j))
+		if (read_edid_block(priv, ext_block, j))
 			goto fail;
 
 		if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
@@ -1137,12 +1127,28 @@ fail:
 	return NULL;
 }
 
+/* DRM connector functions */
+
+static struct drm_encoder *
+tda998x_connector_best_encoder(struct drm_connector *connector)
+{
+	struct tda998x_priv *priv = connector_priv(connector);
+
+	return &priv->encoder;
+}
+
+static int
+tda998x_connector_mode_valid(struct drm_connector *connector,
+			  struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
 static int
-tda998x_encoder_get_modes(struct drm_encoder *encoder,
-			 struct drm_connector *connector)
+tda998x_connector_get_modes(struct drm_connector *connector)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-	struct edid *edid = (struct edid *)do_get_edid(encoder);
+	struct tda998x_priv *priv = connector_priv(connector);
+	struct edid *edid = (struct edid *) do_get_edid(priv);
 	int n = 0;
 
 	if (edid) {
@@ -1156,22 +1162,7 @@ tda998x_encoder_get_modes(struct drm_encoder *encoder,
 }
 
 static int
-tda998x_encoder_create_resources(struct drm_encoder *encoder,
-				struct drm_connector *connector)
-{
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-
-	if (priv->hdmi->irq)
-		connector->polled = DRM_CONNECTOR_POLL_HPD;
-	else
-		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			DRM_CONNECTOR_POLL_DISCONNECT;
-	return 0;
-}
-
-static int
-tda998x_encoder_set_property(struct drm_encoder *encoder,
-			    struct drm_connector *connector,
+tda998x_connector_set_property(struct drm_connector *connector,
 			    struct drm_property *property,
 			    uint64_t val)
 {
@@ -1179,56 +1170,117 @@ tda998x_encoder_set_property(struct drm_encoder *encoder,
 	return 0;
 }
 
-static void
-tda998x_encoder_destroy(struct drm_encoder *encoder)
+static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
+	.dpms = tda998x_encoder_dpms,
+	.mode_fixup = tda998x_encoder_mode_fixup,
+	.prepare = tda998x_encoder_prepare,
+	.commit = tda998x_encoder_commit,
+	.mode_set = tda998x_encoder_mode_set,
+};
+
+static void tda998x_encoder_destroy(struct drm_encoder *encoder)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-	drm_i2c_encoder_destroy(encoder);
+	drm_encoder_cleanup(encoder);
+}
 
-	/* disable all IRQs and free the IRQ handler */
-	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
-	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
-	if (priv->hdmi->irq)
-		free_irq(priv->hdmi->irq, priv);
+static const struct drm_encoder_funcs encoder_funcs = {
+	.destroy = tda998x_encoder_destroy,
+};
 
-	if (priv->cec)
-		i2c_unregister_device(priv->cec);
-	kfree(priv);
+static const struct drm_connector_helper_funcs connector_helper_funcs = {
+	.get_modes = tda998x_connector_get_modes,
+	.mode_valid = tda998x_connector_mode_valid,
+	.best_encoder = tda998x_connector_best_encoder,
+};
+
+static void tda998x_connector_destroy(struct drm_connector *connector)
+{
+	if (!connector->dev)
+		return;
+	drm_sysfs_connector_remove(connector);
+	drm_connector_cleanup(connector);
 }
 
-static struct drm_encoder_slave_funcs tda998x_encoder_funcs = {
-	.set_config = tda998x_encoder_set_config,
-	.destroy = tda998x_encoder_destroy,
-	.dpms = tda998x_encoder_dpms,
-	.save = tda998x_encoder_save,
-	.restore = tda998x_encoder_restore,
-	.mode_fixup = tda998x_encoder_mode_fixup,
-	.mode_valid = tda998x_encoder_mode_valid,
-	.mode_set = tda998x_encoder_mode_set,
-	.detect = tda998x_encoder_detect,
-	.get_modes = tda998x_encoder_get_modes,
-	.create_resources = tda998x_encoder_create_resources,
-	.set_property = tda998x_encoder_set_property,
+static const struct drm_connector_funcs connector_funcs = {
+	.detect = tda998x_connector_detect,
+	.set_property = tda998x_connector_set_property,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.dpms = drm_helper_connector_dpms,
+	.destroy = tda998x_connector_destroy,
 };
 
 /* I2C driver functions */
 
-static int
-tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
+static int tda_bind(struct device *dev, struct device *master, void *data)
 {
+	struct drm_device *drm = data;
+	struct i2c_client *i2c_client = to_i2c_client(dev);
+	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
+	struct drm_connector *connector = &priv->connector;
+	struct drm_encoder *encoder = &priv->encoder;
+	int ret;
+
+	if (!try_module_get(THIS_MODULE)) {
+		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
+		return -EINVAL;
+	}
+
+	ret = drm_connector_init(drm, connector,
+				&connector_funcs,
+				DRM_MODE_CONNECTOR_HDMIA);
+	if (ret < 0)
+		return ret;
+	drm_connector_helper_add(connector, &connector_helper_funcs);
+
+	ret = drm_encoder_init(drm, encoder,
+				&encoder_funcs,
+				DRM_MODE_ENCODER_TMDS);
+
+	encoder->possible_crtcs = 1;	// 1 << lcd_num
+
+	if (ret < 0)
+		goto err;
+	drm_encoder_helper_add(encoder, &encoder_helper_funcs);
+
+	ret = drm_mode_connector_attach_encoder(connector, encoder);
+	if (ret < 0)
+		goto err;
+	connector->encoder = encoder;
+
+	drm_sysfs_connector_add(connector);
+
+	drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
+	ret = drm_object_property_set_value(&connector->base,
+					drm->mode_config.dpms_property,
+					DRM_MODE_DPMS_OFF);
+
+	if (priv->hdmi->irq)
+		connector->polled = DRM_CONNECTOR_POLL_HPD;
+	else
+		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			DRM_CONNECTOR_POLL_DISCONNECT;
 	return 0;
+
+err:
+	if (encoder->dev)
+		drm_encoder_cleanup(encoder);
+	if (connector->dev)
+		drm_connector_cleanup(connector);
+	return ret;
 }
 
-static int
-tda998x_remove(struct i2c_client *client)
+static void tda_unbind(struct device *dev, struct device *master, void *data)
 {
-	return 0;
+	module_put(THIS_MODULE);
 }
 
+static const struct component_ops comp_ops = {
+	.bind = tda_bind,
+	.unbind = tda_unbind,
+};
+
 static int
-tda998x_encoder_init(struct i2c_client *client,
-		    struct drm_device *dev,
-		    struct drm_encoder_slave *encoder_slave)
+tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct tda998x_priv *priv;
 	struct device_node *np = client->dev.of_node;
@@ -1239,6 +1291,8 @@ tda998x_encoder_init(struct i2c_client *client,
 	if (!priv)
 		return -ENOMEM;
 
+	i2c_set_clientdata(client, priv);
+
 	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
 	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
 	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
@@ -1250,13 +1304,8 @@ tda998x_encoder_init(struct i2c_client *client,
 		kfree(priv);
 		return -ENODEV;
 	}
-
-	priv->encoder = &encoder_slave->base;
 	priv->dpms = DRM_MODE_DPMS_OFF;
 
-	encoder_slave->slave_priv = priv;
-	encoder_slave->slave_funcs = &tda998x_encoder_funcs;
-
 	/* wake up the device: */
 	cec_write(priv, REG_CEC_ENAMODS,
 			CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI);
@@ -1340,31 +1389,55 @@ tda998x_encoder_init(struct i2c_client *client,
 	/* enable EDID read irq: */
 	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
-	if (!np)
-		return 0;		/* non-DT */
+	if (np) {				/* if DT */
 
-	/* get the optional video properties */
-	ret = of_property_read_u32(np, "video-ports", &video);
-	if (ret == 0) {
-		priv->vip_cntrl_0 = video >> 16;
-		priv->vip_cntrl_1 = video >> 8;
-		priv->vip_cntrl_2 = video;
+		/* get the optional video properties */
+		ret = of_property_read_u32(np, "video-ports", &video);
+		if (ret == 0) {
+			priv->vip_cntrl_0 = video >> 16;
+			priv->vip_cntrl_1 = video >> 8;
+			priv->vip_cntrl_2 = video;
+		}
+	} else {
+		struct tda998x_encoder_params *params =
+				(struct tda998x_encoder_params)
+						client->dev.platform_data;
+
+		if (params)
+			tda998x_encoder_set_config(priv, params);
 	}
 
+	/* tda998x video component ready */
+	component_add(&client->dev, &comp_ops);
+
 	return 0;
 
 fail:
-	/* if encoder_init fails, the encoder slave is never registered,
-	 * so cleanup here:
-	 */
 	if (priv->cec)
 		i2c_unregister_device(priv->cec);
 	kfree(priv);
-	encoder_slave->slave_priv = NULL;
-	encoder_slave->slave_funcs = NULL;
 	return -ENXIO;
 }
 
+static int
+tda998x_remove(struct i2c_client *client)
+{
+	struct tda998x_priv *priv = i2c_get_clientdata(client);
+
+	/* disable all IRQs and free the IRQ handler */
+	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
+	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+	if (priv->hdmi->irq)
+		free_irq(priv->hdmi->irq, priv);
+
+	component_del(&client->dev, &comp_ops);
+
+	if (priv->cec)
+		i2c_unregister_device(priv->cec);
+	kfree(priv);
+	return 0;
+}
+
 #ifdef CONFIG_OF
 static const struct of_device_id tda998x_dt_ids[] = {
 	{ .compatible = "nxp,tda9989", },
@@ -1381,38 +1454,18 @@ static struct i2c_device_id tda998x_ids[] = {
 };
 MODULE_DEVICE_TABLE(i2c, tda998x_ids);
 
-static struct drm_i2c_encoder_driver tda998x_driver = {
-	.i2c_driver = {
-		.probe = tda998x_probe,
-		.remove = tda998x_remove,
-		.driver = {
-			.name = "tda998x",
-			.of_match_table = of_match_ptr(tda998x_dt_ids),
-		},
-		.id_table = tda998x_ids,
+static struct i2c_driver tda998x_driver = {
+	.probe = tda998x_probe,
+	.remove = tda998x_remove,
+	.driver = {
+		.name = "tda998x",
+		.of_match_table = of_match_ptr(tda998x_dt_ids),
 	},
-	.encoder_init = tda998x_encoder_init,
+	.id_table = tda998x_ids,
 };
 
-/* Module initialization */
-
-static int __init
-tda998x_init(void)
-{
-	DBG("");
-	return drm_i2c_encoder_register(THIS_MODULE, &tda998x_driver);
-}
-
-static void __exit
-tda998x_exit(void)
-{
-	DBG("");
-	drm_i2c_encoder_unregister(&tda998x_driver);
-}
+module_i2c_driver(tda998x_driver);
 
 MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
 MODULE_DESCRIPTION("NXP Semiconductors TDA998X HDMI Encoder");
 MODULE_LICENSE("GPL");
-
-module_init(tda998x_init);
-module_exit(tda998x_exit);
-- 
1.9.1

[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

The 'slave encoder' structure of the tda998x driver asks for glue
between the DRM driver and the encoder/connector structures.

This patch changes the driver to a normal DRM encoder/connector
thanks to the infrastructure for componentised subsystems.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 323 ++++++++++++++++++++++----------------
 1 file changed, 188 insertions(+), 135 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index fd6751c..1c25e40 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,11 +20,12 @@
 #include <linux/hdmi.h>
 #include <linux/module.h>
 #include <linux/irq.h>
+#include <linux/of_platform.h>
+#include <linux/component.h>
 #include <sound/asoundef.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
-#include <drm/drm_encoder_slave.h>
 #include <drm/drm_edid.h>
 #include <drm/i2c/tda998x.h>
 
@@ -44,10 +45,14 @@ struct tda998x_priv {
 
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
-	struct drm_encoder *encoder;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
 };
 
-#define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
+#define connector_priv(e) \
+		container_of(connector, struct tda998x_priv, connector)
+#define encoder_priv(e) \
+		container_of(encoder, struct tda998x_priv, encoder)
 
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
@@ -559,9 +564,8 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
 	if ((flag2 & INT_FLAGS_2_EDID_BLK_RD) && priv->wq_edid_wait) {
 		priv->wq_edid_wait = 0;
 		wake_up(&priv->wq_edid);
-	} else if (cec != 0) {			/* HPD change */
-		if (priv->encoder && priv->encoder->dev)
-			drm_helper_hpd_irq_event(priv->encoder->dev);
+	} else if (cec != 0 && priv->encoder.dev) {	/* HPD change */
+		drm_helper_hpd_irq_event(priv->encoder.dev);
 	}
 	return IRQ_HANDLED;
 }
@@ -731,9 +735,8 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 /* DRM encoder functions */
 
 static void
-tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
+tda998x_encoder_set_config(struct tda998x_priv *priv, void *params)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	struct tda998x_encoder_params *p = params;
 
 	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
@@ -755,7 +758,7 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
 static void
 tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
+	struct tda998x_priv *priv = encoder_priv(encoder);
 
 	/* we only care about on or off: */
 	if (mode != DRM_MODE_DPMS_ON)
@@ -786,18 +789,6 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 	priv->dpms = mode;
 }
 
-static void
-tda998x_encoder_save(struct drm_encoder *encoder)
-{
-	DBG("");
-}
-
-static void
-tda998x_encoder_restore(struct drm_encoder *encoder)
-{
-	DBG("");
-}
-
 static bool
 tda998x_encoder_mode_fixup(struct drm_encoder *encoder,
 			  const struct drm_display_mode *mode,
@@ -806,11 +797,14 @@ tda998x_encoder_mode_fixup(struct drm_encoder *encoder,
 	return true;
 }
 
-static int
-tda998x_encoder_mode_valid(struct drm_encoder *encoder,
-			  struct drm_display_mode *mode)
+static void tda998x_encoder_prepare(struct drm_encoder *encoder)
 {
-	return MODE_OK;
+	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
+}
+
+static void tda998x_encoder_commit(struct drm_encoder *encoder)
+{
+	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
 }
 
 static void
@@ -818,7 +812,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			struct drm_display_mode *mode,
 			struct drm_display_mode *adjusted_mode)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
+	struct tda998x_priv *priv = encoder_priv(encoder);
 	uint16_t ref_pix, ref_line, n_pix, n_line;
 	uint16_t hs_pix_s, hs_pix_e;
 	uint16_t vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1006,10 +1000,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 }
 
 static enum drm_connector_status
-tda998x_encoder_detect(struct drm_encoder *encoder,
-		      struct drm_connector *connector)
+tda998x_connector_detect(struct drm_connector *connector, bool force)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
+	struct tda998x_priv *priv = connector_priv(connector);
 	uint8_t val = cec_read(priv, REG_CEC_RXSHPDLEV);
 
 	return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
@@ -1017,9 +1010,8 @@ tda998x_encoder_detect(struct drm_encoder *encoder,
 }
 
 static int
-read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
+read_edid_block(struct tda998x_priv *priv, uint8_t *buf, int blk)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	uint8_t offset, segptr;
 	int ret, i;
 
@@ -1073,10 +1065,8 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 	return 0;
 }
 
-static uint8_t *
-do_get_edid(struct drm_encoder *encoder)
+static uint8_t *do_get_edid(struct tda998x_priv *priv)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	int j, valid_extensions = 0;
 	uint8_t *block, *new;
 	bool print_bad_edid = drm_debug & DRM_UT_KMS;
@@ -1088,7 +1078,7 @@ do_get_edid(struct drm_encoder *encoder)
 		reg_clear(priv, REG_TX4, TX4_PD_RAM);
 
 	/* base block fetch */
-	if (read_edid_block(encoder, block, 0))
+	if (read_edid_block(priv, block, 0))
 		goto fail;
 
 	if (!drm_edid_block_valid(block, 0, print_bad_edid))
@@ -1105,7 +1095,7 @@ do_get_edid(struct drm_encoder *encoder)
 
 	for (j = 1; j <= block[0x7e]; j++) {
 		uint8_t *ext_block = block + (valid_extensions + 1) * EDID_LENGTH;
-		if (read_edid_block(encoder, ext_block, j))
+		if (read_edid_block(priv, ext_block, j))
 			goto fail;
 
 		if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
@@ -1137,12 +1127,28 @@ fail:
 	return NULL;
 }
 
+/* DRM connector functions */
+
+static struct drm_encoder *
+tda998x_connector_best_encoder(struct drm_connector *connector)
+{
+	struct tda998x_priv *priv = connector_priv(connector);
+
+	return &priv->encoder;
+}
+
+static int
+tda998x_connector_mode_valid(struct drm_connector *connector,
+			  struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
 static int
-tda998x_encoder_get_modes(struct drm_encoder *encoder,
-			 struct drm_connector *connector)
+tda998x_connector_get_modes(struct drm_connector *connector)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-	struct edid *edid = (struct edid *)do_get_edid(encoder);
+	struct tda998x_priv *priv = connector_priv(connector);
+	struct edid *edid = (struct edid *) do_get_edid(priv);
 	int n = 0;
 
 	if (edid) {
@@ -1156,22 +1162,7 @@ tda998x_encoder_get_modes(struct drm_encoder *encoder,
 }
 
 static int
-tda998x_encoder_create_resources(struct drm_encoder *encoder,
-				struct drm_connector *connector)
-{
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-
-	if (priv->hdmi->irq)
-		connector->polled = DRM_CONNECTOR_POLL_HPD;
-	else
-		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			DRM_CONNECTOR_POLL_DISCONNECT;
-	return 0;
-}
-
-static int
-tda998x_encoder_set_property(struct drm_encoder *encoder,
-			    struct drm_connector *connector,
+tda998x_connector_set_property(struct drm_connector *connector,
 			    struct drm_property *property,
 			    uint64_t val)
 {
@@ -1179,56 +1170,117 @@ tda998x_encoder_set_property(struct drm_encoder *encoder,
 	return 0;
 }
 
-static void
-tda998x_encoder_destroy(struct drm_encoder *encoder)
+static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
+	.dpms = tda998x_encoder_dpms,
+	.mode_fixup = tda998x_encoder_mode_fixup,
+	.prepare = tda998x_encoder_prepare,
+	.commit = tda998x_encoder_commit,
+	.mode_set = tda998x_encoder_mode_set,
+};
+
+static void tda998x_encoder_destroy(struct drm_encoder *encoder)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-	drm_i2c_encoder_destroy(encoder);
+	drm_encoder_cleanup(encoder);
+}
 
-	/* disable all IRQs and free the IRQ handler */
-	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
-	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
-	if (priv->hdmi->irq)
-		free_irq(priv->hdmi->irq, priv);
+static const struct drm_encoder_funcs encoder_funcs = {
+	.destroy = tda998x_encoder_destroy,
+};
 
-	if (priv->cec)
-		i2c_unregister_device(priv->cec);
-	kfree(priv);
+static const struct drm_connector_helper_funcs connector_helper_funcs = {
+	.get_modes = tda998x_connector_get_modes,
+	.mode_valid = tda998x_connector_mode_valid,
+	.best_encoder = tda998x_connector_best_encoder,
+};
+
+static void tda998x_connector_destroy(struct drm_connector *connector)
+{
+	if (!connector->dev)
+		return;
+	drm_sysfs_connector_remove(connector);
+	drm_connector_cleanup(connector);
 }
 
-static struct drm_encoder_slave_funcs tda998x_encoder_funcs = {
-	.set_config = tda998x_encoder_set_config,
-	.destroy = tda998x_encoder_destroy,
-	.dpms = tda998x_encoder_dpms,
-	.save = tda998x_encoder_save,
-	.restore = tda998x_encoder_restore,
-	.mode_fixup = tda998x_encoder_mode_fixup,
-	.mode_valid = tda998x_encoder_mode_valid,
-	.mode_set = tda998x_encoder_mode_set,
-	.detect = tda998x_encoder_detect,
-	.get_modes = tda998x_encoder_get_modes,
-	.create_resources = tda998x_encoder_create_resources,
-	.set_property = tda998x_encoder_set_property,
+static const struct drm_connector_funcs connector_funcs = {
+	.detect = tda998x_connector_detect,
+	.set_property = tda998x_connector_set_property,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.dpms = drm_helper_connector_dpms,
+	.destroy = tda998x_connector_destroy,
 };
 
 /* I2C driver functions */
 
-static int
-tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
+static int tda_bind(struct device *dev, struct device *master, void *data)
 {
+	struct drm_device *drm = data;
+	struct i2c_client *i2c_client = to_i2c_client(dev);
+	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
+	struct drm_connector *connector = &priv->connector;
+	struct drm_encoder *encoder = &priv->encoder;
+	int ret;
+
+	if (!try_module_get(THIS_MODULE)) {
+		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
+		return -EINVAL;
+	}
+
+	ret = drm_connector_init(drm, connector,
+				&connector_funcs,
+				DRM_MODE_CONNECTOR_HDMIA);
+	if (ret < 0)
+		return ret;
+	drm_connector_helper_add(connector, &connector_helper_funcs);
+
+	ret = drm_encoder_init(drm, encoder,
+				&encoder_funcs,
+				DRM_MODE_ENCODER_TMDS);
+
+	encoder->possible_crtcs = 1;	// 1 << lcd_num
+
+	if (ret < 0)
+		goto err;
+	drm_encoder_helper_add(encoder, &encoder_helper_funcs);
+
+	ret = drm_mode_connector_attach_encoder(connector, encoder);
+	if (ret < 0)
+		goto err;
+	connector->encoder = encoder;
+
+	drm_sysfs_connector_add(connector);
+
+	drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
+	ret = drm_object_property_set_value(&connector->base,
+					drm->mode_config.dpms_property,
+					DRM_MODE_DPMS_OFF);
+
+	if (priv->hdmi->irq)
+		connector->polled = DRM_CONNECTOR_POLL_HPD;
+	else
+		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			DRM_CONNECTOR_POLL_DISCONNECT;
 	return 0;
+
+err:
+	if (encoder->dev)
+		drm_encoder_cleanup(encoder);
+	if (connector->dev)
+		drm_connector_cleanup(connector);
+	return ret;
 }
 
-static int
-tda998x_remove(struct i2c_client *client)
+static void tda_unbind(struct device *dev, struct device *master, void *data)
 {
-	return 0;
+	module_put(THIS_MODULE);
 }
 
+static const struct component_ops comp_ops = {
+	.bind = tda_bind,
+	.unbind = tda_unbind,
+};
+
 static int
-tda998x_encoder_init(struct i2c_client *client,
-		    struct drm_device *dev,
-		    struct drm_encoder_slave *encoder_slave)
+tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct tda998x_priv *priv;
 	struct device_node *np = client->dev.of_node;
@@ -1239,6 +1291,8 @@ tda998x_encoder_init(struct i2c_client *client,
 	if (!priv)
 		return -ENOMEM;
 
+	i2c_set_clientdata(client, priv);
+
 	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
 	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
 	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
@@ -1250,13 +1304,8 @@ tda998x_encoder_init(struct i2c_client *client,
 		kfree(priv);
 		return -ENODEV;
 	}
-
-	priv->encoder = &encoder_slave->base;
 	priv->dpms = DRM_MODE_DPMS_OFF;
 
-	encoder_slave->slave_priv = priv;
-	encoder_slave->slave_funcs = &tda998x_encoder_funcs;
-
 	/* wake up the device: */
 	cec_write(priv, REG_CEC_ENAMODS,
 			CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI);
@@ -1340,31 +1389,55 @@ tda998x_encoder_init(struct i2c_client *client,
 	/* enable EDID read irq: */
 	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
-	if (!np)
-		return 0;		/* non-DT */
+	if (np) {				/* if DT */
 
-	/* get the optional video properties */
-	ret = of_property_read_u32(np, "video-ports", &video);
-	if (ret == 0) {
-		priv->vip_cntrl_0 = video >> 16;
-		priv->vip_cntrl_1 = video >> 8;
-		priv->vip_cntrl_2 = video;
+		/* get the optional video properties */
+		ret = of_property_read_u32(np, "video-ports", &video);
+		if (ret == 0) {
+			priv->vip_cntrl_0 = video >> 16;
+			priv->vip_cntrl_1 = video >> 8;
+			priv->vip_cntrl_2 = video;
+		}
+	} else {
+		struct tda998x_encoder_params *params =
+				(struct tda998x_encoder_params)
+						client->dev.platform_data;
+
+		if (params)
+			tda998x_encoder_set_config(priv, params);
 	}
 
+	/* tda998x video component ready */
+	component_add(&client->dev, &comp_ops);
+
 	return 0;
 
 fail:
-	/* if encoder_init fails, the encoder slave is never registered,
-	 * so cleanup here:
-	 */
 	if (priv->cec)
 		i2c_unregister_device(priv->cec);
 	kfree(priv);
-	encoder_slave->slave_priv = NULL;
-	encoder_slave->slave_funcs = NULL;
 	return -ENXIO;
 }
 
+static int
+tda998x_remove(struct i2c_client *client)
+{
+	struct tda998x_priv *priv = i2c_get_clientdata(client);
+
+	/* disable all IRQs and free the IRQ handler */
+	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
+	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+	if (priv->hdmi->irq)
+		free_irq(priv->hdmi->irq, priv);
+
+	component_del(&client->dev, &comp_ops);
+
+	if (priv->cec)
+		i2c_unregister_device(priv->cec);
+	kfree(priv);
+	return 0;
+}
+
 #ifdef CONFIG_OF
 static const struct of_device_id tda998x_dt_ids[] = {
 	{ .compatible = "nxp,tda9989", },
@@ -1381,38 +1454,18 @@ static struct i2c_device_id tda998x_ids[] = {
 };
 MODULE_DEVICE_TABLE(i2c, tda998x_ids);
 
-static struct drm_i2c_encoder_driver tda998x_driver = {
-	.i2c_driver = {
-		.probe = tda998x_probe,
-		.remove = tda998x_remove,
-		.driver = {
-			.name = "tda998x",
-			.of_match_table = of_match_ptr(tda998x_dt_ids),
-		},
-		.id_table = tda998x_ids,
+static struct i2c_driver tda998x_driver = {
+	.probe = tda998x_probe,
+	.remove = tda998x_remove,
+	.driver = {
+		.name = "tda998x",
+		.of_match_table = of_match_ptr(tda998x_dt_ids),
 	},
-	.encoder_init = tda998x_encoder_init,
+	.id_table = tda998x_ids,
 };
 
-/* Module initialization */
-
-static int __init
-tda998x_init(void)
-{
-	DBG("");
-	return drm_i2c_encoder_register(THIS_MODULE, &tda998x_driver);
-}
-
-static void __exit
-tda998x_exit(void)
-{
-	DBG("");
-	drm_i2c_encoder_unregister(&tda998x_driver);
-}
+module_i2c_driver(tda998x_driver);
 
 MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
 MODULE_DESCRIPTION("NXP Semiconductors TDA998X HDMI Encoder");
 MODULE_LICENSE("GPL");
-
-module_init(tda998x_init);
-module_exit(tda998x_exit);
-- 
1.9.1

[PATCH RFC v2 4/6] drm/tilcdc: Change the interface with the tda998x driver

[Jean-Francois Moine]

The tda998x being moved from a 'slave encoder' to a normal DRM
encoder/connector, the tilcdc_slave glue is not needed anymore.

This patch uses the infrastructure for componentised subsystems
for waiting to the tda998x full start and to give it the pointer
to the DRM device.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/tilcdc/Makefile       |   1 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.c   |  85 +++++--
 drivers/gpu/drm/tilcdc/tilcdc_slave.c | 406 ----------------------------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.h |  26 ---
 4 files changed, 68 insertions(+), 450 deletions(-)
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h

diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
index 7d2eefe..44485f9 100644
--- a/drivers/gpu/drm/tilcdc/Makefile
+++ b/drivers/gpu/drm/tilcdc/Makefile
@@ -6,7 +6,6 @@ endif
 tilcdc-y := \
 	tilcdc_crtc.o \
 	tilcdc_tfp410.o \
-	tilcdc_slave.o \
 	tilcdc_panel.o \
 	tilcdc_drv.o
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 171a820..dd6ebd3 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -17,16 +17,16 @@
 
 /* LCDC DRM driver, based on da8xx-fb */
 
+#include <linux/component.h>
+
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"
 #include "tilcdc_tfp410.h"
-#include "tilcdc_slave.h"
 #include "tilcdc_panel.h"
 
 #include "drm_fb_helper.h"
 
 static LIST_HEAD(module_list);
-static bool slave_probing;
 
 void tilcdc_module_init(struct tilcdc_module *mod, const char *name,
 		const struct tilcdc_module_ops *funcs)
@@ -42,11 +42,6 @@ void tilcdc_module_cleanup(struct tilcdc_module *mod)
 	list_del(&mod->list);
 }
 
-void tilcdc_slave_probedefer(bool defered)
-{
-	slave_probing = defered;
-}
-
 static struct of_device_id tilcdc_of_match[];
 
 static struct drm_framebuffer *tilcdc_fb_create(struct drm_device *dev,
@@ -277,6 +272,10 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 
 	platform_set_drvdata(pdev, dev);
 
+	/* initialize the subdevices */
+	ret = component_bind_all(&pdev->dev, dev);
+	if (ret < 0)
+		goto fail;
 
 	list_for_each_entry(mod, &module_list, list) {
 		DBG("%s: preferred_bpp: %d", mod->name, mod->preferred_bpp);
@@ -577,6 +576,66 @@ static const struct dev_pm_ops tilcdc_pm_ops = {
  * Platform driver:
  */
 
+/* component stuff */
+static int of_dev_node_match(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static int tilcdc_add_components(struct device *master, struct master *m)
+{
+	struct device_node *np = master->of_node, *child;
+	int ret;
+
+	/* scan the video ports */
+	child = NULL;
+	for (;;) {
+		struct device_node *endpoint, *port, *i2c_node;
+
+		child = of_get_next_child(np, child);
+		if (!child)
+			break;
+		if (strcmp(child->name, "port") != 0)
+			continue;
+
+		endpoint = of_get_next_child(child, NULL);
+		if (!endpoint) {
+			dev_err(master, "tilcdc: no port endpoint\n");
+			return -EINVAL;
+		}
+		port = of_parse_phandle(endpoint, "remote-endpoint", 0);
+		of_node_put(endpoint);
+		if (!port) {
+			dev_err(master, "ticldc: no remote-endpoint\n");
+			return -EINVAL;
+		}
+		i2c_node = of_get_parent(of_get_parent(port));
+		of_node_put(port);
+		ret = component_master_add_child(m, of_dev_node_match,
+						 i2c_node);
+		of_node_put(i2c_node);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+static int tilcdc_bind(struct device *dev)
+{
+	return drm_platform_init(&tilcdc_driver, to_platform_device(dev));
+}
+
+static void tilcdc_unbind(struct device *dev)
+{
+	drm_put_dev(dev_get_drvdata(dev));
+}
+
+static const struct component_master_ops tilcdc_comp_ops = {
+	.add_components = tilcdc_add_components,
+	.bind = tilcdc_bind,
+	.unbind = tilcdc_unbind,
+};
+
 static int tilcdc_pdev_probe(struct platform_device *pdev)
 {
 	/* bail out early if no DT data: */
@@ -584,18 +643,12 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "device-tree data is missing\n");
 		return -ENXIO;
 	}
-
-	/* defer probing if slave is in deferred probing */
-	if (slave_probing == true)
-		return -EPROBE_DEFER;
-
-	return drm_platform_init(&tilcdc_driver, pdev);
+	return component_master_add(&pdev->dev, &tilcdc_comp_ops);
 }
 
 static int tilcdc_pdev_remove(struct platform_device *pdev)
 {
-	drm_put_dev(platform_get_drvdata(pdev));
-
+	component_master_del(&pdev->dev, &tilcdc_comp_ops);
 	return 0;
 }
 
@@ -620,7 +673,6 @@ static int __init tilcdc_drm_init(void)
 {
 	DBG("init");
 	tilcdc_tfp410_init();
-	tilcdc_slave_init();
 	tilcdc_panel_init();
 	return platform_driver_register(&tilcdc_platform_driver);
 }
@@ -629,7 +681,6 @@ static void __exit tilcdc_drm_fini(void)
 {
 	DBG("fini");
 	tilcdc_tfp410_fini();
-	tilcdc_slave_fini();
 	tilcdc_panel_fini();
 	platform_driver_unregister(&tilcdc_platform_driver);
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
deleted file mode 100644
index 595068b..0000000
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ /dev/null
@@ -1,406 +0,0 @@
-/*
- * Copyright (C) 2012 Texas Instruments
- * Author: Rob Clark <robdclark@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 2 as published by
- * the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#include <linux/i2c.h>
-#include <linux/pinctrl/pinmux.h>
-#include <linux/pinctrl/consumer.h>
-#include <drm/drm_encoder_slave.h>
-
-#include "tilcdc_drv.h"
-
-struct slave_module {
-	struct tilcdc_module base;
-	struct i2c_adapter *i2c;
-};
-#define to_slave_module(x) container_of(x, struct slave_module, base)
-
-static const struct tilcdc_panel_info slave_info = {
-		.bpp                    = 16,
-		.ac_bias                = 255,
-		.ac_bias_intrpt         = 0,
-		.dma_burst_sz           = 16,
-		.fdd                    = 0x80,
-		.tft_alt_mode           = 0,
-		.sync_edge              = 0,
-		.sync_ctrl              = 1,
-		.raster_order           = 0,
-};
-
-
-/*
- * Encoder:
- */
-
-struct slave_encoder {
-	struct drm_encoder_slave base;
-	struct slave_module *mod;
-};
-#define to_slave_encoder(x) container_of(to_encoder_slave(x), struct slave_encoder, base)
-
-static inline struct drm_encoder_slave_funcs *
-get_slave_funcs(struct drm_encoder *enc)
-{
-	return to_encoder_slave(enc)->slave_funcs;
-}
-
-static void slave_encoder_destroy(struct drm_encoder *encoder)
-{
-	struct slave_encoder *slave_encoder = to_slave_encoder(encoder);
-	if (get_slave_funcs(encoder))
-		get_slave_funcs(encoder)->destroy(encoder);
-	drm_encoder_cleanup(encoder);
-	kfree(slave_encoder);
-}
-
-static void slave_encoder_prepare(struct drm_encoder *encoder)
-{
-	drm_i2c_encoder_prepare(encoder);
-	tilcdc_crtc_set_panel_info(encoder->crtc, &slave_info);
-}
-
-static bool slave_encoder_fixup(struct drm_encoder *encoder,
-		const struct drm_display_mode *mode,
-		struct drm_display_mode *adjusted_mode)
-{
-	/*
-	 * tilcdc does not generate VESA-complient sync but aligns
-	 * VS on the second edge of HS instead of first edge.
-	 * We use adjusted_mode, to fixup sync by aligning both rising
-	 * edges and add HSKEW offset to let the slave encoder fix it up.
-	 */
-	adjusted_mode->hskew = mode->hsync_end - mode->hsync_start;
-	adjusted_mode->flags |= DRM_MODE_FLAG_HSKEW;
-
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC) {
-		adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
-		adjusted_mode->flags &= ~DRM_MODE_FLAG_NHSYNC;
-	} else {
-		adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC;
-		adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
-	}
-
-	return drm_i2c_encoder_mode_fixup(encoder, mode, adjusted_mode);
-}
-
-
-static const struct drm_encoder_funcs slave_encoder_funcs = {
-		.destroy        = slave_encoder_destroy,
-};
-
-static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
-		.dpms           = drm_i2c_encoder_dpms,
-		.mode_fixup     = slave_encoder_fixup,
-		.prepare        = slave_encoder_prepare,
-		.commit         = drm_i2c_encoder_commit,
-		.mode_set       = drm_i2c_encoder_mode_set,
-		.save           = drm_i2c_encoder_save,
-		.restore        = drm_i2c_encoder_restore,
-};
-
-static const struct i2c_board_info info = {
-		I2C_BOARD_INFO("tda998x", 0x70)
-};
-
-static struct drm_encoder *slave_encoder_create(struct drm_device *dev,
-		struct slave_module *mod)
-{
-	struct slave_encoder *slave_encoder;
-	struct drm_encoder *encoder;
-	int ret;
-
-	slave_encoder = kzalloc(sizeof(*slave_encoder), GFP_KERNEL);
-	if (!slave_encoder) {
-		dev_err(dev->dev, "allocation failed\n");
-		return NULL;
-	}
-
-	slave_encoder->mod = mod;
-
-	encoder = &slave_encoder->base.base;
-	encoder->possible_crtcs = 1;
-
-	ret = drm_encoder_init(dev, encoder, &slave_encoder_funcs,
-			DRM_MODE_ENCODER_TMDS);
-	if (ret)
-		goto fail;
-
-	drm_encoder_helper_add(encoder, &slave_encoder_helper_funcs);
-
-	ret = drm_i2c_encoder_init(dev, to_encoder_slave(encoder), mod->i2c, &info);
-	if (ret)
-		goto fail;
-
-	return encoder;
-
-fail:
-	slave_encoder_destroy(encoder);
-	return NULL;
-}
-
-/*
- * Connector:
- */
-
-struct slave_connector {
-	struct drm_connector base;
-
-	struct drm_encoder *encoder;  /* our connected encoder */
-	struct slave_module *mod;
-};
-#define to_slave_connector(x) container_of(x, struct slave_connector, base)
-
-static void slave_connector_destroy(struct drm_connector *connector)
-{
-	struct slave_connector *slave_connector = to_slave_connector(connector);
-	drm_connector_cleanup(connector);
-	kfree(slave_connector);
-}
-
-static enum drm_connector_status slave_connector_detect(
-		struct drm_connector *connector,
-		bool force)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	return get_slave_funcs(encoder)->detect(encoder, connector);
-}
-
-static int slave_connector_get_modes(struct drm_connector *connector)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	return get_slave_funcs(encoder)->get_modes(encoder, connector);
-}
-
-static int slave_connector_mode_valid(struct drm_connector *connector,
-		  struct drm_display_mode *mode)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	struct tilcdc_drm_private *priv = connector->dev->dev_private;
-	int ret;
-
-	ret = tilcdc_crtc_mode_valid(priv->crtc, mode);
-	if (ret != MODE_OK)
-		return ret;
-
-	return get_slave_funcs(encoder)->mode_valid(encoder, mode);
-}
-
-static struct drm_encoder *slave_connector_best_encoder(
-		struct drm_connector *connector)
-{
-	struct slave_connector *slave_connector = to_slave_connector(connector);
-	return slave_connector->encoder;
-}
-
-static int slave_connector_set_property(struct drm_connector *connector,
-		struct drm_property *property, uint64_t value)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	return get_slave_funcs(encoder)->set_property(encoder,
-			connector, property, value);
-}
-
-static const struct drm_connector_funcs slave_connector_funcs = {
-	.destroy            = slave_connector_destroy,
-	.dpms               = drm_helper_connector_dpms,
-	.detect             = slave_connector_detect,
-	.fill_modes         = drm_helper_probe_single_connector_modes,
-	.set_property       = slave_connector_set_property,
-};
-
-static const struct drm_connector_helper_funcs slave_connector_helper_funcs = {
-	.get_modes          = slave_connector_get_modes,
-	.mode_valid         = slave_connector_mode_valid,
-	.best_encoder       = slave_connector_best_encoder,
-};
-
-static struct drm_connector *slave_connector_create(struct drm_device *dev,
-		struct slave_module *mod, struct drm_encoder *encoder)
-{
-	struct slave_connector *slave_connector;
-	struct drm_connector *connector;
-	int ret;
-
-	slave_connector = kzalloc(sizeof(*slave_connector), GFP_KERNEL);
-	if (!slave_connector) {
-		dev_err(dev->dev, "allocation failed\n");
-		return NULL;
-	}
-
-	slave_connector->encoder = encoder;
-	slave_connector->mod = mod;
-
-	connector = &slave_connector->base;
-
-	drm_connector_init(dev, connector, &slave_connector_funcs,
-			DRM_MODE_CONNECTOR_HDMIA);
-	drm_connector_helper_add(connector, &slave_connector_helper_funcs);
-
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			DRM_CONNECTOR_POLL_DISCONNECT;
-
-	connector->interlace_allowed = 0;
-	connector->doublescan_allowed = 0;
-
-	get_slave_funcs(encoder)->create_resources(encoder, connector);
-
-	ret = drm_mode_connector_attach_encoder(connector, encoder);
-	if (ret)
-		goto fail;
-
-	drm_sysfs_connector_add(connector);
-
-	return connector;
-
-fail:
-	slave_connector_destroy(connector);
-	return NULL;
-}
-
-/*
- * Module:
- */
-
-static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
-{
-	struct slave_module *slave_mod = to_slave_module(mod);
-	struct tilcdc_drm_private *priv = dev->dev_private;
-	struct drm_encoder *encoder;
-	struct drm_connector *connector;
-
-	encoder = slave_encoder_create(dev, slave_mod);
-	if (!encoder)
-		return -ENOMEM;
-
-	connector = slave_connector_create(dev, slave_mod, encoder);
-	if (!connector)
-		return -ENOMEM;
-
-	priv->encoders[priv->num_encoders++] = encoder;
-	priv->connectors[priv->num_connectors++] = connector;
-
-	return 0;
-}
-
-static void slave_destroy(struct tilcdc_module *mod)
-{
-	struct slave_module *slave_mod = to_slave_module(mod);
-
-	tilcdc_module_cleanup(mod);
-	kfree(slave_mod);
-}
-
-static const struct tilcdc_module_ops slave_module_ops = {
-		.modeset_init = slave_modeset_init,
-		.destroy = slave_destroy,
-};
-
-/*
- * Device:
- */
-
-static struct of_device_id slave_of_match[];
-
-static int slave_probe(struct platform_device *pdev)
-{
-	struct device_node *node = pdev->dev.of_node;
-	struct device_node *i2c_node;
-	struct slave_module *slave_mod;
-	struct tilcdc_module *mod;
-	struct pinctrl *pinctrl;
-	uint32_t i2c_phandle;
-	struct i2c_adapter *slavei2c;
-	int ret = -EINVAL;
-
-	/* bail out early if no DT data: */
-	if (!node) {
-		dev_err(&pdev->dev, "device-tree data is missing\n");
-		return -ENXIO;
-	}
-
-	/* Bail out early if i2c not specified */
-	if (of_property_read_u32(node, "i2c", &i2c_phandle)) {
-		dev_err(&pdev->dev, "could not get i2c bus phandle\n");
-		return ret;
-	}
-
-	i2c_node = of_find_node_by_phandle(i2c_phandle);
-	if (!i2c_node) {
-		dev_err(&pdev->dev, "could not get i2c bus node\n");
-		return ret;
-	}
-
-	/* but defer the probe if it can't be initialized it might come later */
-	slavei2c = of_find_i2c_adapter_by_node(i2c_node);
-	of_node_put(i2c_node);
-
-	if (!slavei2c) {
-		ret = -EPROBE_DEFER;
-		tilcdc_slave_probedefer(true);
-		dev_err(&pdev->dev, "could not get i2c\n");
-		return ret;
-	}
-
-	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
-	if (!slave_mod)
-		return -ENOMEM;
-
-	mod = &slave_mod->base;
-
-	mod->preferred_bpp = slave_info.bpp;
-
-	slave_mod->i2c = slavei2c;
-
-	tilcdc_module_init(mod, "slave", &slave_module_ops);
-
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev, "pins are not configured\n");
-
-	tilcdc_slave_probedefer(false);
-
-	return 0;
-}
-
-static int slave_remove(struct platform_device *pdev)
-{
-	return 0;
-}
-
-static struct of_device_id slave_of_match[] = {
-		{ .compatible = "ti,tilcdc,slave", },
-		{ },
-};
-
-struct platform_driver slave_driver = {
-	.probe = slave_probe,
-	.remove = slave_remove,
-	.driver = {
-		.owner = THIS_MODULE,
-		.name = "slave",
-		.of_match_table = slave_of_match,
-	},
-};
-
-int __init tilcdc_slave_init(void)
-{
-	return platform_driver_register(&slave_driver);
-}
-
-void __exit tilcdc_slave_fini(void)
-{
-	platform_driver_unregister(&slave_driver);
-}
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.h b/drivers/gpu/drm/tilcdc/tilcdc_slave.h
deleted file mode 100644
index 2f85048..0000000
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- * Copyright (C) 2012 Texas Instruments
- * Author: Rob Clark <robdclark@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 2 as published by
- * the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#ifndef __TILCDC_SLAVE_H__
-#define __TILCDC_SLAVE_H__
-
-/* sub-module for i2c slave encoder output */
-
-int tilcdc_slave_init(void);
-void tilcdc_slave_fini(void);
-
-#endif /* __TILCDC_SLAVE_H__ */
-- 
1.9.1

[PATCH RFC v2 4/6] drm/tilcdc: Change the interface with the tda998x driver

[Jean-Francois Moine]

The tda998x being moved from a 'slave encoder' to a normal DRM
encoder/connector, the tilcdc_slave glue is not needed anymore.

This patch uses the infrastructure for componentised subsystems
for waiting to the tda998x full start and to give it the pointer
to the DRM device.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/tilcdc/Makefile       |   1 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.c   |  85 +++++--
 drivers/gpu/drm/tilcdc/tilcdc_slave.c | 406 ----------------------------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.h |  26 ---
 4 files changed, 68 insertions(+), 450 deletions(-)
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h

diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
index 7d2eefe..44485f9 100644
--- a/drivers/gpu/drm/tilcdc/Makefile
+++ b/drivers/gpu/drm/tilcdc/Makefile
@@ -6,7 +6,6 @@ endif
 tilcdc-y := \
 	tilcdc_crtc.o \
 	tilcdc_tfp410.o \
-	tilcdc_slave.o \
 	tilcdc_panel.o \
 	tilcdc_drv.o
 
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 171a820..dd6ebd3 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -17,16 +17,16 @@
 
 /* LCDC DRM driver, based on da8xx-fb */
 
+#include <linux/component.h>
+
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"
 #include "tilcdc_tfp410.h"
-#include "tilcdc_slave.h"
 #include "tilcdc_panel.h"
 
 #include "drm_fb_helper.h"
 
 static LIST_HEAD(module_list);
-static bool slave_probing;
 
 void tilcdc_module_init(struct tilcdc_module *mod, const char *name,
 		const struct tilcdc_module_ops *funcs)
@@ -42,11 +42,6 @@ void tilcdc_module_cleanup(struct tilcdc_module *mod)
 	list_del(&mod->list);
 }
 
-void tilcdc_slave_probedefer(bool defered)
-{
-	slave_probing = defered;
-}
-
 static struct of_device_id tilcdc_of_match[];
 
 static struct drm_framebuffer *tilcdc_fb_create(struct drm_device *dev,
@@ -277,6 +272,10 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 
 	platform_set_drvdata(pdev, dev);
 
+	/* initialize the subdevices */
+	ret = component_bind_all(&pdev->dev, dev);
+	if (ret < 0)
+		goto fail;
 
 	list_for_each_entry(mod, &module_list, list) {
 		DBG("%s: preferred_bpp: %d", mod->name, mod->preferred_bpp);
@@ -577,6 +576,66 @@ static const struct dev_pm_ops tilcdc_pm_ops = {
  * Platform driver:
  */
 
+/* component stuff */
+static int of_dev_node_match(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static int tilcdc_add_components(struct device *master, struct master *m)
+{
+	struct device_node *np = master->of_node, *child;
+	int ret;
+
+	/* scan the video ports */
+	child = NULL;
+	for (;;) {
+		struct device_node *endpoint, *port, *i2c_node;
+
+		child = of_get_next_child(np, child);
+		if (!child)
+			break;
+		if (strcmp(child->name, "port") != 0)
+			continue;
+
+		endpoint = of_get_next_child(child, NULL);
+		if (!endpoint) {
+			dev_err(master, "tilcdc: no port endpoint\n");
+			return -EINVAL;
+		}
+		port = of_parse_phandle(endpoint, "remote-endpoint", 0);
+		of_node_put(endpoint);
+		if (!port) {
+			dev_err(master, "ticldc: no remote-endpoint\n");
+			return -EINVAL;
+		}
+		i2c_node = of_get_parent(of_get_parent(port));
+		of_node_put(port);
+		ret = component_master_add_child(m, of_dev_node_match,
+						 i2c_node);
+		of_node_put(i2c_node);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+static int tilcdc_bind(struct device *dev)
+{
+	return drm_platform_init(&tilcdc_driver, to_platform_device(dev));
+}
+
+static void tilcdc_unbind(struct device *dev)
+{
+	drm_put_dev(dev_get_drvdata(dev));
+}
+
+static const struct component_master_ops tilcdc_comp_ops = {
+	.add_components = tilcdc_add_components,
+	.bind = tilcdc_bind,
+	.unbind = tilcdc_unbind,
+};
+
 static int tilcdc_pdev_probe(struct platform_device *pdev)
 {
 	/* bail out early if no DT data: */
@@ -584,18 +643,12 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "device-tree data is missing\n");
 		return -ENXIO;
 	}
-
-	/* defer probing if slave is in deferred probing */
-	if (slave_probing == true)
-		return -EPROBE_DEFER;
-
-	return drm_platform_init(&tilcdc_driver, pdev);
+	return component_master_add(&pdev->dev, &tilcdc_comp_ops);
 }
 
 static int tilcdc_pdev_remove(struct platform_device *pdev)
 {
-	drm_put_dev(platform_get_drvdata(pdev));
-
+	component_master_del(&pdev->dev, &tilcdc_comp_ops);
 	return 0;
 }
 
@@ -620,7 +673,6 @@ static int __init tilcdc_drm_init(void)
 {
 	DBG("init");
 	tilcdc_tfp410_init();
-	tilcdc_slave_init();
 	tilcdc_panel_init();
 	return platform_driver_register(&tilcdc_platform_driver);
 }
@@ -629,7 +681,6 @@ static void __exit tilcdc_drm_fini(void)
 {
 	DBG("fini");
 	tilcdc_tfp410_fini();
-	tilcdc_slave_fini();
 	tilcdc_panel_fini();
 	platform_driver_unregister(&tilcdc_platform_driver);
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
deleted file mode 100644
index 595068b..0000000
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ /dev/null
@@ -1,406 +0,0 @@
-/*
- * Copyright (C) 2012 Texas Instruments
- * Author: Rob Clark <robdclark@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 2 as published by
- * the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#include <linux/i2c.h>
-#include <linux/pinctrl/pinmux.h>
-#include <linux/pinctrl/consumer.h>
-#include <drm/drm_encoder_slave.h>
-
-#include "tilcdc_drv.h"
-
-struct slave_module {
-	struct tilcdc_module base;
-	struct i2c_adapter *i2c;
-};
-#define to_slave_module(x) container_of(x, struct slave_module, base)
-
-static const struct tilcdc_panel_info slave_info = {
-		.bpp                    = 16,
-		.ac_bias                = 255,
-		.ac_bias_intrpt         = 0,
-		.dma_burst_sz           = 16,
-		.fdd                    = 0x80,
-		.tft_alt_mode           = 0,
-		.sync_edge              = 0,
-		.sync_ctrl              = 1,
-		.raster_order           = 0,
-};
-
-
-/*
- * Encoder:
- */
-
-struct slave_encoder {
-	struct drm_encoder_slave base;
-	struct slave_module *mod;
-};
-#define to_slave_encoder(x) container_of(to_encoder_slave(x), struct slave_encoder, base)
-
-static inline struct drm_encoder_slave_funcs *
-get_slave_funcs(struct drm_encoder *enc)
-{
-	return to_encoder_slave(enc)->slave_funcs;
-}
-
-static void slave_encoder_destroy(struct drm_encoder *encoder)
-{
-	struct slave_encoder *slave_encoder = to_slave_encoder(encoder);
-	if (get_slave_funcs(encoder))
-		get_slave_funcs(encoder)->destroy(encoder);
-	drm_encoder_cleanup(encoder);
-	kfree(slave_encoder);
-}
-
-static void slave_encoder_prepare(struct drm_encoder *encoder)
-{
-	drm_i2c_encoder_prepare(encoder);
-	tilcdc_crtc_set_panel_info(encoder->crtc, &slave_info);
-}
-
-static bool slave_encoder_fixup(struct drm_encoder *encoder,
-		const struct drm_display_mode *mode,
-		struct drm_display_mode *adjusted_mode)
-{
-	/*
-	 * tilcdc does not generate VESA-complient sync but aligns
-	 * VS on the second edge of HS instead of first edge.
-	 * We use adjusted_mode, to fixup sync by aligning both rising
-	 * edges and add HSKEW offset to let the slave encoder fix it up.
-	 */
-	adjusted_mode->hskew = mode->hsync_end - mode->hsync_start;
-	adjusted_mode->flags |= DRM_MODE_FLAG_HSKEW;
-
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC) {
-		adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
-		adjusted_mode->flags &= ~DRM_MODE_FLAG_NHSYNC;
-	} else {
-		adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC;
-		adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
-	}
-
-	return drm_i2c_encoder_mode_fixup(encoder, mode, adjusted_mode);
-}
-
-
-static const struct drm_encoder_funcs slave_encoder_funcs = {
-		.destroy        = slave_encoder_destroy,
-};
-
-static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
-		.dpms           = drm_i2c_encoder_dpms,
-		.mode_fixup     = slave_encoder_fixup,
-		.prepare        = slave_encoder_prepare,
-		.commit         = drm_i2c_encoder_commit,
-		.mode_set       = drm_i2c_encoder_mode_set,
-		.save           = drm_i2c_encoder_save,
-		.restore        = drm_i2c_encoder_restore,
-};
-
-static const struct i2c_board_info info = {
-		I2C_BOARD_INFO("tda998x", 0x70)
-};
-
-static struct drm_encoder *slave_encoder_create(struct drm_device *dev,
-		struct slave_module *mod)
-{
-	struct slave_encoder *slave_encoder;
-	struct drm_encoder *encoder;
-	int ret;
-
-	slave_encoder = kzalloc(sizeof(*slave_encoder), GFP_KERNEL);
-	if (!slave_encoder) {
-		dev_err(dev->dev, "allocation failed\n");
-		return NULL;
-	}
-
-	slave_encoder->mod = mod;
-
-	encoder = &slave_encoder->base.base;
-	encoder->possible_crtcs = 1;
-
-	ret = drm_encoder_init(dev, encoder, &slave_encoder_funcs,
-			DRM_MODE_ENCODER_TMDS);
-	if (ret)
-		goto fail;
-
-	drm_encoder_helper_add(encoder, &slave_encoder_helper_funcs);
-
-	ret = drm_i2c_encoder_init(dev, to_encoder_slave(encoder), mod->i2c, &info);
-	if (ret)
-		goto fail;
-
-	return encoder;
-
-fail:
-	slave_encoder_destroy(encoder);
-	return NULL;
-}
-
-/*
- * Connector:
- */
-
-struct slave_connector {
-	struct drm_connector base;
-
-	struct drm_encoder *encoder;  /* our connected encoder */
-	struct slave_module *mod;
-};
-#define to_slave_connector(x) container_of(x, struct slave_connector, base)
-
-static void slave_connector_destroy(struct drm_connector *connector)
-{
-	struct slave_connector *slave_connector = to_slave_connector(connector);
-	drm_connector_cleanup(connector);
-	kfree(slave_connector);
-}
-
-static enum drm_connector_status slave_connector_detect(
-		struct drm_connector *connector,
-		bool force)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	return get_slave_funcs(encoder)->detect(encoder, connector);
-}
-
-static int slave_connector_get_modes(struct drm_connector *connector)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	return get_slave_funcs(encoder)->get_modes(encoder, connector);
-}
-
-static int slave_connector_mode_valid(struct drm_connector *connector,
-		  struct drm_display_mode *mode)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	struct tilcdc_drm_private *priv = connector->dev->dev_private;
-	int ret;
-
-	ret = tilcdc_crtc_mode_valid(priv->crtc, mode);
-	if (ret != MODE_OK)
-		return ret;
-
-	return get_slave_funcs(encoder)->mode_valid(encoder, mode);
-}
-
-static struct drm_encoder *slave_connector_best_encoder(
-		struct drm_connector *connector)
-{
-	struct slave_connector *slave_connector = to_slave_connector(connector);
-	return slave_connector->encoder;
-}
-
-static int slave_connector_set_property(struct drm_connector *connector,
-		struct drm_property *property, uint64_t value)
-{
-	struct drm_encoder *encoder = to_slave_connector(connector)->encoder;
-	return get_slave_funcs(encoder)->set_property(encoder,
-			connector, property, value);
-}
-
-static const struct drm_connector_funcs slave_connector_funcs = {
-	.destroy            = slave_connector_destroy,
-	.dpms               = drm_helper_connector_dpms,
-	.detect             = slave_connector_detect,
-	.fill_modes         = drm_helper_probe_single_connector_modes,
-	.set_property       = slave_connector_set_property,
-};
-
-static const struct drm_connector_helper_funcs slave_connector_helper_funcs = {
-	.get_modes          = slave_connector_get_modes,
-	.mode_valid         = slave_connector_mode_valid,
-	.best_encoder       = slave_connector_best_encoder,
-};
-
-static struct drm_connector *slave_connector_create(struct drm_device *dev,
-		struct slave_module *mod, struct drm_encoder *encoder)
-{
-	struct slave_connector *slave_connector;
-	struct drm_connector *connector;
-	int ret;
-
-	slave_connector = kzalloc(sizeof(*slave_connector), GFP_KERNEL);
-	if (!slave_connector) {
-		dev_err(dev->dev, "allocation failed\n");
-		return NULL;
-	}
-
-	slave_connector->encoder = encoder;
-	slave_connector->mod = mod;
-
-	connector = &slave_connector->base;
-
-	drm_connector_init(dev, connector, &slave_connector_funcs,
-			DRM_MODE_CONNECTOR_HDMIA);
-	drm_connector_helper_add(connector, &slave_connector_helper_funcs);
-
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			DRM_CONNECTOR_POLL_DISCONNECT;
-
-	connector->interlace_allowed = 0;
-	connector->doublescan_allowed = 0;
-
-	get_slave_funcs(encoder)->create_resources(encoder, connector);
-
-	ret = drm_mode_connector_attach_encoder(connector, encoder);
-	if (ret)
-		goto fail;
-
-	drm_sysfs_connector_add(connector);
-
-	return connector;
-
-fail:
-	slave_connector_destroy(connector);
-	return NULL;
-}
-
-/*
- * Module:
- */
-
-static int slave_modeset_init(struct tilcdc_module *mod, struct drm_device *dev)
-{
-	struct slave_module *slave_mod = to_slave_module(mod);
-	struct tilcdc_drm_private *priv = dev->dev_private;
-	struct drm_encoder *encoder;
-	struct drm_connector *connector;
-
-	encoder = slave_encoder_create(dev, slave_mod);
-	if (!encoder)
-		return -ENOMEM;
-
-	connector = slave_connector_create(dev, slave_mod, encoder);
-	if (!connector)
-		return -ENOMEM;
-
-	priv->encoders[priv->num_encoders++] = encoder;
-	priv->connectors[priv->num_connectors++] = connector;
-
-	return 0;
-}
-
-static void slave_destroy(struct tilcdc_module *mod)
-{
-	struct slave_module *slave_mod = to_slave_module(mod);
-
-	tilcdc_module_cleanup(mod);
-	kfree(slave_mod);
-}
-
-static const struct tilcdc_module_ops slave_module_ops = {
-		.modeset_init = slave_modeset_init,
-		.destroy = slave_destroy,
-};
-
-/*
- * Device:
- */
-
-static struct of_device_id slave_of_match[];
-
-static int slave_probe(struct platform_device *pdev)
-{
-	struct device_node *node = pdev->dev.of_node;
-	struct device_node *i2c_node;
-	struct slave_module *slave_mod;
-	struct tilcdc_module *mod;
-	struct pinctrl *pinctrl;
-	uint32_t i2c_phandle;
-	struct i2c_adapter *slavei2c;
-	int ret = -EINVAL;
-
-	/* bail out early if no DT data: */
-	if (!node) {
-		dev_err(&pdev->dev, "device-tree data is missing\n");
-		return -ENXIO;
-	}
-
-	/* Bail out early if i2c not specified */
-	if (of_property_read_u32(node, "i2c", &i2c_phandle)) {
-		dev_err(&pdev->dev, "could not get i2c bus phandle\n");
-		return ret;
-	}
-
-	i2c_node = of_find_node_by_phandle(i2c_phandle);
-	if (!i2c_node) {
-		dev_err(&pdev->dev, "could not get i2c bus node\n");
-		return ret;
-	}
-
-	/* but defer the probe if it can't be initialized it might come later */
-	slavei2c = of_find_i2c_adapter_by_node(i2c_node);
-	of_node_put(i2c_node);
-
-	if (!slavei2c) {
-		ret = -EPROBE_DEFER;
-		tilcdc_slave_probedefer(true);
-		dev_err(&pdev->dev, "could not get i2c\n");
-		return ret;
-	}
-
-	slave_mod = kzalloc(sizeof(*slave_mod), GFP_KERNEL);
-	if (!slave_mod)
-		return -ENOMEM;
-
-	mod = &slave_mod->base;
-
-	mod->preferred_bpp = slave_info.bpp;
-
-	slave_mod->i2c = slavei2c;
-
-	tilcdc_module_init(mod, "slave", &slave_module_ops);
-
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev, "pins are not configured\n");
-
-	tilcdc_slave_probedefer(false);
-
-	return 0;
-}
-
-static int slave_remove(struct platform_device *pdev)
-{
-	return 0;
-}
-
-static struct of_device_id slave_of_match[] = {
-		{ .compatible = "ti,tilcdc,slave", },
-		{ },
-};
-
-struct platform_driver slave_driver = {
-	.probe = slave_probe,
-	.remove = slave_remove,
-	.driver = {
-		.owner = THIS_MODULE,
-		.name = "slave",
-		.of_match_table = slave_of_match,
-	},
-};
-
-int __init tilcdc_slave_init(void)
-{
-	return platform_driver_register(&slave_driver);
-}
-
-void __exit tilcdc_slave_fini(void)
-{
-	platform_driver_unregister(&slave_driver);
-}
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.h b/drivers/gpu/drm/tilcdc/tilcdc_slave.h
deleted file mode 100644
index 2f85048..0000000
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- * Copyright (C) 2012 Texas Instruments
- * Author: Rob Clark <robdclark@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 2 as published by
- * the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#ifndef __TILCDC_SLAVE_H__
-#define __TILCDC_SLAVE_H__
-
-/* sub-module for i2c slave encoder output */
-
-int tilcdc_slave_init(void);
-void tilcdc_slave_fini(void);
-
-#endif /* __TILCDC_SLAVE_H__ */
-- 
1.9.1

[PATCH RFC v2 6/6] ARM: AM33XX: dts: Change the tda998x description

[Jean-Francois Moine]

The tda998x being moved from a 'slave encoder' to a normal DRM
encoder/connector and the tilcdc_slave glue being removed, the
declaration of the HDMI transmitter description must be changed in
the associated DTs.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 arch/arm/boot/dts/am335x-base0033.dts  | 28 +++++++++++++++++++---------
 arch/arm/boot/dts/am335x-boneblack.dts | 21 ++++++++++++++++-----
 2 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-base0033.dts b/arch/arm/boot/dts/am335x-base0033.dts
index 72a9b3f..05f2b8f 100644
--- a/arch/arm/boot/dts/am335x-base0033.dts
+++ b/arch/arm/boot/dts/am335x-base0033.dts
@@ -14,15 +14,6 @@
 	model = "IGEP COM AM335x on AQUILA Expansion";
 	compatible = "isee,am335x-base0033", "isee,am335x-igep0033", "ti,am33xx";
 
-	hdmi {
-		compatible = "ti,tilcdc,slave";
-		i2c = <&i2c0>;
-		pinctrl-names = "default", "off";
-		pinctrl-0 = <&nxp_hdmi_pins>;
-		pinctrl-1 = <&nxp_hdmi_off_pins>;
-		status = "okay";
-	};
-
 	leds_base {
 		pinctrl-names = "default";
 		pinctrl-0 = <&leds_base_pins>;
@@ -85,6 +76,11 @@
 
 &lcdc {
 	status = "okay";
+	port {
+		lcd_0: endpoint@0 {
+			remote-endpoint = <&hdmi_0>;
+		};
+	};
 };
 
 &i2c0 {
@@ -92,4 +88,18 @@
 		compatible = "at,24c256";
 		reg = <0x50>;
 	};
+	hdmi: hdmi-encoder {
+		compatible = "nxp,tda19988";
+		reg = <0x70>;
+
+		pinctrl-names = "default", "off";
+		pinctrl-0 = <&nxp_hdmi_pins>;
+		pinctrl-1 = <&nxp_hdmi_off_pins>;
+
+		port {
+			hdmi_0: endpoint@0 {
+				remote-endpoint = <&lcd_0>;
+			};
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
index 6b71ad9..b94d8bd 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -64,15 +64,26 @@
 
 &lcdc {
 	status = "okay";
+	port {
+		lcd_0: endpoint@0 {
+			remote-endpoint = <&hdmi_0>;
+		};
+	};
 };
 
-/ {
-	hdmi {
-		compatible = "ti,tilcdc,slave";
-		i2c = <&i2c0>;
+&i2c0 {
+	hdmi: hdmi-encoder {
+		compatible = "nxp,tda19988";
+		reg = <0x70>;
+
 		pinctrl-names = "default", "off";
 		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
 		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
-		status = "okay";
+
+		port {
+			hdmi_0: endpoint@0 {
+				remote-endpoint = <&lcd_0>;
+			};
+		};
 	};
 };
-- 
1.9.1

[PATCH RFC v2 6/6] ARM: AM33XX: dts: Change the tda998x description

[Jean-Francois Moine]

The tda998x being moved from a 'slave encoder' to a normal DRM
encoder/connector and the tilcdc_slave glue being removed, the
declaration of the HDMI transmitter description must be changed in
the associated DTs.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 arch/arm/boot/dts/am335x-base0033.dts  | 28 +++++++++++++++++++---------
 arch/arm/boot/dts/am335x-boneblack.dts | 21 ++++++++++++++++-----
 2 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-base0033.dts b/arch/arm/boot/dts/am335x-base0033.dts
index 72a9b3f..05f2b8f 100644
--- a/arch/arm/boot/dts/am335x-base0033.dts
+++ b/arch/arm/boot/dts/am335x-base0033.dts
@@ -14,15 +14,6 @@
 	model = "IGEP COM AM335x on AQUILA Expansion";
 	compatible = "isee,am335x-base0033", "isee,am335x-igep0033", "ti,am33xx";
 
-	hdmi {
-		compatible = "ti,tilcdc,slave";
-		i2c = <&i2c0>;
-		pinctrl-names = "default", "off";
-		pinctrl-0 = <&nxp_hdmi_pins>;
-		pinctrl-1 = <&nxp_hdmi_off_pins>;
-		status = "okay";
-	};
-
 	leds_base {
 		pinctrl-names = "default";
 		pinctrl-0 = <&leds_base_pins>;
@@ -85,6 +76,11 @@
 
 &lcdc {
 	status = "okay";
+	port {
+		lcd_0: endpoint at 0 {
+			remote-endpoint = <&hdmi_0>;
+		};
+	};
 };
 
 &i2c0 {
@@ -92,4 +88,18 @@
 		compatible = "at,24c256";
 		reg = <0x50>;
 	};
+	hdmi: hdmi-encoder {
+		compatible = "nxp,tda19988";
+		reg = <0x70>;
+
+		pinctrl-names = "default", "off";
+		pinctrl-0 = <&nxp_hdmi_pins>;
+		pinctrl-1 = <&nxp_hdmi_off_pins>;
+
+		port {
+			hdmi_0: endpoint at 0 {
+				remote-endpoint = <&lcd_0>;
+			};
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
index 6b71ad9..b94d8bd 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -64,15 +64,26 @@
 
 &lcdc {
 	status = "okay";
+	port {
+		lcd_0: endpoint at 0 {
+			remote-endpoint = <&hdmi_0>;
+		};
+	};
 };
 
-/ {
-	hdmi {
-		compatible = "ti,tilcdc,slave";
-		i2c = <&i2c0>;
+&i2c0 {
+	hdmi: hdmi-encoder {
+		compatible = "nxp,tda19988";
+		reg = <0x70>;
+
 		pinctrl-names = "default", "off";
 		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
 		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
-		status = "okay";
+
+		port {
+			hdmi_0: endpoint at 0 {
+				remote-endpoint = <&lcd_0>;
+			};
+		};
 	};
 };
-- 
1.9.1

[PATCH RFC v2 3/6] drm/tilcd: dts: Add the video output port

[Jean-Francois Moine]

The connection between the video source and sink must follow
the media video interface.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt b/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
index fff10da..d0de848 100644
--- a/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
+++ b/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
@@ -18,6 +18,12 @@ Optional properties:
  - max-pixelclock: The maximum pixel clock that can be supported
    by the lcd controller in KHz.
 
+Optional nodes:
+
+ - port: reference of the video sink as described in media/video-interfaces.
+   This reference is required when the video sink is the TDA19988 HDMI
+   transmitter.
+
 Example:
 
 	fb: fb@4830e000 {
@@ -27,3 +33,11 @@ Example:
 		interrupts = <36>;
 		ti,hwmods = "lcdc";
 	};
+
+	&fb {
+		port {
+			lcd_0: endpoint@0 {
+				remote-endpoint = <&hdmi_0>;
+			};
+		};
+	};
-- 
1.9.1

[PATCH RFC v2 5/6] drm/tilcd: dts: Remove unused slave description

[Jean-Francois Moine]

The tda998x being converted to a normal DRM encoder/connector,
there is no slave notion anymore.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 Documentation/devicetree/bindings/drm/tilcdc/slave.txt | 18 ------------------
 1 file changed, 18 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/drm/tilcdc/slave.txt

diff --git a/Documentation/devicetree/bindings/drm/tilcdc/slave.txt b/Documentation/devicetree/bindings/drm/tilcdc/slave.txt
deleted file mode 100644
index 3d2c524..0000000
--- a/Documentation/devicetree/bindings/drm/tilcdc/slave.txt
+++ /dev/null
@@ -1,18 +0,0 @@
-Device-Tree bindings for tilcdc DRM encoder slave output driver
-
-Required properties:
- - compatible: value should be "ti,tilcdc,slave".
- - i2c: the phandle for the i2c device the encoder slave is connected to
-
-Recommended properties:
- - pinctrl-names, pinctrl-0: the pincontrol settings to configure
-   muxing properly for pins that connect to TFP410 device
-
-Example:
-
-	hdmi {
-		compatible = "ti,tilcdc,slave";
-		i2c = <&i2c0>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
-	};
-- 
1.9.1

[PATCH RFC v2 3/6] drm/tilcd: dts: Add the video output port

[Jean-Francois Moine]

The connection between the video source and sink must follow
the media video interface.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt b/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
index fff10da..d0de848 100644
--- a/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
+++ b/Documentation/devicetree/bindings/drm/tilcdc/tilcdc.txt
@@ -18,6 +18,12 @@ Optional properties:
  - max-pixelclock: The maximum pixel clock that can be supported
    by the lcd controller in KHz.
 
+Optional nodes:
+
+ - port: reference of the video sink as described in media/video-interfaces.
+   This reference is required when the video sink is the TDA19988 HDMI
+   transmitter.
+
 Example:
 
 	fb: fb at 4830e000 {
@@ -27,3 +33,11 @@ Example:
 		interrupts = <36>;
 		ti,hwmods = "lcdc";
 	};
+
+	&fb {
+		port {
+			lcd_0: endpoint at 0 {
+				remote-endpoint = <&hdmi_0>;
+			};
+		};
+	};
-- 
1.9.1

[PATCH RFC v2 5/6] drm/tilcd: dts: Remove unused slave description

[Jean-Francois Moine]

The tda998x being converted to a normal DRM encoder/connector,
there is no slave notion anymore.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 Documentation/devicetree/bindings/drm/tilcdc/slave.txt | 18 ------------------
 1 file changed, 18 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/drm/tilcdc/slave.txt

diff --git a/Documentation/devicetree/bindings/drm/tilcdc/slave.txt b/Documentation/devicetree/bindings/drm/tilcdc/slave.txt
deleted file mode 100644
index 3d2c524..0000000
--- a/Documentation/devicetree/bindings/drm/tilcdc/slave.txt
+++ /dev/null
@@ -1,18 +0,0 @@
-Device-Tree bindings for tilcdc DRM encoder slave output driver
-
-Required properties:
- - compatible: value should be "ti,tilcdc,slave".
- - i2c: the phandle for the i2c device the encoder slave is connected to
-
-Recommended properties:
- - pinctrl-names, pinctrl-0: the pincontrol settings to configure
-   muxing properly for pins that connect to TFP410 device
-
-Example:
-
-	hdmi {
-		compatible = "ti,tilcdc,slave";
-		i2c = <&i2c0>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
-	};
-- 
1.9.1

[PATCH RFC v2 1/6] drm/i2c: tda998x: Add the required port property

[Jean-Francois Moine]

According to the media video interface, the video source and sink
ports must be identified by mutual phandles.

This patch adds the 'port' property of the tda998x (sink side).

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 Documentation/devicetree/bindings/drm/i2c/tda998x.txt | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
index e3f3d65..10c5b5e 100644
--- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
+++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
@@ -17,13 +17,22 @@ Optional properties:
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
 
+Required nodes:
+  - port: reference of the video source as described in media/video-interfaces
+
 Example:
 
-	tda998x: hdmi-encoder {
+	hdmi: hdmi-encoder {
 		compatible = "nxp,tda19988";
 		reg = <0x70>;
 		interrupt-parent = <&gpio0>;
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+
+		port {
+			hdmi_0: endpoint@0 {
+				remote-endpoint = <&lcd0_0>;
+			};
+		};
 	};
-- 
1.9.1

[PATCH RFC v2 1/6] drm/i2c: tda998x: Add the required port property

[Jean-Francois Moine]

According to the media video interface, the video source and sink
ports must be identified by mutual phandles.

This patch adds the 'port' property of the tda998x (sink side).

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 Documentation/devicetree/bindings/drm/i2c/tda998x.txt | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
index e3f3d65..10c5b5e 100644
--- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
+++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
@@ -17,13 +17,22 @@ Optional properties:
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
 
+Required nodes:
+  - port: reference of the video source as described in media/video-interfaces
+
 Example:
 
-	tda998x: hdmi-encoder {
+	hdmi: hdmi-encoder {
 		compatible = "nxp,tda19988";
 		reg = <0x70>;
 		interrupt-parent = <&gpio0>;
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+
+		port {
+			hdmi_0: endpoint at 0 {
+				remote-endpoint = <&lcd0_0>;
+			};
+		};
 	};
-- 
1.9.1

[PATCH RFC v2 0/6] drm/i2c: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

The 'slave encoder' structure of the tda998x driver asks for glue
between the DRM driver and the encoder/connector structures.

Changing the tda998x driver to a simple encoder/connector simplifies
the code of the tilcdc driver. This change is permitted by
Russell's infrastructure for componentised subsystems.

The proposed patch set does not include changes to the Armada DRM driver.
These changes should already have been prepared by Russell King, as
told in the message
  https://www.mail-archive.com/linux-media@vger.kernel.org/msg71202.html

The tilcdc part of this patch set has not been tested.

This patch set applies after the patchs:
	drm/i2c: tda998x: Fix lack of required reg in DT documentation
	drm/i2c: tda998x: Change the compatible strings

- v2
	- fix lack of call to component_bind_all() in tilcdc_drv.c
	- add tda998x configuration for non-DT systems

Jean-Francois Moine (6):
  drm/i2c: tda998x: Add the required port property
  drm/i2c: tda998x: Move tda998x to a couple encoder/connector
  drm/tilcd: dts: Add the video output port
  drm/tilcdc: Change the interface with the tda998x driver
  drm/tilcd: dts: Remove unused slave description
  ARM: AM33XX: dts: Change the tda998x description

 .../devicetree/bindings/drm/i2c/tda998x.txt        |  11 +-
 .../devicetree/bindings/drm/tilcdc/slave.txt       |  18 -
 .../devicetree/bindings/drm/tilcdc/tilcdc.txt      |  14 +
 arch/arm/boot/dts/am335x-base0033.dts              |  28 +-
 arch/arm/boot/dts/am335x-boneblack.dts             |  21 +-
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 323 +++++++++-------
 drivers/gpu/drm/tilcdc/Makefile                    |   1 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                |  85 ++++-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c              | 406 ---------------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.h              |  26 --
 10 files changed, 315 insertions(+), 618 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/drm/tilcdc/slave.txt
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h

-- 
1.9.1

[PATCH RFC v2 0/6] drm/i2c: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

The 'slave encoder' structure of the tda998x driver asks for glue
between the DRM driver and the encoder/connector structures.

Changing the tda998x driver to a simple encoder/connector simplifies
the code of the tilcdc driver. This change is permitted by
Russell's infrastructure for componentised subsystems.

The proposed patch set does not include changes to the Armada DRM driver.
These changes should already have been prepared by Russell King, as
told in the message
  https://www.mail-archive.com/linux-media at vger.kernel.org/msg71202.html

The tilcdc part of this patch set has not been tested.

This patch set applies after the patchs:
	drm/i2c: tda998x: Fix lack of required reg in DT documentation
	drm/i2c: tda998x: Change the compatible strings

- v2
	- fix lack of call to component_bind_all() in tilcdc_drv.c
	- add tda998x configuration for non-DT systems

Jean-Francois Moine (6):
  drm/i2c: tda998x: Add the required port property
  drm/i2c: tda998x: Move tda998x to a couple encoder/connector
  drm/tilcd: dts: Add the video output port
  drm/tilcdc: Change the interface with the tda998x driver
  drm/tilcd: dts: Remove unused slave description
  ARM: AM33XX: dts: Change the tda998x description

 .../devicetree/bindings/drm/i2c/tda998x.txt        |  11 +-
 .../devicetree/bindings/drm/tilcdc/slave.txt       |  18 -
 .../devicetree/bindings/drm/tilcdc/tilcdc.txt      |  14 +
 arch/arm/boot/dts/am335x-base0033.dts              |  28 +-
 arch/arm/boot/dts/am335x-boneblack.dts             |  21 +-
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 323 +++++++++-------
 drivers/gpu/drm/tilcdc/Makefile                    |   1 -
 drivers/gpu/drm/tilcdc/tilcdc_drv.c                |  85 ++++-
 drivers/gpu/drm/tilcdc/tilcdc_slave.c              | 406 ---------------------
 drivers/gpu/drm/tilcdc/tilcdc_slave.h              |  26 --
 10 files changed, 315 insertions(+), 618 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/drm/tilcdc/slave.txt
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.c
 delete mode 100644 drivers/gpu/drm/tilcdc/tilcdc_slave.h

-- 
1.9.1

Re: [PATCH RFC v2 0/6] drm/i2c: Move tda998x to a couple encoder/connector

[Russell King - ARM Linux]

On Fri, Mar 21, 2014 at 11:27:45AM +0100, Jean-Francois Moine wrote:
> The 'slave encoder' structure of the tda998x driver asks for glue
> between the DRM driver and the encoder/connector structures.

Given how close we are to the coming merge window, that the discussion
about the of-graph bindings is still going on without concensus being
reached, and that this driver is not in staging, this needs to be
delayed until the following merge window when hopefully we have a
clearer picture about the DT side of this.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH RFC v2 0/6] drm/i2c: Move tda998x to a couple encoder/connector

[Russell King - ARM Linux]

On Fri, Mar 21, 2014 at 11:27:45AM +0100, Jean-Francois Moine wrote:
> The 'slave encoder' structure of the tda998x driver asks for glue
> between the DRM driver and the encoder/connector structures.

Given how close we are to the coming merge window, that the discussion
about the of-graph bindings is still going on without concensus being
reached, and that this driver is not in staging, this needs to be
delayed until the following merge window when hopefully we have a
clearer picture about the DT side of this.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH RFC v2 0/6] drm/i2c: Move tda998x to a couple encoder/connector

[Russell King - ARM Linux]

On Fri, Mar 21, 2014 at 11:27:45AM +0100, Jean-Francois Moine wrote:
> The 'slave encoder' structure of the tda998x driver asks for glue
> between the DRM driver and the encoder/connector structures.

Given how close we are to the coming merge window, that the discussion
about the of-graph bindings is still going on without concensus being
reached, and that this driver is not in staging, this needs to be
delayed until the following merge window when hopefully we have a
clearer picture about the DT side of this.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Laurent Pinchart]

Hi Jean-François,

Thank you for the patch.

On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> The 'slave encoder' structure of the tda998x driver asks for glue
> between the DRM driver and the encoder/connector structures.
> 
> This patch changes the driver to a normal DRM encoder/connector
> thanks to the infrastructure for componentised subsystems.

I like the idea, but I'm not really happy with the implementation. Let me try 
to explain why below.

> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++++++++++++++++++----------------
>  1 file changed, 188 insertions(+), 135 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c

[snip]

> @@ -44,10 +45,14 @@ struct tda998x_priv {
> 
>  	wait_queue_head_t wq_edid;
>  	volatile int wq_edid_wait;
> -	struct drm_encoder *encoder;
> +	struct drm_encoder encoder;
> +	struct drm_connector connector;
>  };

[snip]

> -static int
> -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +static int tda_bind(struct device *dev, struct device *master, void *data)
>  {
> +	struct drm_device *drm = data;

This is the part that bothers me. You're making two assumptions here, that the 
DRM driver will pass a struct drm_device pointer to the bind operation, and 
that the I2C encoder driver can take control of DRM encoder and connector 
creation. Although it could become problematic later, the first assumption 
isn't too much of an issue for now. I'll thus focus on the second one.

The component framework isolate the encoder and DRM master drivers as far as 
component creation and binding is concerned, but doesn't provide a way for the 
two drivers to communicate together (nor should it). You're solving this by 
passing a pointer to the DRM device to the encoder bind operation, making the 
encoder driver create a DRM encoder and connector, and relying on the DRM core 
to orchestrate CRTCs, encoders and connectors. You thus assume that the 
encoder hardware should be represented by a DRM encoder object, and that its 
output is connected to a connector that should be represented by a DRM 
connector object. While this can work in your use case, that won't always hold 
true. Hardware encoders can be chained together, while DRM encoders can't. The 
DRM core has recently received support for bridge objects to overcome that 
limitation. Depending on the hardware topology, a given hardware encoder 
should be modeled as a DRM encoder or as a DRM bridge. That decision shouldn't 
be taken by the encoder driver but by the DRM master driver. The I2C encoder 
driver thus shouldn't create the DRM encoder and DRM connector itself.

I believe the encoder/master communication problem should be solved 
differently. Instead of passing a pointer to the DRM device to the encoder 
driver and making the encoder driver control DRM encoder and connector 
creation, the encoder driver should instead create an object not visible to 
userspace that can be retrieved by the DRM master driver (possibly through 
registration with the DRM core, or by going through drvdata in the encoder's 
struct device). The DRM master could use that object to communicate with the 
encoder, and would register the DRM encoder and DRM connector itself based on 
hardware topology.

> +	struct i2c_client *i2c_client = to_i2c_client(dev);
> +	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> +	struct drm_connector *connector = &priv->connector;
> +	struct drm_encoder *encoder = &priv->encoder;
> +	int ret;
> +
> +	if (!try_module_get(THIS_MODULE)) {
> +		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> +		return -EINVAL;
> +	}
> +
> +	ret = drm_connector_init(drm, connector,
> +				&connector_funcs,
> +				DRM_MODE_CONNECTOR_HDMIA);

This is one example of the shortcomings I've explained above. An encoder 
driver can't always know what connector type it is connected to. If I'm not 
mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be 
up to the master driver to select the connector type based on its overall view 
of the hardware, or even to a connector driver that would be bound to a 
connector DT node (as proposed in https://www.mail-archive.com/devicetree@vger.kernel.org/msg16585.html).

> +	if (ret < 0)
> +		return ret;
> +	drm_connector_helper_add(connector, &connector_helper_funcs);
> +
> +	ret = drm_encoder_init(drm, encoder,
> +				&encoder_funcs,
> +				DRM_MODE_ENCODER_TMDS);
> +
> +	encoder->possible_crtcs = 1;	// 1 << lcd_num
> +
> +	if (ret < 0)
> +		goto err;
> +	drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> +
> +	ret = drm_mode_connector_attach_encoder(connector, encoder);
> +	if (ret < 0)
> +		goto err;
> +	connector->encoder = encoder;
> +
> +	drm_sysfs_connector_add(connector);
> +
> +	drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> +	ret = drm_object_property_set_value(&connector->base,
> +					drm->mode_config.dpms_property,
> +					DRM_MODE_DPMS_OFF);
> +
> +	if (priv->hdmi->irq)
> +		connector->polled = DRM_CONNECTOR_POLL_HPD;
> +	else
> +		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> +			DRM_CONNECTOR_POLL_DISCONNECT;
>  	return 0;
> +
> +err:
> +	if (encoder->dev)
> +		drm_encoder_cleanup(encoder);
> +	if (connector->dev)
> +		drm_connector_cleanup(connector);
> +	return ret;
>  }
> 
> -static int
> -tda998x_remove(struct i2c_client *client)
> +static void tda_unbind(struct device *dev, struct device *master, void
> *data) {
> -	return 0;
> +	module_put(THIS_MODULE);
>  }
> 
> +static const struct component_ops comp_ops = {
> +	.bind = tda_bind,
> +	.unbind = tda_unbind,
> +};
> +
>  static int
> -tda998x_encoder_init(struct i2c_client *client,
> -		    struct drm_device *dev,
> -		    struct drm_encoder_slave *encoder_slave)
> +tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	struct tda998x_priv *priv;
>  	struct device_node *np = client->dev.of_node;
> @@ -1239,6 +1291,8 @@ tda998x_encoder_init(struct i2c_client *client,
>  	if (!priv)
>  		return -ENOMEM;
> 
> +	i2c_set_clientdata(client, priv);
> +
>  	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
>  	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
>  	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
> @@ -1250,13 +1304,8 @@ tda998x_encoder_init(struct i2c_client *client,
>  		kfree(priv);
>  		return -ENODEV;
>  	}
> -
> -	priv->encoder = &encoder_slave->base;
>  	priv->dpms = DRM_MODE_DPMS_OFF;
> 
> -	encoder_slave->slave_priv = priv;
> -	encoder_slave->slave_funcs = &tda998x_encoder_funcs;
> -
>  	/* wake up the device: */
>  	cec_write(priv, REG_CEC_ENAMODS,
>  			CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI);
> @@ -1340,31 +1389,55 @@ tda998x_encoder_init(struct i2c_client *client,
>  	/* enable EDID read irq: */
>  	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
> 
> -	if (!np)
> -		return 0;		/* non-DT */
> +	if (np) {				/* if DT */
> 
> -	/* get the optional video properties */
> -	ret = of_property_read_u32(np, "video-ports", &video);
> -	if (ret == 0) {
> -		priv->vip_cntrl_0 = video >> 16;
> -		priv->vip_cntrl_1 = video >> 8;
> -		priv->vip_cntrl_2 = video;
> +		/* get the optional video properties */
> +		ret = of_property_read_u32(np, "video-ports", &video);
> +		if (ret == 0) {
> +			priv->vip_cntrl_0 = video >> 16;
> +			priv->vip_cntrl_1 = video >> 8;
> +			priv->vip_cntrl_2 = video;
> +		}
> +	} else {
> +		struct tda998x_encoder_params *params =
> +				(struct tda998x_encoder_params)
> +						client->dev.platform_data;
> +
> +		if (params)
> +			tda998x_encoder_set_config(priv, params);
>  	}
> 
> +	/* tda998x video component ready */
> +	component_add(&client->dev, &comp_ops);
> +
>  	return 0;
> 
>  fail:
> -	/* if encoder_init fails, the encoder slave is never registered,
> -	 * so cleanup here:
> -	 */
>  	if (priv->cec)
>  		i2c_unregister_device(priv->cec);
>  	kfree(priv);
> -	encoder_slave->slave_priv = NULL;
> -	encoder_slave->slave_funcs = NULL;
>  	return -ENXIO;
>  }
> 
> +static int
> +tda998x_remove(struct i2c_client *client)
> +{
> +	struct tda998x_priv *priv = i2c_get_clientdata(client);
> +
> +	/* disable all IRQs and free the IRQ handler */
> +	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
> +	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
> +	if (priv->hdmi->irq)
> +		free_irq(priv->hdmi->irq, priv);
> +
> +	component_del(&client->dev, &comp_ops);
> +
> +	if (priv->cec)
> +		i2c_unregister_device(priv->cec);
> +	kfree(priv);
> +	return 0;
> +}
> +
>  #ifdef CONFIG_OF
>  static const struct of_device_id tda998x_dt_ids[] = {
>  	{ .compatible = "nxp,tda9989", },
> @@ -1381,38 +1454,18 @@ static struct i2c_device_id tda998x_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, tda998x_ids);
> 
> -static struct drm_i2c_encoder_driver tda998x_driver = {
> -	.i2c_driver = {
> -		.probe = tda998x_probe,
> -		.remove = tda998x_remove,
> -		.driver = {
> -			.name = "tda998x",
> -			.of_match_table = of_match_ptr(tda998x_dt_ids),
> -		},
> -		.id_table = tda998x_ids,
> +static struct i2c_driver tda998x_driver = {
> +	.probe = tda998x_probe,
> +	.remove = tda998x_remove,
> +	.driver = {
> +		.name = "tda998x",
> +		.of_match_table = of_match_ptr(tda998x_dt_ids),
>  	},
> -	.encoder_init = tda998x_encoder_init,
> +	.id_table = tda998x_ids,
>  };

-- 
Regards,

Laurent Pinchart

[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Laurent Pinchart]

Hi Jean-Fran?ois,

Thank you for the patch.

On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> The 'slave encoder' structure of the tda998x driver asks for glue
> between the DRM driver and the encoder/connector structures.
> 
> This patch changes the driver to a normal DRM encoder/connector
> thanks to the infrastructure for componentised subsystems.

I like the idea, but I'm not really happy with the implementation. Let me try 
to explain why below.

> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++++++++++++++++++----------------
>  1 file changed, 188 insertions(+), 135 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c

[snip]

> @@ -44,10 +45,14 @@ struct tda998x_priv {
> 
>  	wait_queue_head_t wq_edid;
>  	volatile int wq_edid_wait;
> -	struct drm_encoder *encoder;
> +	struct drm_encoder encoder;
> +	struct drm_connector connector;
>  };

[snip]

> -static int
> -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +static int tda_bind(struct device *dev, struct device *master, void *data)
>  {
> +	struct drm_device *drm = data;

This is the part that bothers me. You're making two assumptions here, that the 
DRM driver will pass a struct drm_device pointer to the bind operation, and 
that the I2C encoder driver can take control of DRM encoder and connector 
creation. Although it could become problematic later, the first assumption 
isn't too much of an issue for now. I'll thus focus on the second one.

The component framework isolate the encoder and DRM master drivers as far as 
component creation and binding is concerned, but doesn't provide a way for the 
two drivers to communicate together (nor should it). You're solving this by 
passing a pointer to the DRM device to the encoder bind operation, making the 
encoder driver create a DRM encoder and connector, and relying on the DRM core 
to orchestrate CRTCs, encoders and connectors. You thus assume that the 
encoder hardware should be represented by a DRM encoder object, and that its 
output is connected to a connector that should be represented by a DRM 
connector object. While this can work in your use case, that won't always hold 
true. Hardware encoders can be chained together, while DRM encoders can't. The 
DRM core has recently received support for bridge objects to overcome that 
limitation. Depending on the hardware topology, a given hardware encoder 
should be modeled as a DRM encoder or as a DRM bridge. That decision shouldn't 
be taken by the encoder driver but by the DRM master driver. The I2C encoder 
driver thus shouldn't create the DRM encoder and DRM connector itself.

I believe the encoder/master communication problem should be solved 
differently. Instead of passing a pointer to the DRM device to the encoder 
driver and making the encoder driver control DRM encoder and connector 
creation, the encoder driver should instead create an object not visible to 
userspace that can be retrieved by the DRM master driver (possibly through 
registration with the DRM core, or by going through drvdata in the encoder's 
struct device). The DRM master could use that object to communicate with the 
encoder, and would register the DRM encoder and DRM connector itself based on 
hardware topology.

> +	struct i2c_client *i2c_client = to_i2c_client(dev);
> +	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> +	struct drm_connector *connector = &priv->connector;
> +	struct drm_encoder *encoder = &priv->encoder;
> +	int ret;
> +
> +	if (!try_module_get(THIS_MODULE)) {
> +		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> +		return -EINVAL;
> +	}
> +
> +	ret = drm_connector_init(drm, connector,
> +				&connector_funcs,
> +				DRM_MODE_CONNECTOR_HDMIA);

This is one example of the shortcomings I've explained above. An encoder 
driver can't always know what connector type it is connected to. If I'm not 
mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be 
up to the master driver to select the connector type based on its overall view 
of the hardware, or even to a connector driver that would be bound to a 
connector DT node (as proposed in https://www.mail-archive.com/devicetree at vger.kernel.org/msg16585.html).

> +	if (ret < 0)
> +		return ret;
> +	drm_connector_helper_add(connector, &connector_helper_funcs);
> +
> +	ret = drm_encoder_init(drm, encoder,
> +				&encoder_funcs,
> +				DRM_MODE_ENCODER_TMDS);
> +
> +	encoder->possible_crtcs = 1;	// 1 << lcd_num
> +
> +	if (ret < 0)
> +		goto err;
> +	drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> +
> +	ret = drm_mode_connector_attach_encoder(connector, encoder);
> +	if (ret < 0)
> +		goto err;
> +	connector->encoder = encoder;
> +
> +	drm_sysfs_connector_add(connector);
> +
> +	drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> +	ret = drm_object_property_set_value(&connector->base,
> +					drm->mode_config.dpms_property,
> +					DRM_MODE_DPMS_OFF);
> +
> +	if (priv->hdmi->irq)
> +		connector->polled = DRM_CONNECTOR_POLL_HPD;
> +	else
> +		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> +			DRM_CONNECTOR_POLL_DISCONNECT;
>  	return 0;
> +
> +err:
> +	if (encoder->dev)
> +		drm_encoder_cleanup(encoder);
> +	if (connector->dev)
> +		drm_connector_cleanup(connector);
> +	return ret;
>  }
> 
> -static int
> -tda998x_remove(struct i2c_client *client)
> +static void tda_unbind(struct device *dev, struct device *master, void
> *data) {
> -	return 0;
> +	module_put(THIS_MODULE);
>  }
> 
> +static const struct component_ops comp_ops = {
> +	.bind = tda_bind,
> +	.unbind = tda_unbind,
> +};
> +
>  static int
> -tda998x_encoder_init(struct i2c_client *client,
> -		    struct drm_device *dev,
> -		    struct drm_encoder_slave *encoder_slave)
> +tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	struct tda998x_priv *priv;
>  	struct device_node *np = client->dev.of_node;
> @@ -1239,6 +1291,8 @@ tda998x_encoder_init(struct i2c_client *client,
>  	if (!priv)
>  		return -ENOMEM;
> 
> +	i2c_set_clientdata(client, priv);
> +
>  	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
>  	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
>  	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
> @@ -1250,13 +1304,8 @@ tda998x_encoder_init(struct i2c_client *client,
>  		kfree(priv);
>  		return -ENODEV;
>  	}
> -
> -	priv->encoder = &encoder_slave->base;
>  	priv->dpms = DRM_MODE_DPMS_OFF;
> 
> -	encoder_slave->slave_priv = priv;
> -	encoder_slave->slave_funcs = &tda998x_encoder_funcs;
> -
>  	/* wake up the device: */
>  	cec_write(priv, REG_CEC_ENAMODS,
>  			CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI);
> @@ -1340,31 +1389,55 @@ tda998x_encoder_init(struct i2c_client *client,
>  	/* enable EDID read irq: */
>  	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
> 
> -	if (!np)
> -		return 0;		/* non-DT */
> +	if (np) {				/* if DT */
> 
> -	/* get the optional video properties */
> -	ret = of_property_read_u32(np, "video-ports", &video);
> -	if (ret == 0) {
> -		priv->vip_cntrl_0 = video >> 16;
> -		priv->vip_cntrl_1 = video >> 8;
> -		priv->vip_cntrl_2 = video;
> +		/* get the optional video properties */
> +		ret = of_property_read_u32(np, "video-ports", &video);
> +		if (ret == 0) {
> +			priv->vip_cntrl_0 = video >> 16;
> +			priv->vip_cntrl_1 = video >> 8;
> +			priv->vip_cntrl_2 = video;
> +		}
> +	} else {
> +		struct tda998x_encoder_params *params =
> +				(struct tda998x_encoder_params)
> +						client->dev.platform_data;
> +
> +		if (params)
> +			tda998x_encoder_set_config(priv, params);
>  	}
> 
> +	/* tda998x video component ready */
> +	component_add(&client->dev, &comp_ops);
> +
>  	return 0;
> 
>  fail:
> -	/* if encoder_init fails, the encoder slave is never registered,
> -	 * so cleanup here:
> -	 */
>  	if (priv->cec)
>  		i2c_unregister_device(priv->cec);
>  	kfree(priv);
> -	encoder_slave->slave_priv = NULL;
> -	encoder_slave->slave_funcs = NULL;
>  	return -ENXIO;
>  }
> 
> +static int
> +tda998x_remove(struct i2c_client *client)
> +{
> +	struct tda998x_priv *priv = i2c_get_clientdata(client);
> +
> +	/* disable all IRQs and free the IRQ handler */
> +	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
> +	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
> +	if (priv->hdmi->irq)
> +		free_irq(priv->hdmi->irq, priv);
> +
> +	component_del(&client->dev, &comp_ops);
> +
> +	if (priv->cec)
> +		i2c_unregister_device(priv->cec);
> +	kfree(priv);
> +	return 0;
> +}
> +
>  #ifdef CONFIG_OF
>  static const struct of_device_id tda998x_dt_ids[] = {
>  	{ .compatible = "nxp,tda9989", },
> @@ -1381,38 +1454,18 @@ static struct i2c_device_id tda998x_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, tda998x_ids);
> 
> -static struct drm_i2c_encoder_driver tda998x_driver = {
> -	.i2c_driver = {
> -		.probe = tda998x_probe,
> -		.remove = tda998x_remove,
> -		.driver = {
> -			.name = "tda998x",
> -			.of_match_table = of_match_ptr(tda998x_dt_ids),
> -		},
> -		.id_table = tda998x_ids,
> +static struct i2c_driver tda998x_driver = {
> +	.probe = tda998x_probe,
> +	.remove = tda998x_remove,
> +	.driver = {
> +		.name = "tda998x",
> +		.of_match_table = of_match_ptr(tda998x_dt_ids),
>  	},
> -	.encoder_init = tda998x_encoder_init,
> +	.id_table = tda998x_ids,
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Laurent Pinchart]

Hi Jean-François,

Thank you for the patch.

On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> The 'slave encoder' structure of the tda998x driver asks for glue
> between the DRM driver and the encoder/connector structures.
> 
> This patch changes the driver to a normal DRM encoder/connector
> thanks to the infrastructure for componentised subsystems.

I like the idea, but I'm not really happy with the implementation. Let me try 
to explain why below.

> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++++++++++++++++++----------------
>  1 file changed, 188 insertions(+), 135 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c

[snip]

> @@ -44,10 +45,14 @@ struct tda998x_priv {
> 
>  	wait_queue_head_t wq_edid;
>  	volatile int wq_edid_wait;
> -	struct drm_encoder *encoder;
> +	struct drm_encoder encoder;
> +	struct drm_connector connector;
>  };

[snip]

> -static int
> -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +static int tda_bind(struct device *dev, struct device *master, void *data)
>  {
> +	struct drm_device *drm = data;

This is the part that bothers me. You're making two assumptions here, that the 
DRM driver will pass a struct drm_device pointer to the bind operation, and 
that the I2C encoder driver can take control of DRM encoder and connector 
creation. Although it could become problematic later, the first assumption 
isn't too much of an issue for now. I'll thus focus on the second one.

The component framework isolate the encoder and DRM master drivers as far as 
component creation and binding is concerned, but doesn't provide a way for the 
two drivers to communicate together (nor should it). You're solving this by 
passing a pointer to the DRM device to the encoder bind operation, making the 
encoder driver create a DRM encoder and connector, and relying on the DRM core 
to orchestrate CRTCs, encoders and connectors. You thus assume that the 
encoder hardware should be represented by a DRM encoder object, and that its 
output is connected to a connector that should be represented by a DRM 
connector object. While this can work in your use case, that won't always hold 
true. Hardware encoders can be chained together, while DRM encoders can't. The 
DRM core has recently received support for bridge objects to overcome that 
limitation. Depending on the hardware topology, a given hardware encoder 
should be modeled as a DRM encoder or as a DRM bridge. That decision shouldn't 
be taken by the encoder driver but by the DRM master driver. The I2C encoder 
driver thus shouldn't create the DRM encoder and DRM connector itself.

I believe the encoder/master communication problem should be solved 
differently. Instead of passing a pointer to the DRM device to the encoder 
driver and making the encoder driver control DRM encoder and connector 
creation, the encoder driver should instead create an object not visible to 
userspace that can be retrieved by the DRM master driver (possibly through 
registration with the DRM core, or by going through drvdata in the encoder's 
struct device). The DRM master could use that object to communicate with the 
encoder, and would register the DRM encoder and DRM connector itself based on 
hardware topology.

> +	struct i2c_client *i2c_client = to_i2c_client(dev);
> +	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> +	struct drm_connector *connector = &priv->connector;
> +	struct drm_encoder *encoder = &priv->encoder;
> +	int ret;
> +
> +	if (!try_module_get(THIS_MODULE)) {
> +		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> +		return -EINVAL;
> +	}
> +
> +	ret = drm_connector_init(drm, connector,
> +				&connector_funcs,
> +				DRM_MODE_CONNECTOR_HDMIA);

This is one example of the shortcomings I've explained above. An encoder 
driver can't always know what connector type it is connected to. If I'm not 
mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be 
up to the master driver to select the connector type based on its overall view 
of the hardware, or even to a connector driver that would be bound to a 
connector DT node (as proposed in https://www.mail-archive.com/devicetree@vger.kernel.org/msg16585.html).

> +	if (ret < 0)
> +		return ret;
> +	drm_connector_helper_add(connector, &connector_helper_funcs);
> +
> +	ret = drm_encoder_init(drm, encoder,
> +				&encoder_funcs,
> +				DRM_MODE_ENCODER_TMDS);
> +
> +	encoder->possible_crtcs = 1;	// 1 << lcd_num
> +
> +	if (ret < 0)
> +		goto err;
> +	drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> +
> +	ret = drm_mode_connector_attach_encoder(connector, encoder);
> +	if (ret < 0)
> +		goto err;
> +	connector->encoder = encoder;
> +
> +	drm_sysfs_connector_add(connector);
> +
> +	drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> +	ret = drm_object_property_set_value(&connector->base,
> +					drm->mode_config.dpms_property,
> +					DRM_MODE_DPMS_OFF);
> +
> +	if (priv->hdmi->irq)
> +		connector->polled = DRM_CONNECTOR_POLL_HPD;
> +	else
> +		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> +			DRM_CONNECTOR_POLL_DISCONNECT;
>  	return 0;
> +
> +err:
> +	if (encoder->dev)
> +		drm_encoder_cleanup(encoder);
> +	if (connector->dev)
> +		drm_connector_cleanup(connector);
> +	return ret;
>  }
> 
> -static int
> -tda998x_remove(struct i2c_client *client)
> +static void tda_unbind(struct device *dev, struct device *master, void
> *data) {
> -	return 0;
> +	module_put(THIS_MODULE);
>  }
> 
> +static const struct component_ops comp_ops = {
> +	.bind = tda_bind,
> +	.unbind = tda_unbind,
> +};
> +
>  static int
> -tda998x_encoder_init(struct i2c_client *client,
> -		    struct drm_device *dev,
> -		    struct drm_encoder_slave *encoder_slave)
> +tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	struct tda998x_priv *priv;
>  	struct device_node *np = client->dev.of_node;
> @@ -1239,6 +1291,8 @@ tda998x_encoder_init(struct i2c_client *client,
>  	if (!priv)
>  		return -ENOMEM;
> 
> +	i2c_set_clientdata(client, priv);
> +
>  	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
>  	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
>  	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
> @@ -1250,13 +1304,8 @@ tda998x_encoder_init(struct i2c_client *client,
>  		kfree(priv);
>  		return -ENODEV;
>  	}
> -
> -	priv->encoder = &encoder_slave->base;
>  	priv->dpms = DRM_MODE_DPMS_OFF;
> 
> -	encoder_slave->slave_priv = priv;
> -	encoder_slave->slave_funcs = &tda998x_encoder_funcs;
> -
>  	/* wake up the device: */
>  	cec_write(priv, REG_CEC_ENAMODS,
>  			CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI);
> @@ -1340,31 +1389,55 @@ tda998x_encoder_init(struct i2c_client *client,
>  	/* enable EDID read irq: */
>  	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
> 
> -	if (!np)
> -		return 0;		/* non-DT */
> +	if (np) {				/* if DT */
> 
> -	/* get the optional video properties */
> -	ret = of_property_read_u32(np, "video-ports", &video);
> -	if (ret == 0) {
> -		priv->vip_cntrl_0 = video >> 16;
> -		priv->vip_cntrl_1 = video >> 8;
> -		priv->vip_cntrl_2 = video;
> +		/* get the optional video properties */
> +		ret = of_property_read_u32(np, "video-ports", &video);
> +		if (ret == 0) {
> +			priv->vip_cntrl_0 = video >> 16;
> +			priv->vip_cntrl_1 = video >> 8;
> +			priv->vip_cntrl_2 = video;
> +		}
> +	} else {
> +		struct tda998x_encoder_params *params =
> +				(struct tda998x_encoder_params)
> +						client->dev.platform_data;
> +
> +		if (params)
> +			tda998x_encoder_set_config(priv, params);
>  	}
> 
> +	/* tda998x video component ready */
> +	component_add(&client->dev, &comp_ops);
> +
>  	return 0;
> 
>  fail:
> -	/* if encoder_init fails, the encoder slave is never registered,
> -	 * so cleanup here:
> -	 */
>  	if (priv->cec)
>  		i2c_unregister_device(priv->cec);
>  	kfree(priv);
> -	encoder_slave->slave_priv = NULL;
> -	encoder_slave->slave_funcs = NULL;
>  	return -ENXIO;
>  }
> 
> +static int
> +tda998x_remove(struct i2c_client *client)
> +{
> +	struct tda998x_priv *priv = i2c_get_clientdata(client);
> +
> +	/* disable all IRQs and free the IRQ handler */
> +	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
> +	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
> +	if (priv->hdmi->irq)
> +		free_irq(priv->hdmi->irq, priv);
> +
> +	component_del(&client->dev, &comp_ops);
> +
> +	if (priv->cec)
> +		i2c_unregister_device(priv->cec);
> +	kfree(priv);
> +	return 0;
> +}
> +
>  #ifdef CONFIG_OF
>  static const struct of_device_id tda998x_dt_ids[] = {
>  	{ .compatible = "nxp,tda9989", },
> @@ -1381,38 +1454,18 @@ static struct i2c_device_id tda998x_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, tda998x_ids);
> 
> -static struct drm_i2c_encoder_driver tda998x_driver = {
> -	.i2c_driver = {
> -		.probe = tda998x_probe,
> -		.remove = tda998x_remove,
> -		.driver = {
> -			.name = "tda998x",
> -			.of_match_table = of_match_ptr(tda998x_dt_ids),
> -		},
> -		.id_table = tda998x_ids,
> +static struct i2c_driver tda998x_driver = {
> +	.probe = tda998x_probe,
> +	.remove = tda998x_remove,
> +	.driver = {
> +		.name = "tda998x",
> +		.of_match_table = of_match_ptr(tda998x_dt_ids),
>  	},
> -	.encoder_init = tda998x_encoder_init,
> +	.id_table = tda998x_ids,
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

On Mon, 24 Mar 2014 23:39:01 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Jean-François,

Hi Laurent,

> Thank you for the patch.
> 
> On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> > The 'slave encoder' structure of the tda998x driver asks for glue
> > between the DRM driver and the encoder/connector structures.
> > 
> > This patch changes the driver to a normal DRM encoder/connector
> > thanks to the infrastructure for componentised subsystems.
> 
> I like the idea, but I'm not really happy with the implementation. Let me try 
> to explain why below.
> 
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > ---
> >  drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++++++++++++++++++----------------
> >  1 file changed, 188 insertions(+), 135 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> 
> [snip]
> 
> > @@ -44,10 +45,14 @@ struct tda998x_priv {
> > 
> >  	wait_queue_head_t wq_edid;
> >  	volatile int wq_edid_wait;
> > -	struct drm_encoder *encoder;
> > +	struct drm_encoder encoder;
> > +	struct drm_connector connector;
> >  };
> 
> [snip]
> 
> > -static int
> > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > +static int tda_bind(struct device *dev, struct device *master, void *data)
> >  {
> > +	struct drm_device *drm = data;
> 
> This is the part that bothers me. You're making two assumptions here, that the 
> DRM driver will pass a struct drm_device pointer to the bind operation, and 
> that the I2C encoder driver can take control of DRM encoder and connector 
> creation. Although it could become problematic later, the first assumption 
> isn't too much of an issue for now. I'll thus focus on the second one.
> 
> The component framework isolate the encoder and DRM master drivers as far as 
> component creation and binding is concerned, but doesn't provide a way for the 
> two drivers to communicate together (nor should it). You're solving this by 
> passing a pointer to the DRM device to the encoder bind operation, making the 
> encoder driver create a DRM encoder and connector, and relying on the DRM core 
> to orchestrate CRTCs, encoders and connectors. You thus assume that the 
> encoder hardware should be represented by a DRM encoder object, and that its 
> output is connected to a connector that should be represented by a DRM 
> connector object. While this can work in your use case, that won't always hold 
> true. Hardware encoders can be chained together, while DRM encoders can't. The 
> DRM core has recently received support for bridge objects to overcome that 
> limitation. Depending on the hardware topology, a given hardware encoder 
> should be modeled as a DRM encoder or as a DRM bridge. That decision shouldn't 
> be taken by the encoder driver but by the DRM master driver. The I2C encoder 
> driver thus shouldn't create the DRM encoder and DRM connector itself.
> 
> I believe the encoder/master communication problem should be solved 
> differently. Instead of passing a pointer to the DRM device to the encoder 
> driver and making the encoder driver control DRM encoder and connector 
> creation, the encoder driver should instead create an object not visible to 
> userspace that can be retrieved by the DRM master driver (possibly through 
> registration with the DRM core, or by going through drvdata in the encoder's 
> struct device). The DRM master could use that object to communicate with the 
> encoder, and would register the DRM encoder and DRM connector itself based on 
> hardware topology.
> 
> > +	struct i2c_client *i2c_client = to_i2c_client(dev);
> > +	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> > +	struct drm_connector *connector = &priv->connector;
> > +	struct drm_encoder *encoder = &priv->encoder;
> > +	int ret;
> > +
> > +	if (!try_module_get(THIS_MODULE)) {
> > +		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = drm_connector_init(drm, connector,
> > +				&connector_funcs,
> > +				DRM_MODE_CONNECTOR_HDMIA);
> 
> This is one example of the shortcomings I've explained above. An encoder 
> driver can't always know what connector type it is connected to. If I'm not 
> mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be 
> up to the master driver to select the connector type based on its overall view 
> of the hardware, or even to a connector driver that would be bound to a 
> connector DT node (as proposed in https://www.mail-archive.com/devicetree@vger.kernel.org/msg16585.html).
	[snip]

The tda998x, as a HDMI transmitter, has to deal with both video and
audio.

Whereas the hardware connection schemes are the same in both worlds,
the way they are translated to computer objects are very different:

- video
	DRM card -> CRTCs -> encoders -> (bridges) -> connectors

- audio
	ALSA card -> CPUs -> (CODECs) -> CODECs

and it would be nice to have a common layout.

Actually, the tda998x is a slave encoder, that is, it plays the roles
of both encoder and connector. In the 2 DRM drivers (armada and tilcdc)
which use it, yes, the encoders and connectors are created by the main
DRM drivers, but, there is no notion of bridge, and, also, the encoder
is DRM_MODE_ENCODER_TMDS and the connector is DRM_MODE_CONNECTOR_HDMIA.
Then, nothing is changed in the global system working.

About the connector, yes, I let its type as hard-coded, but this could
be changed by configuration in the platform data or in the DT. Anyway,
there is nothing as such in the proposed patch

	'Add DT binding documentation for HDMI Connector'

I also dislike this patch because it adds a device which is of no use.
I had a same remark from Mark Brown about a tda998x CODEC proposal of
mine:

	hdmi_codec: hdmi-codec {
		compatible = "nxp,tda998x-codec";
		audio-ports = <0x03>, <0x04>;
	};

So, the next tda998x CODEC will be directly included in the tda998x
driver, the audio output being the HDMI connector. Here is the DT
definition I have for the Cubox:

&i2c0 {
	hdmi: hdmi-encoder {
		compatible = "nxp,tda9989";
		reg = <0x70>;
		interrupt-parent = <&gpio0>;
		interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
		pinctrl-0 = <&pmx_camera>;
		pinctrl-names = "default";

		audio-ports = <0x03>, <0x04>;		/* 2 audio input ports */
		audio-port-names = "i2s", "spdif";
		#sound-dai-cells = <1>;

		port {					/* 1 video input port */
			hdmi_0: endpoint@0 {
				remote-endpoint = <&lcd0_0>;
			};
		};
	};
};

Back to the DRM device pointer given to the tda998x driver, as the
tda998x is an encoder/connector, it has no bridge function, so, there
is no need to add a complex API for information exchange between both
drivers.

Anyway, there is a big lack in my proposal: the tda998x encoder is
hard-coded to the first CRTC. This could be solved by a scan of the DT
and of the encoder list by the DRM driver, but I think that the actual
definitions as proposed by media/video-interfaces.txt are not easy to
use.

Do you think that a description as done for ALSA could work?

A sound card creation is done by a global sound configuration and a
description of the links between the card elements. Here is the tda998x
part of the Cubox audio card ('audio1' is the audio device):

	sound {
		compatible = "simple-audio-card";
		simple-audio-card,name = "Cubox Audio";

		simple-audio-card,dai-link@0 {		/* I2S - HDMI */
			format = "i2s";
			cpu {
				sound-dai = <&audio1 0>;	/* I2S output */
			};
			codec {
				sound-dai = <&hdmi 0>;		/* I2S input */
			};
		};

		simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
			cpu {
				sound-dai = <&audio1 1>;	/* S/PDIF output */
			};
			codec {
				sound-dai = <&hdmi 1>;		/* S/PDIF input */
			};
		};
	};

Using the same elements, here is what could be the video card of the
Armada 510 with a panel, the tda998x and the display controller:

	video {
		compatible = "simple-video-card";

		simple-video-card,dvi-link {
			crtc {
				dvi = <&lcd0>;
			};
			encoder {
				dvi = <&panel>;
				connector-type = 7;	/* LVDS */
			};
		};
		simple-video-card,dvi-link {
			crtc {
				dvi = <&lcd0>;
			};
			encoder {
				dvi = <&hdmi>;
				connector-type = 11;	/* HDMI-A */
			};
		};
	};

	lcd0: lcd-controller@820000 {
		compatible = "marvell,armada-510-lcd";
		... hardware definitions ...
	};

	hdmi : hdmi-encoder {
		.. same as above, but without the video input port ..
	};

	panel: panel {
		.. panel parameters ..
	};

Then, the generic 'simple-video-card' has all elements to create the
DRM device.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

On Mon, 24 Mar 2014 23:39:01 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Jean-Fran?ois,

Hi Laurent,

> Thank you for the patch.
> 
> On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> > The 'slave encoder' structure of the tda998x driver asks for glue
> > between the DRM driver and the encoder/connector structures.
> > 
> > This patch changes the driver to a normal DRM encoder/connector
> > thanks to the infrastructure for componentised subsystems.
> 
> I like the idea, but I'm not really happy with the implementation. Let me try 
> to explain why below.
> 
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > ---
> >  drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++++++++++++++++++----------------
> >  1 file changed, 188 insertions(+), 135 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> 
> [snip]
> 
> > @@ -44,10 +45,14 @@ struct tda998x_priv {
> > 
> >  	wait_queue_head_t wq_edid;
> >  	volatile int wq_edid_wait;
> > -	struct drm_encoder *encoder;
> > +	struct drm_encoder encoder;
> > +	struct drm_connector connector;
> >  };
> 
> [snip]
> 
> > -static int
> > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > +static int tda_bind(struct device *dev, struct device *master, void *data)
> >  {
> > +	struct drm_device *drm = data;
> 
> This is the part that bothers me. You're making two assumptions here, that the 
> DRM driver will pass a struct drm_device pointer to the bind operation, and 
> that the I2C encoder driver can take control of DRM encoder and connector 
> creation. Although it could become problematic later, the first assumption 
> isn't too much of an issue for now. I'll thus focus on the second one.
> 
> The component framework isolate the encoder and DRM master drivers as far as 
> component creation and binding is concerned, but doesn't provide a way for the 
> two drivers to communicate together (nor should it). You're solving this by 
> passing a pointer to the DRM device to the encoder bind operation, making the 
> encoder driver create a DRM encoder and connector, and relying on the DRM core 
> to orchestrate CRTCs, encoders and connectors. You thus assume that the 
> encoder hardware should be represented by a DRM encoder object, and that its 
> output is connected to a connector that should be represented by a DRM 
> connector object. While this can work in your use case, that won't always hold 
> true. Hardware encoders can be chained together, while DRM encoders can't. The 
> DRM core has recently received support for bridge objects to overcome that 
> limitation. Depending on the hardware topology, a given hardware encoder 
> should be modeled as a DRM encoder or as a DRM bridge. That decision shouldn't 
> be taken by the encoder driver but by the DRM master driver. The I2C encoder 
> driver thus shouldn't create the DRM encoder and DRM connector itself.
> 
> I believe the encoder/master communication problem should be solved 
> differently. Instead of passing a pointer to the DRM device to the encoder 
> driver and making the encoder driver control DRM encoder and connector 
> creation, the encoder driver should instead create an object not visible to 
> userspace that can be retrieved by the DRM master driver (possibly through 
> registration with the DRM core, or by going through drvdata in the encoder's 
> struct device). The DRM master could use that object to communicate with the 
> encoder, and would register the DRM encoder and DRM connector itself based on 
> hardware topology.
> 
> > +	struct i2c_client *i2c_client = to_i2c_client(dev);
> > +	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> > +	struct drm_connector *connector = &priv->connector;
> > +	struct drm_encoder *encoder = &priv->encoder;
> > +	int ret;
> > +
> > +	if (!try_module_get(THIS_MODULE)) {
> > +		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = drm_connector_init(drm, connector,
> > +				&connector_funcs,
> > +				DRM_MODE_CONNECTOR_HDMIA);
> 
> This is one example of the shortcomings I've explained above. An encoder 
> driver can't always know what connector type it is connected to. If I'm not 
> mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be 
> up to the master driver to select the connector type based on its overall view 
> of the hardware, or even to a connector driver that would be bound to a 
> connector DT node (as proposed in https://www.mail-archive.com/devicetree at vger.kernel.org/msg16585.html).
	[snip]

The tda998x, as a HDMI transmitter, has to deal with both video and
audio.

Whereas the hardware connection schemes are the same in both worlds,
the way they are translated to computer objects are very different:

- video
	DRM card -> CRTCs -> encoders -> (bridges) -> connectors

- audio
	ALSA card -> CPUs -> (CODECs) -> CODECs

and it would be nice to have a common layout.

Actually, the tda998x is a slave encoder, that is, it plays the roles
of both encoder and connector. In the 2 DRM drivers (armada and tilcdc)
which use it, yes, the encoders and connectors are created by the main
DRM drivers, but, there is no notion of bridge, and, also, the encoder
is DRM_MODE_ENCODER_TMDS and the connector is DRM_MODE_CONNECTOR_HDMIA.
Then, nothing is changed in the global system working.

About the connector, yes, I let its type as hard-coded, but this could
be changed by configuration in the platform data or in the DT. Anyway,
there is nothing as such in the proposed patch

	'Add DT binding documentation for HDMI Connector'

I also dislike this patch because it adds a device which is of no use.
I had a same remark from Mark Brown about a tda998x CODEC proposal of
mine:

	hdmi_codec: hdmi-codec {
		compatible = "nxp,tda998x-codec";
		audio-ports = <0x03>, <0x04>;
	};

So, the next tda998x CODEC will be directly included in the tda998x
driver, the audio output being the HDMI connector. Here is the DT
definition I have for the Cubox:

&i2c0 {
	hdmi: hdmi-encoder {
		compatible = "nxp,tda9989";
		reg = <0x70>;
		interrupt-parent = <&gpio0>;
		interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
		pinctrl-0 = <&pmx_camera>;
		pinctrl-names = "default";

		audio-ports = <0x03>, <0x04>;		/* 2 audio input ports */
		audio-port-names = "i2s", "spdif";
		#sound-dai-cells = <1>;

		port {					/* 1 video input port */
			hdmi_0: endpoint at 0 {
				remote-endpoint = <&lcd0_0>;
			};
		};
	};
};

Back to the DRM device pointer given to the tda998x driver, as the
tda998x is an encoder/connector, it has no bridge function, so, there
is no need to add a complex API for information exchange between both
drivers.

Anyway, there is a big lack in my proposal: the tda998x encoder is
hard-coded to the first CRTC. This could be solved by a scan of the DT
and of the encoder list by the DRM driver, but I think that the actual
definitions as proposed by media/video-interfaces.txt are not easy to
use.

Do you think that a description as done for ALSA could work?

A sound card creation is done by a global sound configuration and a
description of the links between the card elements. Here is the tda998x
part of the Cubox audio card ('audio1' is the audio device):

	sound {
		compatible = "simple-audio-card";
		simple-audio-card,name = "Cubox Audio";

		simple-audio-card,dai-link at 0 {		/* I2S - HDMI */
			format = "i2s";
			cpu {
				sound-dai = <&audio1 0>;	/* I2S output */
			};
			codec {
				sound-dai = <&hdmi 0>;		/* I2S input */
			};
		};

		simple-audio-card,dai-link at 1 {		/* S/PDIF - HDMI */
			cpu {
				sound-dai = <&audio1 1>;	/* S/PDIF output */
			};
			codec {
				sound-dai = <&hdmi 1>;		/* S/PDIF input */
			};
		};
	};

Using the same elements, here is what could be the video card of the
Armada 510 with a panel, the tda998x and the display controller:

	video {
		compatible = "simple-video-card";

		simple-video-card,dvi-link {
			crtc {
				dvi = <&lcd0>;
			};
			encoder {
				dvi = <&panel>;
				connector-type = 7;	/* LVDS */
			};
		};
		simple-video-card,dvi-link {
			crtc {
				dvi = <&lcd0>;
			};
			encoder {
				dvi = <&hdmi>;
				connector-type = 11;	/* HDMI-A */
			};
		};
	};

	lcd0: lcd-controller at 820000 {
		compatible = "marvell,armada-510-lcd";
		... hardware definitions ...
	};

	hdmi : hdmi-encoder {
		.. same as above, but without the video input port ..
	};

	panel: panel {
		.. panel parameters ..
	};

Then, the generic 'simple-video-card' has all elements to create the
DRM device.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

On Mon, 24 Mar 2014 23:39:01 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Jean-François,

Hi Laurent,

> Thank you for the patch.
> 
> On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> > The 'slave encoder' structure of the tda998x driver asks for glue
> > between the DRM driver and the encoder/connector structures.
> > 
> > This patch changes the driver to a normal DRM encoder/connector
> > thanks to the infrastructure for componentised subsystems.
> 
> I like the idea, but I'm not really happy with the implementation. Let me try 
> to explain why below.
> 
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > ---
> >  drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++++++++++++++++++----------------
> >  1 file changed, 188 insertions(+), 135 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> 
> [snip]
> 
> > @@ -44,10 +45,14 @@ struct tda998x_priv {
> > 
> >  	wait_queue_head_t wq_edid;
> >  	volatile int wq_edid_wait;
> > -	struct drm_encoder *encoder;
> > +	struct drm_encoder encoder;
> > +	struct drm_connector connector;
> >  };
> 
> [snip]
> 
> > -static int
> > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > +static int tda_bind(struct device *dev, struct device *master, void *data)
> >  {
> > +	struct drm_device *drm = data;
> 
> This is the part that bothers me. You're making two assumptions here, that the 
> DRM driver will pass a struct drm_device pointer to the bind operation, and 
> that the I2C encoder driver can take control of DRM encoder and connector 
> creation. Although it could become problematic later, the first assumption 
> isn't too much of an issue for now. I'll thus focus on the second one.
> 
> The component framework isolate the encoder and DRM master drivers as far as 
> component creation and binding is concerned, but doesn't provide a way for the 
> two drivers to communicate together (nor should it). You're solving this by 
> passing a pointer to the DRM device to the encoder bind operation, making the 
> encoder driver create a DRM encoder and connector, and relying on the DRM core 
> to orchestrate CRTCs, encoders and connectors. You thus assume that the 
> encoder hardware should be represented by a DRM encoder object, and that its 
> output is connected to a connector that should be represented by a DRM 
> connector object. While this can work in your use case, that won't always hold 
> true. Hardware encoders can be chained together, while DRM encoders can't. The 
> DRM core has recently received support for bridge objects to overcome that 
> limitation. Depending on the hardware topology, a given hardware encoder 
> should be modeled as a DRM encoder or as a DRM bridge. That decision shouldn't 
> be taken by the encoder driver but by the DRM master driver. The I2C encoder 
> driver thus shouldn't create the DRM encoder and DRM connector itself.
> 
> I believe the encoder/master communication problem should be solved 
> differently. Instead of passing a pointer to the DRM device to the encoder 
> driver and making the encoder driver control DRM encoder and connector 
> creation, the encoder driver should instead create an object not visible to 
> userspace that can be retrieved by the DRM master driver (possibly through 
> registration with the DRM core, or by going through drvdata in the encoder's 
> struct device). The DRM master could use that object to communicate with the 
> encoder, and would register the DRM encoder and DRM connector itself based on 
> hardware topology.
> 
> > +	struct i2c_client *i2c_client = to_i2c_client(dev);
> > +	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> > +	struct drm_connector *connector = &priv->connector;
> > +	struct drm_encoder *encoder = &priv->encoder;
> > +	int ret;
> > +
> > +	if (!try_module_get(THIS_MODULE)) {
> > +		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = drm_connector_init(drm, connector,
> > +				&connector_funcs,
> > +				DRM_MODE_CONNECTOR_HDMIA);
> 
> This is one example of the shortcomings I've explained above. An encoder 
> driver can't always know what connector type it is connected to. If I'm not 
> mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be 
> up to the master driver to select the connector type based on its overall view 
> of the hardware, or even to a connector driver that would be bound to a 
> connector DT node (as proposed in https://www.mail-archive.com/devicetree@vger.kernel.org/msg16585.html).
	[snip]

The tda998x, as a HDMI transmitter, has to deal with both video and
audio.

Whereas the hardware connection schemes are the same in both worlds,
the way they are translated to computer objects are very different:

- video
	DRM card -> CRTCs -> encoders -> (bridges) -> connectors

- audio
	ALSA card -> CPUs -> (CODECs) -> CODECs

and it would be nice to have a common layout.

Actually, the tda998x is a slave encoder, that is, it plays the roles
of both encoder and connector. In the 2 DRM drivers (armada and tilcdc)
which use it, yes, the encoders and connectors are created by the main
DRM drivers, but, there is no notion of bridge, and, also, the encoder
is DRM_MODE_ENCODER_TMDS and the connector is DRM_MODE_CONNECTOR_HDMIA.
Then, nothing is changed in the global system working.

About the connector, yes, I let its type as hard-coded, but this could
be changed by configuration in the platform data or in the DT. Anyway,
there is nothing as such in the proposed patch

	'Add DT binding documentation for HDMI Connector'

I also dislike this patch because it adds a device which is of no use.
I had a same remark from Mark Brown about a tda998x CODEC proposal of
mine:

	hdmi_codec: hdmi-codec {
		compatible = "nxp,tda998x-codec";
		audio-ports = <0x03>, <0x04>;
	};

So, the next tda998x CODEC will be directly included in the tda998x
driver, the audio output being the HDMI connector. Here is the DT
definition I have for the Cubox:

&i2c0 {
	hdmi: hdmi-encoder {
		compatible = "nxp,tda9989";
		reg = <0x70>;
		interrupt-parent = <&gpio0>;
		interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
		pinctrl-0 = <&pmx_camera>;
		pinctrl-names = "default";

		audio-ports = <0x03>, <0x04>;		/* 2 audio input ports */
		audio-port-names = "i2s", "spdif";
		#sound-dai-cells = <1>;

		port {					/* 1 video input port */
			hdmi_0: endpoint@0 {
				remote-endpoint = <&lcd0_0>;
			};
		};
	};
};

Back to the DRM device pointer given to the tda998x driver, as the
tda998x is an encoder/connector, it has no bridge function, so, there
is no need to add a complex API for information exchange between both
drivers.

Anyway, there is a big lack in my proposal: the tda998x encoder is
hard-coded to the first CRTC. This could be solved by a scan of the DT
and of the encoder list by the DRM driver, but I think that the actual
definitions as proposed by media/video-interfaces.txt are not easy to
use.

Do you think that a description as done for ALSA could work?

A sound card creation is done by a global sound configuration and a
description of the links between the card elements. Here is the tda998x
part of the Cubox audio card ('audio1' is the audio device):

	sound {
		compatible = "simple-audio-card";
		simple-audio-card,name = "Cubox Audio";

		simple-audio-card,dai-link@0 {		/* I2S - HDMI */
			format = "i2s";
			cpu {
				sound-dai = <&audio1 0>;	/* I2S output */
			};
			codec {
				sound-dai = <&hdmi 0>;		/* I2S input */
			};
		};

		simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
			cpu {
				sound-dai = <&audio1 1>;	/* S/PDIF output */
			};
			codec {
				sound-dai = <&hdmi 1>;		/* S/PDIF input */
			};
		};
	};

Using the same elements, here is what could be the video card of the
Armada 510 with a panel, the tda998x and the display controller:

	video {
		compatible = "simple-video-card";

		simple-video-card,dvi-link {
			crtc {
				dvi = <&lcd0>;
			};
			encoder {
				dvi = <&panel>;
				connector-type = 7;	/* LVDS */
			};
		};
		simple-video-card,dvi-link {
			crtc {
				dvi = <&lcd0>;
			};
			encoder {
				dvi = <&hdmi>;
				connector-type = 11;	/* HDMI-A */
			};
		};
	};

	lcd0: lcd-controller@820000 {
		compatible = "marvell,armada-510-lcd";
		... hardware definitions ...
	};

	hdmi : hdmi-encoder {
		.. same as above, but without the video input port ..
	};

	panel: panel {
		.. panel parameters ..
	};

Then, the generic 'simple-video-card' has all elements to create the
DRM device.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Laurent Pinchart]

Hi Jean-François,

On Tuesday 25 March 2014 16:55:48 Jean-Francois Moine wrote:
> On Mon, 24 Mar 2014 23:39:01 +0100 Laurent Pinchart wrote:
> > On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> > > The 'slave encoder' structure of the tda998x driver asks for glue
> > > between the DRM driver and the encoder/connector structures.
> > > 
> > > This patch changes the driver to a normal DRM encoder/connector
> > > thanks to the infrastructure for componentised subsystems.
> > 
> > I like the idea, but I'm not really happy with the implementation. Let me
> > try to explain why below.
> > 
> > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > > ---
> > > 
> > >  drivers/gpu/drm/i2c/tda998x_drv.c | 323 ++++++++++++++++---------------
> > >  1 file changed, 188 insertions(+), 135 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > 
> > [snip]
> > 
> > > @@ -44,10 +45,14 @@ struct tda998x_priv {
> > >  	wait_queue_head_t wq_edid;
> > >  	volatile int wq_edid_wait;
> > > 
> > > -	struct drm_encoder *encoder;
> > > +	struct drm_encoder encoder;
> > > +	struct drm_connector connector;
> > > 
> > >  };
> > 
> > [snip]
> > 
> > > -static int
> > > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id
> > > *id)
> > > +static int tda_bind(struct device *dev, struct device *master, void
> > > *data)
> > >  {
> > > +	struct drm_device *drm = data;
> > 
> > This is the part that bothers me. You're making two assumptions here, that
> > the DRM driver will pass a struct drm_device pointer to the bind
> > operation, and that the I2C encoder driver can take control of DRM
> > encoder and connector creation. Although it could become problematic
> > later, the first assumption isn't too much of an issue for now. I'll thus
> > focus on the second one.
> > 
> > The component framework isolate the encoder and DRM master drivers as far
> > as component creation and binding is concerned, but doesn't provide a way
> > for the two drivers to communicate together (nor should it). You're
> > solving this by passing a pointer to the DRM device to the encoder bind
> > operation, making the encoder driver create a DRM encoder and connector,
> > and relying on the DRM core to orchestrate CRTCs, encoders and
> > connectors. You thus assume that the encoder hardware should be
> > represented by a DRM encoder object, and that its output is connected to
> > a connector that should be represented by a DRM connector object. While
> > this can work in your use case, that won't always hold true. Hardware
> > encoders can be chained together, while DRM encoders can't. The DRM core
> > has recently received support for bridge objects to overcome that
> > limitation. Depending on the hardware topology, a given hardware encoder
> > should be modeled as a DRM encoder or as a DRM bridge. That decision
> > shouldn't be taken by the encoder driver but by the DRM master driver.
> > The I2C encoder driver thus shouldn't create the DRM encoder and DRM
> > connector itself.
> > 
> > I believe the encoder/master communication problem should be solved
> > differently. Instead of passing a pointer to the DRM device to the encoder
> > driver and making the encoder driver control DRM encoder and connector
> > creation, the encoder driver should instead create an object not visible
> > to userspace that can be retrieved by the DRM master driver (possibly
> > through registration with the DRM core, or by going through drvdata in the
> > encoder's struct device). The DRM master could use that object to
> > communicate with the encoder, and would register the DRM encoder and DRM
> > connector itself based on hardware topology.
> > 
> > > +	struct i2c_client *i2c_client = to_i2c_client(dev);
> > > +	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> > > +	struct drm_connector *connector = &priv->connector;
> > > +	struct drm_encoder *encoder = &priv->encoder;
> > > +	int ret;
> > > +
> > > +	if (!try_module_get(THIS_MODULE)) {
> > > +		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = drm_connector_init(drm, connector,
> > > +				&connector_funcs,
> > > +				DRM_MODE_CONNECTOR_HDMIA);
> > 
> > This is one example of the shortcomings I've explained above. An encoder
> > driver can't always know what connector type it is connected to. If I'm
> > not mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It
> > should be up to the master driver to select the connector type based on
> > its overall view of the hardware, or even to a connector driver that would
> > be bound to a connector DT node (as proposed in
> > https://www.mail-archive.com/devicetree@vger.kernel.org/msg16585.html).
> 	[snip]
> 
> The tda998x, as a HDMI transmitter, has to deal with both video and
> audio.
> 
> Whereas the hardware connection schemes are the same in both worlds,
> the way they are translated to computer objects are very different:
> 
> - video
> 	DRM card -> CRTCs -> encoders -> (bridges) -> connectors
> 
> - audio
> 	ALSA card -> CPUs -> (CODECs) -> CODECs
> 
> and it would be nice to have a common layout.
> 
> Actually, the tda998x is a slave encoder, that is, it plays the roles of
> both encoder and connector. In the 2 DRM drivers (armada and tilcdc) which
> use it, yes, the encoders and connectors are created by the main DRM
> drivers, but, there is no notion of bridge, and, also, the encoder is
> DRM_MODE_ENCODER_TMDS and the connector is DRM_MODE_CONNECTOR_HDMIA. Then,
> nothing is changed in the global system working.
> 
> About the connector, yes, I let its type as hard-coded, but this could be
> changed by configuration in the platform data or in the DT. Anyway, there is
> nothing as such in the proposed patch
> 
> 	'Add DT binding documentation for HDMI Connector'
> 
> I also dislike this patch because it adds a device which is of no use. I had
> a same remark from Mark Brown about a tda998x CODEC proposal of mine:
> 
> 	hdmi_codec: hdmi-codec {
> 		compatible = "nxp,tda998x-codec";
> 		audio-ports = <0x03>, <0x04>;
> 	};
> 
> So, the next tda998x CODEC will be directly included in the tda998x driver,
> the audio output being the HDMI connector. Here is the DT definition I have
> for the Cubox:
> 
> &i2c0 {
> 	hdmi: hdmi-encoder {
> 		compatible = "nxp,tda9989";
> 		reg = <0x70>;
> 		interrupt-parent = <&gpio0>;
> 		interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
> 		pinctrl-0 = <&pmx_camera>;
> 		pinctrl-names = "default";
> 
> 		audio-ports = <0x03>, <0x04>;		/* 2 audio input ports */
> 		audio-port-names = "i2s", "spdif";
> 		#sound-dai-cells = <1>;
> 
> 		port {					/* 1 video input port */
> 			hdmi_0: endpoint@0 {
> 				remote-endpoint = <&lcd0_0>;
> 			};
> 		};
> 	};
> };
> 
> Back to the DRM device pointer given to the tda998x driver, as the tda998x
> is an encoder/connector, it has no bridge function, so, there is no need to
> add a complex API for information exchange between both drivers.
> 
> Anyway, there is a big lack in my proposal: the tda998x encoder is hard-
>coded to the first CRTC. This could be solved by a scan of the DT and of the
> encoder list by the DRM driver, but I think that the actual definitions as
> proposed by media/video-interfaces.txt are not easy to use.
> 
> Do you think that a description as done for ALSA could work?
> 
> A sound card creation is done by a global sound configuration and a
> description of the links between the card elements. Here is the tda998x
> part of the Cubox audio card ('audio1' is the audio device):
> 
> 	sound {
> 		compatible = "simple-audio-card";
> 		simple-audio-card,name = "Cubox Audio";
> 
> 		simple-audio-card,dai-link@0 {		/* I2S - HDMI */
> 			format = "i2s";
> 			cpu {
> 				sound-dai = <&audio1 0>;	/* I2S output */
> 			};
> 			codec {
> 				sound-dai = <&hdmi 0>;		/* I2S input */
> 			};
> 		};
> 
> 		simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
> 			cpu {
> 				sound-dai = <&audio1 1>;	/* S/PDIF output */
> 			};
> 			codec {
> 				sound-dai = <&hdmi 1>;		/* S/PDIF input */
> 			};
> 		};
> 	};
> 
> Using the same elements, here is what could be the video card of the
> Armada 510 with a panel, the tda998x and the display controller:
> 
> 	video {
> 		compatible = "simple-video-card";
> 
> 		simple-video-card,dvi-link {
> 			crtc {
> 				dvi = <&lcd0>;
> 			};
> 			encoder {
> 				dvi = <&panel>;
> 				connector-type = 7;	/* LVDS */
> 			};
> 		};
> 		simple-video-card,dvi-link {
> 			crtc {
> 				dvi = <&lcd0>;
> 			};
> 			encoder {
> 				dvi = <&hdmi>;
> 				connector-type = 11;	/* HDMI-A */
> 			};
> 		};
> 	};
> 
> 	lcd0: lcd-controller@820000 {
> 		compatible = "marvell,armada-510-lcd";
> 		... hardware definitions ...
> 	};
> 
> 	hdmi : hdmi-encoder {
> 		.. same as above, but without the video input port ..
> 	};
> 
> 	panel: panel {
> 		.. panel parameters ..
> 	};
> 
> Then, the generic 'simple-video-card' has all elements to create the DRM
> device.

That could work in your case, but I don't really like that.

We need to describe the hardware topology, that might be the only point we all 
agree on. There are various hardware topologies we need to support with 
different levels of complexity, and several ways to describe them, also with a 
wide range of complexities and features.

The more complex the hardware topology, the more complex its description needs 
to be, and the more complex the code that will parse the description and 
handle the hardware will be. I don't think there's any doubt here.

A given device can be integrated in a wide variety of hardware with different 
complexities. A driver can't thus always assume that the whole composite 
display device will match a given class of topologies. This is especially true 
for encoders and connectors, they can be hooked up directly at the output of a 
very simple display controller, or can be part of a very complex graph. 
Encoder and connector drivers can't assume much about how they will be 
integrated. I want to avoid a situation where using an HDMI encoder already 
supported in mainline with a different SoC will result in having to rewrite 
the HDMI encoder driver.

The story is a bit different for display controllers. While the hardware 
topology outside (and sometimes inside as well) of the SoC can vary, a display 
controller often approximately implies a given level of complexity. A cheap 
SoC with a simple display controller will likely not be used to drive a 4k 
display requiring 4 encoders running in parallel for instance. While I also 
want to avoid having to make significant changes to a display controller 
driver when using the SoC on a different board, I believe the requirement can 
be slightly relaxed here, and the display controller driver(s) can assume more 
about the hardware topology than the encoder drivers.

I've asked myself whether we needed a single, one-size-fits-them-all DT 
representation of the hardware topology. The view of the world from an 
external encoder point of view must not depend on the SoC it is hooked up to 
(this would prevent using a single encoder driver with multiple SoCs), which 
calls for at least some form of standardization. The topology representation 
on the display controller side may vary from display controller to display 
controller, but I believe this would just result in code duplication and 
having to solve the same problem in multiple drivers. For those reasons I 
believe that the OF graph proposal to represent the display hardware topology 
would be a good choice. The bindings are flexible enough to represent both 
simple and complex hardware.

Now, I don't want to force all display device drivers to implement complex 
code when they only need to support simple hardware and simple hardware 
topologies. Not only would that be rightfully rejected, I would be among the 
ones nack'ing that approach. My opinion is that this calls for the creation of 
helpers to handle common cases. Several (possibly many) display systems only 
need (or want) to support linear pipelines at their output(s), made of a 
single encoder and a single connector. There's no point in duplicating DT 
parsing or encoder/connector instantiation code in several drivers in that 
case where helpers could be reused. Several sets of helpers could support 
different kinds of topologies, with the driver author selecting a set of 
helpers depending on the expected hardware topology complexity.

We also need to decide on who (as in which driver) will be responsible for 
binding all devices together. As DRM exposes a single device to userspace, 
there needs to be a single driver that will perform front line handling of the 
userspace API and delegate calls to the other drivers involved. I believe it 
would be logical for that driver to also be in charge of bindings the 
components together, as it will be the driver that delegate calls to the 
components. For a similar reason I also believe that that driver should also 
be the one in control of creating DRM encoders and DRM connectors. The 
component drivers having only a narrow view of the device they handle, they 
can't perform that task in a generic way and would thus get tied to specific 
master drivers because of the assumptions they would make.

Whether the master driver is the CRTC driver or a separate driver that binds 
standalone CRTCs, connectors and encoders doesn't in my opinion change my 
above conclusions. Some SoCs could use a simple-video-card driver like the one 
you've proposed, and others could implement a display controller driver that 
would perform the same tasks for more complex pipelines in addition to 
controlling the display controller's CRTC(s). The simple-video-card driver 
would perform that same tasks as the helpers I've previously talked about, so 
the two solutions are pretty similar, and I don't see much added value in the 
general case in having a simple-video-card driver compared to using helpers in 
the display controller driver.

The point that matters to me is that encoders DT bindings and drivers must be 
identical regardless of whether the master driver is the display controller 
driver or a driver for a logical composite device. For all those reasons we 
should use the OF graph DT bindings for the simple-video-card driver should we 
decide to implement it, and we should create DRM encoders and connectors in 
the master driver, not in the encoder drivers.

-- 
Regards,

Laurent Pinchart

[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Laurent Pinchart]

Hi Jean-Fran?ois,

On Tuesday 25 March 2014 16:55:48 Jean-Francois Moine wrote:
> On Mon, 24 Mar 2014 23:39:01 +0100 Laurent Pinchart wrote:
> > On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> > > The 'slave encoder' structure of the tda998x driver asks for glue
> > > between the DRM driver and the encoder/connector structures.
> > > 
> > > This patch changes the driver to a normal DRM encoder/connector
> > > thanks to the infrastructure for componentised subsystems.
> > 
> > I like the idea, but I'm not really happy with the implementation. Let me
> > try to explain why below.
> > 
> > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > > ---
> > > 
> > >  drivers/gpu/drm/i2c/tda998x_drv.c | 323 ++++++++++++++++---------------
> > >  1 file changed, 188 insertions(+), 135 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > 
> > [snip]
> > 
> > > @@ -44,10 +45,14 @@ struct tda998x_priv {
> > >  	wait_queue_head_t wq_edid;
> > >  	volatile int wq_edid_wait;
> > > 
> > > -	struct drm_encoder *encoder;
> > > +	struct drm_encoder encoder;
> > > +	struct drm_connector connector;
> > > 
> > >  };
> > 
> > [snip]
> > 
> > > -static int
> > > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id
> > > *id)
> > > +static int tda_bind(struct device *dev, struct device *master, void
> > > *data)
> > >  {
> > > +	struct drm_device *drm = data;
> > 
> > This is the part that bothers me. You're making two assumptions here, that
> > the DRM driver will pass a struct drm_device pointer to the bind
> > operation, and that the I2C encoder driver can take control of DRM
> > encoder and connector creation. Although it could become problematic
> > later, the first assumption isn't too much of an issue for now. I'll thus
> > focus on the second one.
> > 
> > The component framework isolate the encoder and DRM master drivers as far
> > as component creation and binding is concerned, but doesn't provide a way
> > for the two drivers to communicate together (nor should it). You're
> > solving this by passing a pointer to the DRM device to the encoder bind
> > operation, making the encoder driver create a DRM encoder and connector,
> > and relying on the DRM core to orchestrate CRTCs, encoders and
> > connectors. You thus assume that the encoder hardware should be
> > represented by a DRM encoder object, and that its output is connected to
> > a connector that should be represented by a DRM connector object. While
> > this can work in your use case, that won't always hold true. Hardware
> > encoders can be chained together, while DRM encoders can't. The DRM core
> > has recently received support for bridge objects to overcome that
> > limitation. Depending on the hardware topology, a given hardware encoder
> > should be modeled as a DRM encoder or as a DRM bridge. That decision
> > shouldn't be taken by the encoder driver but by the DRM master driver.
> > The I2C encoder driver thus shouldn't create the DRM encoder and DRM
> > connector itself.
> > 
> > I believe the encoder/master communication problem should be solved
> > differently. Instead of passing a pointer to the DRM device to the encoder
> > driver and making the encoder driver control DRM encoder and connector
> > creation, the encoder driver should instead create an object not visible
> > to userspace that can be retrieved by the DRM master driver (possibly
> > through registration with the DRM core, or by going through drvdata in the
> > encoder's struct device). The DRM master could use that object to
> > communicate with the encoder, and would register the DRM encoder and DRM
> > connector itself based on hardware topology.
> > 
> > > +	struct i2c_client *i2c_client = to_i2c_client(dev);
> > > +	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> > > +	struct drm_connector *connector = &priv->connector;
> > > +	struct drm_encoder *encoder = &priv->encoder;
> > > +	int ret;
> > > +
> > > +	if (!try_module_get(THIS_MODULE)) {
> > > +		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = drm_connector_init(drm, connector,
> > > +				&connector_funcs,
> > > +				DRM_MODE_CONNECTOR_HDMIA);
> > 
> > This is one example of the shortcomings I've explained above. An encoder
> > driver can't always know what connector type it is connected to. If I'm
> > not mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It
> > should be up to the master driver to select the connector type based on
> > its overall view of the hardware, or even to a connector driver that would
> > be bound to a connector DT node (as proposed in
> > https://www.mail-archive.com/devicetree at vger.kernel.org/msg16585.html).
> 	[snip]
> 
> The tda998x, as a HDMI transmitter, has to deal with both video and
> audio.
> 
> Whereas the hardware connection schemes are the same in both worlds,
> the way they are translated to computer objects are very different:
> 
> - video
> 	DRM card -> CRTCs -> encoders -> (bridges) -> connectors
> 
> - audio
> 	ALSA card -> CPUs -> (CODECs) -> CODECs
> 
> and it would be nice to have a common layout.
> 
> Actually, the tda998x is a slave encoder, that is, it plays the roles of
> both encoder and connector. In the 2 DRM drivers (armada and tilcdc) which
> use it, yes, the encoders and connectors are created by the main DRM
> drivers, but, there is no notion of bridge, and, also, the encoder is
> DRM_MODE_ENCODER_TMDS and the connector is DRM_MODE_CONNECTOR_HDMIA. Then,
> nothing is changed in the global system working.
> 
> About the connector, yes, I let its type as hard-coded, but this could be
> changed by configuration in the platform data or in the DT. Anyway, there is
> nothing as such in the proposed patch
> 
> 	'Add DT binding documentation for HDMI Connector'
> 
> I also dislike this patch because it adds a device which is of no use. I had
> a same remark from Mark Brown about a tda998x CODEC proposal of mine:
> 
> 	hdmi_codec: hdmi-codec {
> 		compatible = "nxp,tda998x-codec";
> 		audio-ports = <0x03>, <0x04>;
> 	};
> 
> So, the next tda998x CODEC will be directly included in the tda998x driver,
> the audio output being the HDMI connector. Here is the DT definition I have
> for the Cubox:
> 
> &i2c0 {
> 	hdmi: hdmi-encoder {
> 		compatible = "nxp,tda9989";
> 		reg = <0x70>;
> 		interrupt-parent = <&gpio0>;
> 		interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
> 		pinctrl-0 = <&pmx_camera>;
> 		pinctrl-names = "default";
> 
> 		audio-ports = <0x03>, <0x04>;		/* 2 audio input ports */
> 		audio-port-names = "i2s", "spdif";
> 		#sound-dai-cells = <1>;
> 
> 		port {					/* 1 video input port */
> 			hdmi_0: endpoint at 0 {
> 				remote-endpoint = <&lcd0_0>;
> 			};
> 		};
> 	};
> };
> 
> Back to the DRM device pointer given to the tda998x driver, as the tda998x
> is an encoder/connector, it has no bridge function, so, there is no need to
> add a complex API for information exchange between both drivers.
> 
> Anyway, there is a big lack in my proposal: the tda998x encoder is hard-
>coded to the first CRTC. This could be solved by a scan of the DT and of the
> encoder list by the DRM driver, but I think that the actual definitions as
> proposed by media/video-interfaces.txt are not easy to use.
> 
> Do you think that a description as done for ALSA could work?
> 
> A sound card creation is done by a global sound configuration and a
> description of the links between the card elements. Here is the tda998x
> part of the Cubox audio card ('audio1' is the audio device):
> 
> 	sound {
> 		compatible = "simple-audio-card";
> 		simple-audio-card,name = "Cubox Audio";
> 
> 		simple-audio-card,dai-link at 0 {		/* I2S - HDMI */
> 			format = "i2s";
> 			cpu {
> 				sound-dai = <&audio1 0>;	/* I2S output */
> 			};
> 			codec {
> 				sound-dai = <&hdmi 0>;		/* I2S input */
> 			};
> 		};
> 
> 		simple-audio-card,dai-link at 1 {		/* S/PDIF - HDMI */
> 			cpu {
> 				sound-dai = <&audio1 1>;	/* S/PDIF output */
> 			};
> 			codec {
> 				sound-dai = <&hdmi 1>;		/* S/PDIF input */
> 			};
> 		};
> 	};
> 
> Using the same elements, here is what could be the video card of the
> Armada 510 with a panel, the tda998x and the display controller:
> 
> 	video {
> 		compatible = "simple-video-card";
> 
> 		simple-video-card,dvi-link {
> 			crtc {
> 				dvi = <&lcd0>;
> 			};
> 			encoder {
> 				dvi = <&panel>;
> 				connector-type = 7;	/* LVDS */
> 			};
> 		};
> 		simple-video-card,dvi-link {
> 			crtc {
> 				dvi = <&lcd0>;
> 			};
> 			encoder {
> 				dvi = <&hdmi>;
> 				connector-type = 11;	/* HDMI-A */
> 			};
> 		};
> 	};
> 
> 	lcd0: lcd-controller at 820000 {
> 		compatible = "marvell,armada-510-lcd";
> 		... hardware definitions ...
> 	};
> 
> 	hdmi : hdmi-encoder {
> 		.. same as above, but without the video input port ..
> 	};
> 
> 	panel: panel {
> 		.. panel parameters ..
> 	};
> 
> Then, the generic 'simple-video-card' has all elements to create the DRM
> device.

That could work in your case, but I don't really like that.

We need to describe the hardware topology, that might be the only point we all 
agree on. There are various hardware topologies we need to support with 
different levels of complexity, and several ways to describe them, also with a 
wide range of complexities and features.

The more complex the hardware topology, the more complex its description needs 
to be, and the more complex the code that will parse the description and 
handle the hardware will be. I don't think there's any doubt here.

A given device can be integrated in a wide variety of hardware with different 
complexities. A driver can't thus always assume that the whole composite 
display device will match a given class of topologies. This is especially true 
for encoders and connectors, they can be hooked up directly at the output of a 
very simple display controller, or can be part of a very complex graph. 
Encoder and connector drivers can't assume much about how they will be 
integrated. I want to avoid a situation where using an HDMI encoder already 
supported in mainline with a different SoC will result in having to rewrite 
the HDMI encoder driver.

The story is a bit different for display controllers. While the hardware 
topology outside (and sometimes inside as well) of the SoC can vary, a display 
controller often approximately implies a given level of complexity. A cheap 
SoC with a simple display controller will likely not be used to drive a 4k 
display requiring 4 encoders running in parallel for instance. While I also 
want to avoid having to make significant changes to a display controller 
driver when using the SoC on a different board, I believe the requirement can 
be slightly relaxed here, and the display controller driver(s) can assume more 
about the hardware topology than the encoder drivers.

I've asked myself whether we needed a single, one-size-fits-them-all DT 
representation of the hardware topology. The view of the world from an 
external encoder point of view must not depend on the SoC it is hooked up to 
(this would prevent using a single encoder driver with multiple SoCs), which 
calls for at least some form of standardization. The topology representation 
on the display controller side may vary from display controller to display 
controller, but I believe this would just result in code duplication and 
having to solve the same problem in multiple drivers. For those reasons I 
believe that the OF graph proposal to represent the display hardware topology 
would be a good choice. The bindings are flexible enough to represent both 
simple and complex hardware.

Now, I don't want to force all display device drivers to implement complex 
code when they only need to support simple hardware and simple hardware 
topologies. Not only would that be rightfully rejected, I would be among the 
ones nack'ing that approach. My opinion is that this calls for the creation of 
helpers to handle common cases. Several (possibly many) display systems only 
need (or want) to support linear pipelines at their output(s), made of a 
single encoder and a single connector. There's no point in duplicating DT 
parsing or encoder/connector instantiation code in several drivers in that 
case where helpers could be reused. Several sets of helpers could support 
different kinds of topologies, with the driver author selecting a set of 
helpers depending on the expected hardware topology complexity.

We also need to decide on who (as in which driver) will be responsible for 
binding all devices together. As DRM exposes a single device to userspace, 
there needs to be a single driver that will perform front line handling of the 
userspace API and delegate calls to the other drivers involved. I believe it 
would be logical for that driver to also be in charge of bindings the 
components together, as it will be the driver that delegate calls to the 
components. For a similar reason I also believe that that driver should also 
be the one in control of creating DRM encoders and DRM connectors. The 
component drivers having only a narrow view of the device they handle, they 
can't perform that task in a generic way and would thus get tied to specific 
master drivers because of the assumptions they would make.

Whether the master driver is the CRTC driver or a separate driver that binds 
standalone CRTCs, connectors and encoders doesn't in my opinion change my 
above conclusions. Some SoCs could use a simple-video-card driver like the one 
you've proposed, and others could implement a display controller driver that 
would perform the same tasks for more complex pipelines in addition to 
controlling the display controller's CRTC(s). The simple-video-card driver 
would perform that same tasks as the helpers I've previously talked about, so 
the two solutions are pretty similar, and I don't see much added value in the 
general case in having a simple-video-card driver compared to using helpers in 
the display controller driver.

The point that matters to me is that encoders DT bindings and drivers must be 
identical regardless of whether the master driver is the display controller 
driver or a driver for a logical composite device. For all those reasons we 
should use the OF graph DT bindings for the simple-video-card driver should we 
decide to implement it, and we should create DRM encoders and connectors in 
the master driver, not in the encoder drivers.

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Laurent Pinchart]

Hi Jean-François,

On Tuesday 25 March 2014 16:55:48 Jean-Francois Moine wrote:
> On Mon, 24 Mar 2014 23:39:01 +0100 Laurent Pinchart wrote:
> > On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote:
> > > The 'slave encoder' structure of the tda998x driver asks for glue
> > > between the DRM driver and the encoder/connector structures.
> > > 
> > > This patch changes the driver to a normal DRM encoder/connector
> > > thanks to the infrastructure for componentised subsystems.
> > 
> > I like the idea, but I'm not really happy with the implementation. Let me
> > try to explain why below.
> > 
> > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > > ---
> > > 
> > >  drivers/gpu/drm/i2c/tda998x_drv.c | 323 ++++++++++++++++---------------
> > >  1 file changed, 188 insertions(+), 135 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644
> > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > 
> > [snip]
> > 
> > > @@ -44,10 +45,14 @@ struct tda998x_priv {
> > >  	wait_queue_head_t wq_edid;
> > >  	volatile int wq_edid_wait;
> > > 
> > > -	struct drm_encoder *encoder;
> > > +	struct drm_encoder encoder;
> > > +	struct drm_connector connector;
> > > 
> > >  };
> > 
> > [snip]
> > 
> > > -static int
> > > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id
> > > *id)
> > > +static int tda_bind(struct device *dev, struct device *master, void
> > > *data)
> > >  {
> > > +	struct drm_device *drm = data;
> > 
> > This is the part that bothers me. You're making two assumptions here, that
> > the DRM driver will pass a struct drm_device pointer to the bind
> > operation, and that the I2C encoder driver can take control of DRM
> > encoder and connector creation. Although it could become problematic
> > later, the first assumption isn't too much of an issue for now. I'll thus
> > focus on the second one.
> > 
> > The component framework isolate the encoder and DRM master drivers as far
> > as component creation and binding is concerned, but doesn't provide a way
> > for the two drivers to communicate together (nor should it). You're
> > solving this by passing a pointer to the DRM device to the encoder bind
> > operation, making the encoder driver create a DRM encoder and connector,
> > and relying on the DRM core to orchestrate CRTCs, encoders and
> > connectors. You thus assume that the encoder hardware should be
> > represented by a DRM encoder object, and that its output is connected to
> > a connector that should be represented by a DRM connector object. While
> > this can work in your use case, that won't always hold true. Hardware
> > encoders can be chained together, while DRM encoders can't. The DRM core
> > has recently received support for bridge objects to overcome that
> > limitation. Depending on the hardware topology, a given hardware encoder
> > should be modeled as a DRM encoder or as a DRM bridge. That decision
> > shouldn't be taken by the encoder driver but by the DRM master driver.
> > The I2C encoder driver thus shouldn't create the DRM encoder and DRM
> > connector itself.
> > 
> > I believe the encoder/master communication problem should be solved
> > differently. Instead of passing a pointer to the DRM device to the encoder
> > driver and making the encoder driver control DRM encoder and connector
> > creation, the encoder driver should instead create an object not visible
> > to userspace that can be retrieved by the DRM master driver (possibly
> > through registration with the DRM core, or by going through drvdata in the
> > encoder's struct device). The DRM master could use that object to
> > communicate with the encoder, and would register the DRM encoder and DRM
> > connector itself based on hardware topology.
> > 
> > > +	struct i2c_client *i2c_client = to_i2c_client(dev);
> > > +	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
> > > +	struct drm_connector *connector = &priv->connector;
> > > +	struct drm_encoder *encoder = &priv->encoder;
> > > +	int ret;
> > > +
> > > +	if (!try_module_get(THIS_MODULE)) {
> > > +		dev_err(dev, "cannot get module %s\n", THIS_MODULE->name);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = drm_connector_init(drm, connector,
> > > +				&connector_funcs,
> > > +				DRM_MODE_CONNECTOR_HDMIA);
> > 
> > This is one example of the shortcomings I've explained above. An encoder
> > driver can't always know what connector type it is connected to. If I'm
> > not mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It
> > should be up to the master driver to select the connector type based on
> > its overall view of the hardware, or even to a connector driver that would
> > be bound to a connector DT node (as proposed in
> > https://www.mail-archive.com/devicetree@vger.kernel.org/msg16585.html).
> 	[snip]
> 
> The tda998x, as a HDMI transmitter, has to deal with both video and
> audio.
> 
> Whereas the hardware connection schemes are the same in both worlds,
> the way they are translated to computer objects are very different:
> 
> - video
> 	DRM card -> CRTCs -> encoders -> (bridges) -> connectors
> 
> - audio
> 	ALSA card -> CPUs -> (CODECs) -> CODECs
> 
> and it would be nice to have a common layout.
> 
> Actually, the tda998x is a slave encoder, that is, it plays the roles of
> both encoder and connector. In the 2 DRM drivers (armada and tilcdc) which
> use it, yes, the encoders and connectors are created by the main DRM
> drivers, but, there is no notion of bridge, and, also, the encoder is
> DRM_MODE_ENCODER_TMDS and the connector is DRM_MODE_CONNECTOR_HDMIA. Then,
> nothing is changed in the global system working.
> 
> About the connector, yes, I let its type as hard-coded, but this could be
> changed by configuration in the platform data or in the DT. Anyway, there is
> nothing as such in the proposed patch
> 
> 	'Add DT binding documentation for HDMI Connector'
> 
> I also dislike this patch because it adds a device which is of no use. I had
> a same remark from Mark Brown about a tda998x CODEC proposal of mine:
> 
> 	hdmi_codec: hdmi-codec {
> 		compatible = "nxp,tda998x-codec";
> 		audio-ports = <0x03>, <0x04>;
> 	};
> 
> So, the next tda998x CODEC will be directly included in the tda998x driver,
> the audio output being the HDMI connector. Here is the DT definition I have
> for the Cubox:
> 
> &i2c0 {
> 	hdmi: hdmi-encoder {
> 		compatible = "nxp,tda9989";
> 		reg = <0x70>;
> 		interrupt-parent = <&gpio0>;
> 		interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
> 		pinctrl-0 = <&pmx_camera>;
> 		pinctrl-names = "default";
> 
> 		audio-ports = <0x03>, <0x04>;		/* 2 audio input ports */
> 		audio-port-names = "i2s", "spdif";
> 		#sound-dai-cells = <1>;
> 
> 		port {					/* 1 video input port */
> 			hdmi_0: endpoint@0 {
> 				remote-endpoint = <&lcd0_0>;
> 			};
> 		};
> 	};
> };
> 
> Back to the DRM device pointer given to the tda998x driver, as the tda998x
> is an encoder/connector, it has no bridge function, so, there is no need to
> add a complex API for information exchange between both drivers.
> 
> Anyway, there is a big lack in my proposal: the tda998x encoder is hard-
>coded to the first CRTC. This could be solved by a scan of the DT and of the
> encoder list by the DRM driver, but I think that the actual definitions as
> proposed by media/video-interfaces.txt are not easy to use.
> 
> Do you think that a description as done for ALSA could work?
> 
> A sound card creation is done by a global sound configuration and a
> description of the links between the card elements. Here is the tda998x
> part of the Cubox audio card ('audio1' is the audio device):
> 
> 	sound {
> 		compatible = "simple-audio-card";
> 		simple-audio-card,name = "Cubox Audio";
> 
> 		simple-audio-card,dai-link@0 {		/* I2S - HDMI */
> 			format = "i2s";
> 			cpu {
> 				sound-dai = <&audio1 0>;	/* I2S output */
> 			};
> 			codec {
> 				sound-dai = <&hdmi 0>;		/* I2S input */
> 			};
> 		};
> 
> 		simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
> 			cpu {
> 				sound-dai = <&audio1 1>;	/* S/PDIF output */
> 			};
> 			codec {
> 				sound-dai = <&hdmi 1>;		/* S/PDIF input */
> 			};
> 		};
> 	};
> 
> Using the same elements, here is what could be the video card of the
> Armada 510 with a panel, the tda998x and the display controller:
> 
> 	video {
> 		compatible = "simple-video-card";
> 
> 		simple-video-card,dvi-link {
> 			crtc {
> 				dvi = <&lcd0>;
> 			};
> 			encoder {
> 				dvi = <&panel>;
> 				connector-type = 7;	/* LVDS */
> 			};
> 		};
> 		simple-video-card,dvi-link {
> 			crtc {
> 				dvi = <&lcd0>;
> 			};
> 			encoder {
> 				dvi = <&hdmi>;
> 				connector-type = 11;	/* HDMI-A */
> 			};
> 		};
> 	};
> 
> 	lcd0: lcd-controller@820000 {
> 		compatible = "marvell,armada-510-lcd";
> 		... hardware definitions ...
> 	};
> 
> 	hdmi : hdmi-encoder {
> 		.. same as above, but without the video input port ..
> 	};
> 
> 	panel: panel {
> 		.. panel parameters ..
> 	};
> 
> Then, the generic 'simple-video-card' has all elements to create the DRM
> device.

That could work in your case, but I don't really like that.

We need to describe the hardware topology, that might be the only point we all 
agree on. There are various hardware topologies we need to support with 
different levels of complexity, and several ways to describe them, also with a 
wide range of complexities and features.

The more complex the hardware topology, the more complex its description needs 
to be, and the more complex the code that will parse the description and 
handle the hardware will be. I don't think there's any doubt here.

A given device can be integrated in a wide variety of hardware with different 
complexities. A driver can't thus always assume that the whole composite 
display device will match a given class of topologies. This is especially true 
for encoders and connectors, they can be hooked up directly at the output of a 
very simple display controller, or can be part of a very complex graph. 
Encoder and connector drivers can't assume much about how they will be 
integrated. I want to avoid a situation where using an HDMI encoder already 
supported in mainline with a different SoC will result in having to rewrite 
the HDMI encoder driver.

The story is a bit different for display controllers. While the hardware 
topology outside (and sometimes inside as well) of the SoC can vary, a display 
controller often approximately implies a given level of complexity. A cheap 
SoC with a simple display controller will likely not be used to drive a 4k 
display requiring 4 encoders running in parallel for instance. While I also 
want to avoid having to make significant changes to a display controller 
driver when using the SoC on a different board, I believe the requirement can 
be slightly relaxed here, and the display controller driver(s) can assume more 
about the hardware topology than the encoder drivers.

I've asked myself whether we needed a single, one-size-fits-them-all DT 
representation of the hardware topology. The view of the world from an 
external encoder point of view must not depend on the SoC it is hooked up to 
(this would prevent using a single encoder driver with multiple SoCs), which 
calls for at least some form of standardization. The topology representation 
on the display controller side may vary from display controller to display 
controller, but I believe this would just result in code duplication and 
having to solve the same problem in multiple drivers. For those reasons I 
believe that the OF graph proposal to represent the display hardware topology 
would be a good choice. The bindings are flexible enough to represent both 
simple and complex hardware.

Now, I don't want to force all display device drivers to implement complex 
code when they only need to support simple hardware and simple hardware 
topologies. Not only would that be rightfully rejected, I would be among the 
ones nack'ing that approach. My opinion is that this calls for the creation of 
helpers to handle common cases. Several (possibly many) display systems only 
need (or want) to support linear pipelines at their output(s), made of a 
single encoder and a single connector. There's no point in duplicating DT 
parsing or encoder/connector instantiation code in several drivers in that 
case where helpers could be reused. Several sets of helpers could support 
different kinds of topologies, with the driver author selecting a set of 
helpers depending on the expected hardware topology complexity.

We also need to decide on who (as in which driver) will be responsible for 
binding all devices together. As DRM exposes a single device to userspace, 
there needs to be a single driver that will perform front line handling of the 
userspace API and delegate calls to the other drivers involved. I believe it 
would be logical for that driver to also be in charge of bindings the 
components together, as it will be the driver that delegate calls to the 
components. For a similar reason I also believe that that driver should also 
be the one in control of creating DRM encoders and DRM connectors. The 
component drivers having only a narrow view of the device they handle, they 
can't perform that task in a generic way and would thus get tied to specific 
master drivers because of the assumptions they would make.

Whether the master driver is the CRTC driver or a separate driver that binds 
standalone CRTCs, connectors and encoders doesn't in my opinion change my 
above conclusions. Some SoCs could use a simple-video-card driver like the one 
you've proposed, and others could implement a display controller driver that 
would perform the same tasks for more complex pipelines in addition to 
controlling the display controller's CRTC(s). The simple-video-card driver 
would perform that same tasks as the helpers I've previously talked about, so 
the two solutions are pretty similar, and I don't see much added value in the 
general case in having a simple-video-card driver compared to using helpers in 
the display controller driver.

The point that matters to me is that encoders DT bindings and drivers must be 
identical regardless of whether the master driver is the display controller 
driver or a driver for a logical composite device. For all those reasons we 
should use the OF graph DT bindings for the simple-video-card driver should we 
decide to implement it, and we should create DRM encoders and connectors in 
the master driver, not in the encoder drivers.

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

Hi Laurent,

On Wed, 26 Mar 2014 18:33:09 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> That could work in your case, but I don't really like that.
> 
> We need to describe the hardware topology, that might be the only point we all 
> agree on. There are various hardware topologies we need to support with 
> different levels of complexity, and several ways to describe them, also with a 
> wide range of complexities and features.
> 
> The more complex the hardware topology, the more complex its description needs 
> to be, and the more complex the code that will parse the description and 
> handle the hardware will be. I don't think there's any doubt here.

But also, the simpler is the topology and the simpler should be its
description.

In your approach, the connections between endpoints are described in
the components themselves, which makes hard for the DT maintainers to
have a global understanding of the video subsystem.

Having the topology described in the master device is clearer, even if
it is complex.

> A given device can be integrated in a wide variety of hardware with different 
> complexities. A driver can't thus always assume that the whole composite 
> display device will match a given class of topologies. This is especially true 
> for encoders and connectors, they can be hooked up directly at the output of a 
> very simple display controller, or can be part of a very complex graph. 
> Encoder and connector drivers can't assume much about how they will be 
> integrated. I want to avoid a situation where using an HDMI encoder already 
> supported in mainline with a different SoC will result in having to rewrite 
> the HDMI encoder driver.

The tda998x chips are simple enough for being used in many boards: one
video input and one serial digital output. I don't see in which
circumstance the driver should be rewritten.

> The story is a bit different for display controllers. While the hardware 
> topology outside (and sometimes inside as well) of the SoC can vary, a display 
> controller often approximately implies a given level of complexity. A cheap 
> SoC with a simple display controller will likely not be used to drive a 4k 
> display requiring 4 encoders running in parallel for instance. While I also 
> want to avoid having to make significant changes to a display controller 
> driver when using the SoC on a different board, I believe the requirement can 
> be slightly relaxed here, and the display controller driver(s) can assume more 
> about the hardware topology than the encoder drivers.

I don't think that the display controllers or the encoders have to know
about the topology. They have well-knwon specific functions and the way
they are interconnected should not impact these functions. If that
would be the case, they should offer a particular configuration
interface to the master driver.

> I've asked myself whether we needed a single, one-size-fits-them-all DT 
> representation of the hardware topology. The view of the world from an 
> external encoder point of view must not depend on the SoC it is hooked up to 
> (this would prevent using a single encoder driver with multiple SoCs), which 
> calls for at least some form of standardization. The topology representation 
> on the display controller side may vary from display controller to display 
> controller, but I believe this would just result in code duplication and 
> having to solve the same problem in multiple drivers. For those reasons I 
> believe that the OF graph proposal to represent the display hardware topology 
> would be a good choice. The bindings are flexible enough to represent both 
> simple and complex hardware.

Your OF graph proposal is rather complex, and it does not even indicate
which way the data flows.

> Now, I don't want to force all display device drivers to implement complex 
> code when they only need to support simple hardware and simple hardware 
> topologies. Not only would that be rightfully rejected, I would be among the 
> ones nack'ing that approach. My opinion is that this calls for the creation of 
> helpers to handle common cases. Several (possibly many) display systems only 
> need (or want) to support linear pipelines at their output(s), made of a 
> single encoder and a single connector. There's no point in duplicating DT 
> parsing or encoder/connector instantiation code in several drivers in that 
> case where helpers could be reused. Several sets of helpers could support 
> different kinds of topologies, with the driver author selecting a set of 
> helpers depending on the expected hardware topology complexity.

I don't like this 'helper' notion. See below.

> We also need to decide on who (as in which driver) will be responsible for 
> binding all devices together. As DRM exposes a single device to userspace, 
> there needs to be a single driver that will perform front line handling of the 
> userspace API and delegate calls to the other drivers involved. I believe it 
> would be logical for that driver to also be in charge of bindings the 
> components together, as it will be the driver that delegate calls to the 
> components. For a similar reason I also believe that that driver should also 
> be the one in control of creating DRM encoders and DRM connectors. The 
> component drivers having only a narrow view of the device they handle, they 
> can't perform that task in a generic way and would thus get tied to specific 
> master drivers because of the assumptions they would make.

I don't see why the encoders and connectors should be created by some
external entity. They are declared in the DT or created by the board
init, so, they exist at system startup time.

> Whether the master driver is the CRTC driver or a separate driver that binds 
> standalone CRTCs, connectors and encoders doesn't in my opinion change my 
> above conclusions. Some SoCs could use a simple-video-card driver like the one 
> you've proposed, and others could implement a display controller driver that 
> would perform the same tasks for more complex pipelines in addition to 
> controlling the display controller's CRTC(s). The simple-video-card driver 
> would perform that same tasks as the helpers I've previously talked about, so 
> the two solutions are pretty similar, and I don't see much added value in the 
> general case in having a simple-video-card driver compared to using helpers in 
> the display controller driver.

In fact, I wonder why there is not only one DRM driver. The global
logic is always the same, and, actually, it is duplicated in each
specific driver. I had this approach in the gspca driver: one main
driver and many specific subdrivers. Once, I even had the idea to
convert your UVC driver into a gspca subdriver, but, at this time, I
had too much work with the other webcams. Nevertheless, it would have
been easy: all the required interfaces were (are still) present in the
main gspca driver.

In the same order of idea, there is only one ALSA SoC driver, and you
cannot say that the audio topology is less complex than the video one.

When thinking about the unique DRM driver, there would no helper anymore:
the driver would offer to each component the functions they need for
working correctly.

> The point that matters to me is that encoders DT bindings and drivers must be 
> identical regardless of whether the master driver is the display controller 
> driver or a driver for a logical composite device. For all those reasons we 
> should use the OF graph DT bindings for the simple-video-card driver should we 
> decide to implement it, and we should create DRM encoders and connectors in 
> the master driver, not in the encoder drivers.

Sorry, but I think that a DRM encoder is the hardware encoder. What
else could it be? At initialization time, the master driver has only to
manage the relation between the video components. It has not to create
entities which already exist. So my preferred scheme is:

- at starting time, each video component initializes its software and
  hardware, and then, plugs itself into the DRM driver.

- from the central video topology, the DRM device waits for all
  components to be plugged and then it links the components together,
  creating the user's view of the system.

If you create the encoders and connectors in the DRM master, the
hardware encoder and connector devices are just like zombies. They must
put something in the system to say they are here and they must also
have a callback function for them to know to which video subsystem they
belong.

On the contrary, if the DRM encoders and connectors are created by the
hardware devices, they are immediately alive and operational.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

Hi Laurent,

On Wed, 26 Mar 2014 18:33:09 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> That could work in your case, but I don't really like that.
> 
> We need to describe the hardware topology, that might be the only point we all 
> agree on. There are various hardware topologies we need to support with 
> different levels of complexity, and several ways to describe them, also with a 
> wide range of complexities and features.
> 
> The more complex the hardware topology, the more complex its description needs 
> to be, and the more complex the code that will parse the description and 
> handle the hardware will be. I don't think there's any doubt here.

But also, the simpler is the topology and the simpler should be its
description.

In your approach, the connections between endpoints are described in
the components themselves, which makes hard for the DT maintainers to
have a global understanding of the video subsystem.

Having the topology described in the master device is clearer, even if
it is complex.

> A given device can be integrated in a wide variety of hardware with different 
> complexities. A driver can't thus always assume that the whole composite 
> display device will match a given class of topologies. This is especially true 
> for encoders and connectors, they can be hooked up directly at the output of a 
> very simple display controller, or can be part of a very complex graph. 
> Encoder and connector drivers can't assume much about how they will be 
> integrated. I want to avoid a situation where using an HDMI encoder already 
> supported in mainline with a different SoC will result in having to rewrite 
> the HDMI encoder driver.

The tda998x chips are simple enough for being used in many boards: one
video input and one serial digital output. I don't see in which
circumstance the driver should be rewritten.

> The story is a bit different for display controllers. While the hardware 
> topology outside (and sometimes inside as well) of the SoC can vary, a display 
> controller often approximately implies a given level of complexity. A cheap 
> SoC with a simple display controller will likely not be used to drive a 4k 
> display requiring 4 encoders running in parallel for instance. While I also 
> want to avoid having to make significant changes to a display controller 
> driver when using the SoC on a different board, I believe the requirement can 
> be slightly relaxed here, and the display controller driver(s) can assume more 
> about the hardware topology than the encoder drivers.

I don't think that the display controllers or the encoders have to know
about the topology. They have well-knwon specific functions and the way
they are interconnected should not impact these functions. If that
would be the case, they should offer a particular configuration
interface to the master driver.

> I've asked myself whether we needed a single, one-size-fits-them-all DT 
> representation of the hardware topology. The view of the world from an 
> external encoder point of view must not depend on the SoC it is hooked up to 
> (this would prevent using a single encoder driver with multiple SoCs), which 
> calls for at least some form of standardization. The topology representation 
> on the display controller side may vary from display controller to display 
> controller, but I believe this would just result in code duplication and 
> having to solve the same problem in multiple drivers. For those reasons I 
> believe that the OF graph proposal to represent the display hardware topology 
> would be a good choice. The bindings are flexible enough to represent both 
> simple and complex hardware.

Your OF graph proposal is rather complex, and it does not even indicate
which way the data flows.

> Now, I don't want to force all display device drivers to implement complex 
> code when they only need to support simple hardware and simple hardware 
> topologies. Not only would that be rightfully rejected, I would be among the 
> ones nack'ing that approach. My opinion is that this calls for the creation of 
> helpers to handle common cases. Several (possibly many) display systems only 
> need (or want) to support linear pipelines at their output(s), made of a 
> single encoder and a single connector. There's no point in duplicating DT 
> parsing or encoder/connector instantiation code in several drivers in that 
> case where helpers could be reused. Several sets of helpers could support 
> different kinds of topologies, with the driver author selecting a set of 
> helpers depending on the expected hardware topology complexity.

I don't like this 'helper' notion. See below.

> We also need to decide on who (as in which driver) will be responsible for 
> binding all devices together. As DRM exposes a single device to userspace, 
> there needs to be a single driver that will perform front line handling of the 
> userspace API and delegate calls to the other drivers involved. I believe it 
> would be logical for that driver to also be in charge of bindings the 
> components together, as it will be the driver that delegate calls to the 
> components. For a similar reason I also believe that that driver should also 
> be the one in control of creating DRM encoders and DRM connectors. The 
> component drivers having only a narrow view of the device they handle, they 
> can't perform that task in a generic way and would thus get tied to specific 
> master drivers because of the assumptions they would make.

I don't see why the encoders and connectors should be created by some
external entity. They are declared in the DT or created by the board
init, so, they exist at system startup time.

> Whether the master driver is the CRTC driver or a separate driver that binds 
> standalone CRTCs, connectors and encoders doesn't in my opinion change my 
> above conclusions. Some SoCs could use a simple-video-card driver like the one 
> you've proposed, and others could implement a display controller driver that 
> would perform the same tasks for more complex pipelines in addition to 
> controlling the display controller's CRTC(s). The simple-video-card driver 
> would perform that same tasks as the helpers I've previously talked about, so 
> the two solutions are pretty similar, and I don't see much added value in the 
> general case in having a simple-video-card driver compared to using helpers in 
> the display controller driver.

In fact, I wonder why there is not only one DRM driver. The global
logic is always the same, and, actually, it is duplicated in each
specific driver. I had this approach in the gspca driver: one main
driver and many specific subdrivers. Once, I even had the idea to
convert your UVC driver into a gspca subdriver, but, at this time, I
had too much work with the other webcams. Nevertheless, it would have
been easy: all the required interfaces were (are still) present in the
main gspca driver.

In the same order of idea, there is only one ALSA SoC driver, and you
cannot say that the audio topology is less complex than the video one.

When thinking about the unique DRM driver, there would no helper anymore:
the driver would offer to each component the functions they need for
working correctly.

> The point that matters to me is that encoders DT bindings and drivers must be 
> identical regardless of whether the master driver is the display controller 
> driver or a driver for a logical composite device. For all those reasons we 
> should use the OF graph DT bindings for the simple-video-card driver should we 
> decide to implement it, and we should create DRM encoders and connectors in 
> the master driver, not in the encoder drivers.

Sorry, but I think that a DRM encoder is the hardware encoder. What
else could it be? At initialization time, the master driver has only to
manage the relation between the video components. It has not to create
entities which already exist. So my preferred scheme is:

- at starting time, each video component initializes its software and
  hardware, and then, plugs itself into the DRM driver.

- from the central video topology, the DRM device waits for all
  components to be plugged and then it links the components together,
  creating the user's view of the system.

If you create the encoders and connectors in the DRM master, the
hardware encoder and connector devices are just like zombies. They must
put something in the system to say they are here and they must also
have a callback function for them to know to which video subsystem they
belong.

On the contrary, if the DRM encoders and connectors are created by the
hardware devices, they are immediately alive and operational.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Jean-Francois Moine]

Hi Laurent,

On Wed, 26 Mar 2014 18:33:09 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> That could work in your case, but I don't really like that.
> 
> We need to describe the hardware topology, that might be the only point we all 
> agree on. There are various hardware topologies we need to support with 
> different levels of complexity, and several ways to describe them, also with a 
> wide range of complexities and features.
> 
> The more complex the hardware topology, the more complex its description needs 
> to be, and the more complex the code that will parse the description and 
> handle the hardware will be. I don't think there's any doubt here.

But also, the simpler is the topology and the simpler should be its
description.

In your approach, the connections between endpoints are described in
the components themselves, which makes hard for the DT maintainers to
have a global understanding of the video subsystem.

Having the topology described in the master device is clearer, even if
it is complex.

> A given device can be integrated in a wide variety of hardware with different 
> complexities. A driver can't thus always assume that the whole composite 
> display device will match a given class of topologies. This is especially true 
> for encoders and connectors, they can be hooked up directly at the output of a 
> very simple display controller, or can be part of a very complex graph. 
> Encoder and connector drivers can't assume much about how they will be 
> integrated. I want to avoid a situation where using an HDMI encoder already 
> supported in mainline with a different SoC will result in having to rewrite 
> the HDMI encoder driver.

The tda998x chips are simple enough for being used in many boards: one
video input and one serial digital output. I don't see in which
circumstance the driver should be rewritten.

> The story is a bit different for display controllers. While the hardware 
> topology outside (and sometimes inside as well) of the SoC can vary, a display 
> controller often approximately implies a given level of complexity. A cheap 
> SoC with a simple display controller will likely not be used to drive a 4k 
> display requiring 4 encoders running in parallel for instance. While I also 
> want to avoid having to make significant changes to a display controller 
> driver when using the SoC on a different board, I believe the requirement can 
> be slightly relaxed here, and the display controller driver(s) can assume more 
> about the hardware topology than the encoder drivers.

I don't think that the display controllers or the encoders have to know
about the topology. They have well-knwon specific functions and the way
they are interconnected should not impact these functions. If that
would be the case, they should offer a particular configuration
interface to the master driver.

> I've asked myself whether we needed a single, one-size-fits-them-all DT 
> representation of the hardware topology. The view of the world from an 
> external encoder point of view must not depend on the SoC it is hooked up to 
> (this would prevent using a single encoder driver with multiple SoCs), which 
> calls for at least some form of standardization. The topology representation 
> on the display controller side may vary from display controller to display 
> controller, but I believe this would just result in code duplication and 
> having to solve the same problem in multiple drivers. For those reasons I 
> believe that the OF graph proposal to represent the display hardware topology 
> would be a good choice. The bindings are flexible enough to represent both 
> simple and complex hardware.

Your OF graph proposal is rather complex, and it does not even indicate
which way the data flows.

> Now, I don't want to force all display device drivers to implement complex 
> code when they only need to support simple hardware and simple hardware 
> topologies. Not only would that be rightfully rejected, I would be among the 
> ones nack'ing that approach. My opinion is that this calls for the creation of 
> helpers to handle common cases. Several (possibly many) display systems only 
> need (or want) to support linear pipelines at their output(s), made of a 
> single encoder and a single connector. There's no point in duplicating DT 
> parsing or encoder/connector instantiation code in several drivers in that 
> case where helpers could be reused. Several sets of helpers could support 
> different kinds of topologies, with the driver author selecting a set of 
> helpers depending on the expected hardware topology complexity.

I don't like this 'helper' notion. See below.

> We also need to decide on who (as in which driver) will be responsible for 
> binding all devices together. As DRM exposes a single device to userspace, 
> there needs to be a single driver that will perform front line handling of the 
> userspace API and delegate calls to the other drivers involved. I believe it 
> would be logical for that driver to also be in charge of bindings the 
> components together, as it will be the driver that delegate calls to the 
> components. For a similar reason I also believe that that driver should also 
> be the one in control of creating DRM encoders and DRM connectors. The 
> component drivers having only a narrow view of the device they handle, they 
> can't perform that task in a generic way and would thus get tied to specific 
> master drivers because of the assumptions they would make.

I don't see why the encoders and connectors should be created by some
external entity. They are declared in the DT or created by the board
init, so, they exist at system startup time.

> Whether the master driver is the CRTC driver or a separate driver that binds 
> standalone CRTCs, connectors and encoders doesn't in my opinion change my 
> above conclusions. Some SoCs could use a simple-video-card driver like the one 
> you've proposed, and others could implement a display controller driver that 
> would perform the same tasks for more complex pipelines in addition to 
> controlling the display controller's CRTC(s). The simple-video-card driver 
> would perform that same tasks as the helpers I've previously talked about, so 
> the two solutions are pretty similar, and I don't see much added value in the 
> general case in having a simple-video-card driver compared to using helpers in 
> the display controller driver.

In fact, I wonder why there is not only one DRM driver. The global
logic is always the same, and, actually, it is duplicated in each
specific driver. I had this approach in the gspca driver: one main
driver and many specific subdrivers. Once, I even had the idea to
convert your UVC driver into a gspca subdriver, but, at this time, I
had too much work with the other webcams. Nevertheless, it would have
been easy: all the required interfaces were (are still) present in the
main gspca driver.

In the same order of idea, there is only one ALSA SoC driver, and you
cannot say that the audio topology is less complex than the video one.

When thinking about the unique DRM driver, there would no helper anymore:
the driver would offer to each component the functions they need for
working correctly.

> The point that matters to me is that encoders DT bindings and drivers must be 
> identical regardless of whether the master driver is the display controller 
> driver or a driver for a logical composite device. For all those reasons we 
> should use the OF graph DT bindings for the simple-video-card driver should we 
> decide to implement it, and we should create DRM encoders and connectors in 
> the master driver, not in the encoder drivers.

Sorry, but I think that a DRM encoder is the hardware encoder. What
else could it be? At initialization time, the master driver has only to
manage the relation between the video components. It has not to create
entities which already exist. So my preferred scheme is:

- at starting time, each video component initializes its software and
  hardware, and then, plugs itself into the DRM driver.

- from the central video topology, the DRM device waits for all
  components to be plugged and then it links the components together,
  creating the user's view of the system.

If you create the encoders and connectors in the DRM master, the
hardware encoder and connector devices are just like zombies. They must
put something in the system to say they are here and they must also
have a callback function for them to know to which video subsystem they
belong.

On the contrary, if the DRM encoders and connectors are created by the
hardware devices, they are immediately alive and operational.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Laurent Pinchart]

Hi Jean-François,

Sorry for the late reply.

On Thursday 27 March 2014 12:34:49 Jean-Francois Moine wrote:
> On Wed, 26 Mar 2014 18:33:09 +0100 Laurent Pinchart wrote:
> > That could work in your case, but I don't really like that.
> > 
> > We need to describe the hardware topology, that might be the only point we
> > all agree on. There are various hardware topologies we need to support
> > with different levels of complexity, and several ways to describe them,
> > also with a wide range of complexities and features.
> > 
> > The more complex the hardware topology, the more complex its description
> > needs to be, and the more complex the code that will parse the
> > description and handle the hardware will be. I don't think there's any
> > doubt here.
>
> But also, the simpler is the topology and the simpler should be its
> description.

I wouldn't put it so strongly. I believe we can slightly relax our 
requirements regarding DT bindings complexity as long as drivers remain 
simple. There's of course no reason to use more complex bindings just for the 
sake of it.

> In your approach, the connections between endpoints are described in the
> components themselves, which makes hard for the DT maintainers to have a
> global understanding of the video subsystem.
> 
> Having the topology described in the master device is clearer, even if it is
> complex.

First of all, let's clarify what a "master device" is. In the "simple-video-
card" example you've proposed the master device is a logical device (with a DT 
node that has no corresponding hardware device). The second approach I can 
think of is to make the IP core that implements the CRTC (which I usually call 
display controller) be the master device. Let's note that the second case 
makes both the link and "global description" DT binding styles possible.

My concern with the "global description" bindings style is that the approach 
only applies to simple hardware and can't be generalized. Now, I'm not too 
concerned about using that approach for simple hardware and a link-based 
approach for more complex hardware. The "slave" components, however, need to 
use a single DT bindings style, regardless of the DT bindings used by the 
master device. That's why I believe we should try to standardize one DT 
bindings style for master devices, even if it results in a slightly more 
complex DT description than strictly needed in some cases.

> > A given device can be integrated in a wide variety of hardware with
> > different complexities. A driver can't thus always assume that the whole
> > composite display device will match a given class of topologies. This is
> > especially true for encoders and connectors, they can be hooked up
> > directly at the output of a very simple display controller, or can be
> > part of a very complex graph. Encoder and connector drivers can't assume
> > much about how they will be integrated. I want to avoid a situation where
> > using an HDMI encoder already supported in mainline with a different SoC
> > will result in having to rewrite the HDMI encoder driver.
> 
> The tda998x chips are simple enough for being used in many boards: one video
> input and one serial digital output. I don't see in which circumstance the
> driver should be rewritten.

It shouldn't, that's the whole point ;-) I wasn't talking about the tda998x 
only, but about encoder drivers in general. I have a display controller in a 
Renesas SoC that has two LVDS encoders that output LVDS signals out of the 
SoC. One LVDS port is connected to an LVDS panel (a connector from a DRM point 
of view), the second one to an LVDS receiver that outputs parallel RGB data to 
an HDMI encoder. The LVDS encoder can't thus assume any particular downstream 
topology and its driver thus can't create DRM encoder and connector instances. 
The same could be true for an HDMI encoder in theory, although an HDMI encoder 
connected on the same board directly to an HDMI decoder that outputs RGB data 
to a panel is probably a case that will be rarely seen in practice.

In the general case an encoder driver can't assume any specific upstream or 
downstream topology. As long as DRM can't expose the real hardware topology to 
userspace, someone needs to decide on how to map the hardware topology to the 
DRM topology exposed to userspace. Encoder drivers can't take that role and 
thus shouldn't create DRM encoder and connector instances themselves.

> > The story is a bit different for display controllers. While the hardware
> > topology outside (and sometimes inside as well) of the SoC can vary, a
> > display controller often approximately implies a given level of
> > complexity. A cheap SoC with a simple display controller will likely not
> > be used to drive a 4k display requiring 4 encoders running in parallel
> > for instance. While I also want to avoid having to make significant
> > changes to a display controller driver when using the SoC on a different
> > board, I believe the requirement can be slightly relaxed here, and the
> > display controller driver(s) can assume more about the hardware topology
> > than the encoder drivers.
> 
> I don't think that the display controllers or the encoders have to know
> about the topology. They have well-knwon specific functions and the way they
> are interconnected should not impact these functions. If that would be the
> case, they should offer a particular configuration interface to the master
> driver.

As explained above, part of the problem comes from the fact that we need to 
expose a logical topology to userspace that doesn't map 1:1 to the hardware 
topology. We can discuss whether or not this should be changed, but I believe 
that's out of scope. As we'll need to map logical encoders and connectors to 
real hardware in the foreseeable future we need a solution to that problem, 
and I believe the display controller should be in control of the mapping 
(possibly using helpers).

> > I've asked myself whether we needed a single, one-size-fits-them-all DT
> > representation of the hardware topology. The view of the world from an
> > external encoder point of view must not depend on the SoC it is hooked up
> > to (this would prevent using a single encoder driver with multiple SoCs),
> > which calls for at least some form of standardization. The topology
> > representation on the display controller side may vary from display
> > controller to display controller, but I believe this would just result in
> > code duplication and having to solve the same problem in multiple
> > drivers. For those reasons I believe that the OF graph proposal to
> > represent the display hardware topology would be a good choice. The
> > bindings are flexible enough to represent both simple and complex
> > hardware.
> 
> Your OF graph proposal is rather complex,

I agree it's slightly more complex than your simple-video-card proposal for 
simple hardware, but I don't see it as overly complex, as long as we can 
provide helper functions that moves the parsing complexity out of drivers (at 
least drivers for simple hardware).

> and it does not even indicate which way the data flows.

Please note that the OF graph DT bindings are not set in stone. If we need to 
indicate the data flow direction (and I assume we will need to at some point) 
that should just be discussed and implemented.

> > Now, I don't want to force all display device drivers to implement complex
> > code when they only need to support simple hardware and simple hardware
> > topologies. Not only would that be rightfully rejected, I would be among
> > the ones nack'ing that approach. My opinion is that this calls for the
> > creation of helpers to handle common cases. Several (possibly many)
> > display systems only need (or want) to support linear pipelines at their
> > output(s), made of a single encoder and a single connector. There's no
> > point in duplicating DT parsing or encoder/connector instantiation code
> > in several drivers in that case where helpers could be reused. Several
> > sets of helpers could support different kinds of topologies, with the
> > driver author selecting a set of helpers depending on the expected
> > hardware topology complexity.
> 
> I don't like this 'helper' notion. See below.
> 
> > We also need to decide on who (as in which driver) will be responsible for
> > binding all devices together. As DRM exposes a single device to userspace,
> > there needs to be a single driver that will perform front line handling of
> > the userspace API and delegate calls to the other drivers involved. I
> > believe it would be logical for that driver to also be in charge of
> > bindings the components together, as it will be the driver that delegate
> > calls to the components. For a similar reason I also believe that that
> > driver should also be the one in control of creating DRM encoders and DRM
> > connectors. The component drivers having only a narrow view of the device
> > they handle, they can't perform that task in a generic way and would thus
> > get tied to specific master drivers because of the assumptions they would
> > make.
> 
> I don't see why the encoders and connectors should be created by some
> external entity. They are declared in the DT or created by the board init,
> so, they exist at system startup time.

As explained above, the hardware encoders and connectors are declared in DT, 
but they don't map 1:1 to the logical encoders and connectors exposed by DRM 
to userspace.

Obviously, in some cases (and probably a majority of cases today), the mapping 
will be 1:1, and the DRM encoders and connectors could be created by the 
encoder drivers, but that won't scale to more complex hardware. The transition 
to DT is a disruption that I'd like to take as an opportunity to model things 
correctly. We *could* transition to DT by creating encoders and connectors in 
encoder drivers, but a second disruption will then be needed in the near 
future to transition to a model that can handle most hardware. The drm_bridge 
API was a first step in such a direction, and I think we should take it into 
account.

> > Whether the master driver is the CRTC driver or a separate driver that
> > binds standalone CRTCs, connectors and encoders doesn't in my opinion
> > change my above conclusions. Some SoCs could use a simple-video-card
> > driver like the one you've proposed, and others could implement a display
> > controller driver that would perform the same tasks for more complex
> > pipelines in addition to controlling the display controller's CRTC(s).
> > The simple-video-card driver would perform that same tasks as the helpers
> > I've previously talked about, so the two solutions are pretty similar,
> > and I don't see much added value in the general case in having a
> > simple-video-card driver compared to using helpers in the display
> > controller driver.
> 
> In fact, I wonder why there is not only one DRM driver. The global logic is
> always the same, and, actually, it is duplicated in each specific driver.

That's quite a good question, and I agree that several DRM drivers, especially 
drivers for embedded systems, are pretty similar in architecture.

The CRTC helpers are a pretty good example of an attempt to share common code 
between drivers. The Intel DRM driver used them, developers then realized that 
it made their life more difficult, and decided to implement the DRM API 
directly instead of using the helpers.

The devil is in the details. Architectures can seem similar, but subtle 
difference can make a common implementation not only complex and difficult to 
maintain (as changing it will impact all drivers), but also useless when it 
tries to cater for the needs of everybody. An abstraction for something that 
can't be abstracted is just pointless.

I'm all for sharing code. This should, in my opinion, be done by providing 
optional helpers that drivers can use. That's how the CRTC helpers work, 
that's how the V4L2 control framework and videobuf2 library work, and that 
allows drivers to provide their own implementation in the rare cases when the 
generic helpers are not good enough. I think simple helpers that work in 90% 
of the cases are much better than really complex helpers that would achieve a 
95% coverage only anyway.

Given that several embedded DRM drivers duplicate code it might be that a 
different set of CRTC helpers could be better for that class of hardware. If 
you want to experiment with you I'd be glad to discuss requirements and 
architectures.

> I had this approach in the gspca driver: one main driver and many specific
> subdrivers. Once, I even had the idea to convert your UVC driver into a
> gspca subdriver, but, at this time, I had too much work with the other
> webcams. Nevertheless, it would have been easy: all the required interfaces
> were (are still) present in the main gspca driver.

If you look into the details of the uvcvideo driver you'll find that it 
wouldn't be that easy. UVC is a special case in that its architecture is 
pretty different from the other webcam drivers. For instance the driver 
doesn't use the V4L2 control framework, as its needs regarding controls are 
quite special. I've discussed this with Hans Verkuil and we've concluded (at 
least for the time being) that implementing the features uvcvideo requires in 
the control framework would make the framework way too complex, just because 
of a single driver.

This is just a counterexample of course. It doesn't mean that code shouldn't 
be shared, quite the contrary.

> In the same order of idea, there is only one ALSA SoC driver, and you cannot
> say that the audio topology is less complex than the video one.

I can't really comment on that, I don't have enough knowledge of ALSA.

> When thinking about the unique DRM driver, there would no helper anymore:
> the driver would offer to each component the functions they need for working
> correctly.

That's where things get hairy. There will be one driver that won't fit in the 
model. There will then be two options: filling the framework with exceptions 
and corner cases for just one user, create a spaghetti mess in common code, or 
allowing drivers to opt-out. I'm a strong believer in the second option. For 
the sake of completeness I want to mention the third option, which is to make 
the framework so thin that in becomes a wrapper that mostly translates 
function calls 1:1, which is pretty useless.

We're getting out of scope here though. I'd be happy to continue discussing 
the single DRM driver with you, but let's do so in a separate mail thread 
then.

> > The point that matters to me is that encoders DT bindings and drivers must
> > be identical regardless of whether the master driver is the display
> > controller driver or a driver for a logical composite device. For all
> > those reasons we should use the OF graph DT bindings for the
> > simple-video-card driver should we decide to implement it, and we should
> > create DRM encoders and connectors in the master driver, not in the
> > encoder drivers.
> 
> Sorry, but I think that a DRM encoder is the hardware encoder. What else
> could it be?

As explained above, a DRM encoder is a logical entity that usually corresponds 
to a hardware encoder, but that can also correspond to several encoders 
chained (sometimes using the drm_bridge abstraction, but not always).

> At initialization time, the master driver has only to manage the relation
> between the video components. It has not to create entities which already
> exist. So my preferred scheme is:
> 
> - at starting time, each video component initializes its software and
>   hardware, and then, plugs itself into the DRM driver.
> 
> - from the central video topology, the DRM device waits for all
>   components to be plugged and then it links the components together,
>   creating the user's view of the system.

So far I agree with you, but I believe this calls for an in-kernel view of the 
components possibly different than the drm_encoder and drm_connector objects.
 
> If you create the encoders and connectors in the DRM master, the hardware
> encoder and connector devices are just like zombies. They must put something
> in the system to say they are here and they must also have a callback
> function for them to know to which video subsystem they belong.
>
> On the contrary, if the DRM encoders and connectors are created by the
> hardware devices, they are immediately alive and operational.

-- 
Regards,

Laurent Pinchart

[PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Laurent Pinchart]

Hi Jean-Fran?ois,

Sorry for the late reply.

On Thursday 27 March 2014 12:34:49 Jean-Francois Moine wrote:
> On Wed, 26 Mar 2014 18:33:09 +0100 Laurent Pinchart wrote:
> > That could work in your case, but I don't really like that.
> > 
> > We need to describe the hardware topology, that might be the only point we
> > all agree on. There are various hardware topologies we need to support
> > with different levels of complexity, and several ways to describe them,
> > also with a wide range of complexities and features.
> > 
> > The more complex the hardware topology, the more complex its description
> > needs to be, and the more complex the code that will parse the
> > description and handle the hardware will be. I don't think there's any
> > doubt here.
>
> But also, the simpler is the topology and the simpler should be its
> description.

I wouldn't put it so strongly. I believe we can slightly relax our 
requirements regarding DT bindings complexity as long as drivers remain 
simple. There's of course no reason to use more complex bindings just for the 
sake of it.

> In your approach, the connections between endpoints are described in the
> components themselves, which makes hard for the DT maintainers to have a
> global understanding of the video subsystem.
> 
> Having the topology described in the master device is clearer, even if it is
> complex.

First of all, let's clarify what a "master device" is. In the "simple-video-
card" example you've proposed the master device is a logical device (with a DT 
node that has no corresponding hardware device). The second approach I can 
think of is to make the IP core that implements the CRTC (which I usually call 
display controller) be the master device. Let's note that the second case 
makes both the link and "global description" DT binding styles possible.

My concern with the "global description" bindings style is that the approach 
only applies to simple hardware and can't be generalized. Now, I'm not too 
concerned about using that approach for simple hardware and a link-based 
approach for more complex hardware. The "slave" components, however, need to 
use a single DT bindings style, regardless of the DT bindings used by the 
master device. That's why I believe we should try to standardize one DT 
bindings style for master devices, even if it results in a slightly more 
complex DT description than strictly needed in some cases.

> > A given device can be integrated in a wide variety of hardware with
> > different complexities. A driver can't thus always assume that the whole
> > composite display device will match a given class of topologies. This is
> > especially true for encoders and connectors, they can be hooked up
> > directly at the output of a very simple display controller, or can be
> > part of a very complex graph. Encoder and connector drivers can't assume
> > much about how they will be integrated. I want to avoid a situation where
> > using an HDMI encoder already supported in mainline with a different SoC
> > will result in having to rewrite the HDMI encoder driver.
> 
> The tda998x chips are simple enough for being used in many boards: one video
> input and one serial digital output. I don't see in which circumstance the
> driver should be rewritten.

It shouldn't, that's the whole point ;-) I wasn't talking about the tda998x 
only, but about encoder drivers in general. I have a display controller in a 
Renesas SoC that has two LVDS encoders that output LVDS signals out of the 
SoC. One LVDS port is connected to an LVDS panel (a connector from a DRM point 
of view), the second one to an LVDS receiver that outputs parallel RGB data to 
an HDMI encoder. The LVDS encoder can't thus assume any particular downstream 
topology and its driver thus can't create DRM encoder and connector instances. 
The same could be true for an HDMI encoder in theory, although an HDMI encoder 
connected on the same board directly to an HDMI decoder that outputs RGB data 
to a panel is probably a case that will be rarely seen in practice.

In the general case an encoder driver can't assume any specific upstream or 
downstream topology. As long as DRM can't expose the real hardware topology to 
userspace, someone needs to decide on how to map the hardware topology to the 
DRM topology exposed to userspace. Encoder drivers can't take that role and 
thus shouldn't create DRM encoder and connector instances themselves.

> > The story is a bit different for display controllers. While the hardware
> > topology outside (and sometimes inside as well) of the SoC can vary, a
> > display controller often approximately implies a given level of
> > complexity. A cheap SoC with a simple display controller will likely not
> > be used to drive a 4k display requiring 4 encoders running in parallel
> > for instance. While I also want to avoid having to make significant
> > changes to a display controller driver when using the SoC on a different
> > board, I believe the requirement can be slightly relaxed here, and the
> > display controller driver(s) can assume more about the hardware topology
> > than the encoder drivers.
> 
> I don't think that the display controllers or the encoders have to know
> about the topology. They have well-knwon specific functions and the way they
> are interconnected should not impact these functions. If that would be the
> case, they should offer a particular configuration interface to the master
> driver.

As explained above, part of the problem comes from the fact that we need to 
expose a logical topology to userspace that doesn't map 1:1 to the hardware 
topology. We can discuss whether or not this should be changed, but I believe 
that's out of scope. As we'll need to map logical encoders and connectors to 
real hardware in the foreseeable future we need a solution to that problem, 
and I believe the display controller should be in control of the mapping 
(possibly using helpers).

> > I've asked myself whether we needed a single, one-size-fits-them-all DT
> > representation of the hardware topology. The view of the world from an
> > external encoder point of view must not depend on the SoC it is hooked up
> > to (this would prevent using a single encoder driver with multiple SoCs),
> > which calls for at least some form of standardization. The topology
> > representation on the display controller side may vary from display
> > controller to display controller, but I believe this would just result in
> > code duplication and having to solve the same problem in multiple
> > drivers. For those reasons I believe that the OF graph proposal to
> > represent the display hardware topology would be a good choice. The
> > bindings are flexible enough to represent both simple and complex
> > hardware.
> 
> Your OF graph proposal is rather complex,

I agree it's slightly more complex than your simple-video-card proposal for 
simple hardware, but I don't see it as overly complex, as long as we can 
provide helper functions that moves the parsing complexity out of drivers (at 
least drivers for simple hardware).

> and it does not even indicate which way the data flows.

Please note that the OF graph DT bindings are not set in stone. If we need to 
indicate the data flow direction (and I assume we will need to at some point) 
that should just be discussed and implemented.

> > Now, I don't want to force all display device drivers to implement complex
> > code when they only need to support simple hardware and simple hardware
> > topologies. Not only would that be rightfully rejected, I would be among
> > the ones nack'ing that approach. My opinion is that this calls for the
> > creation of helpers to handle common cases. Several (possibly many)
> > display systems only need (or want) to support linear pipelines at their
> > output(s), made of a single encoder and a single connector. There's no
> > point in duplicating DT parsing or encoder/connector instantiation code
> > in several drivers in that case where helpers could be reused. Several
> > sets of helpers could support different kinds of topologies, with the
> > driver author selecting a set of helpers depending on the expected
> > hardware topology complexity.
> 
> I don't like this 'helper' notion. See below.
> 
> > We also need to decide on who (as in which driver) will be responsible for
> > binding all devices together. As DRM exposes a single device to userspace,
> > there needs to be a single driver that will perform front line handling of
> > the userspace API and delegate calls to the other drivers involved. I
> > believe it would be logical for that driver to also be in charge of
> > bindings the components together, as it will be the driver that delegate
> > calls to the components. For a similar reason I also believe that that
> > driver should also be the one in control of creating DRM encoders and DRM
> > connectors. The component drivers having only a narrow view of the device
> > they handle, they can't perform that task in a generic way and would thus
> > get tied to specific master drivers because of the assumptions they would
> > make.
> 
> I don't see why the encoders and connectors should be created by some
> external entity. They are declared in the DT or created by the board init,
> so, they exist at system startup time.

As explained above, the hardware encoders and connectors are declared in DT, 
but they don't map 1:1 to the logical encoders and connectors exposed by DRM 
to userspace.

Obviously, in some cases (and probably a majority of cases today), the mapping 
will be 1:1, and the DRM encoders and connectors could be created by the 
encoder drivers, but that won't scale to more complex hardware. The transition 
to DT is a disruption that I'd like to take as an opportunity to model things 
correctly. We *could* transition to DT by creating encoders and connectors in 
encoder drivers, but a second disruption will then be needed in the near 
future to transition to a model that can handle most hardware. The drm_bridge 
API was a first step in such a direction, and I think we should take it into 
account.

> > Whether the master driver is the CRTC driver or a separate driver that
> > binds standalone CRTCs, connectors and encoders doesn't in my opinion
> > change my above conclusions. Some SoCs could use a simple-video-card
> > driver like the one you've proposed, and others could implement a display
> > controller driver that would perform the same tasks for more complex
> > pipelines in addition to controlling the display controller's CRTC(s).
> > The simple-video-card driver would perform that same tasks as the helpers
> > I've previously talked about, so the two solutions are pretty similar,
> > and I don't see much added value in the general case in having a
> > simple-video-card driver compared to using helpers in the display
> > controller driver.
> 
> In fact, I wonder why there is not only one DRM driver. The global logic is
> always the same, and, actually, it is duplicated in each specific driver.

That's quite a good question, and I agree that several DRM drivers, especially 
drivers for embedded systems, are pretty similar in architecture.

The CRTC helpers are a pretty good example of an attempt to share common code 
between drivers. The Intel DRM driver used them, developers then realized that 
it made their life more difficult, and decided to implement the DRM API 
directly instead of using the helpers.

The devil is in the details. Architectures can seem similar, but subtle 
difference can make a common implementation not only complex and difficult to 
maintain (as changing it will impact all drivers), but also useless when it 
tries to cater for the needs of everybody. An abstraction for something that 
can't be abstracted is just pointless.

I'm all for sharing code. This should, in my opinion, be done by providing 
optional helpers that drivers can use. That's how the CRTC helpers work, 
that's how the V4L2 control framework and videobuf2 library work, and that 
allows drivers to provide their own implementation in the rare cases when the 
generic helpers are not good enough. I think simple helpers that work in 90% 
of the cases are much better than really complex helpers that would achieve a 
95% coverage only anyway.

Given that several embedded DRM drivers duplicate code it might be that a 
different set of CRTC helpers could be better for that class of hardware. If 
you want to experiment with you I'd be glad to discuss requirements and 
architectures.

> I had this approach in the gspca driver: one main driver and many specific
> subdrivers. Once, I even had the idea to convert your UVC driver into a
> gspca subdriver, but, at this time, I had too much work with the other
> webcams. Nevertheless, it would have been easy: all the required interfaces
> were (are still) present in the main gspca driver.

If you look into the details of the uvcvideo driver you'll find that it 
wouldn't be that easy. UVC is a special case in that its architecture is 
pretty different from the other webcam drivers. For instance the driver 
doesn't use the V4L2 control framework, as its needs regarding controls are 
quite special. I've discussed this with Hans Verkuil and we've concluded (at 
least for the time being) that implementing the features uvcvideo requires in 
the control framework would make the framework way too complex, just because 
of a single driver.

This is just a counterexample of course. It doesn't mean that code shouldn't 
be shared, quite the contrary.

> In the same order of idea, there is only one ALSA SoC driver, and you cannot
> say that the audio topology is less complex than the video one.

I can't really comment on that, I don't have enough knowledge of ALSA.

> When thinking about the unique DRM driver, there would no helper anymore:
> the driver would offer to each component the functions they need for working
> correctly.

That's where things get hairy. There will be one driver that won't fit in the 
model. There will then be two options: filling the framework with exceptions 
and corner cases for just one user, create a spaghetti mess in common code, or 
allowing drivers to opt-out. I'm a strong believer in the second option. For 
the sake of completeness I want to mention the third option, which is to make 
the framework so thin that in becomes a wrapper that mostly translates 
function calls 1:1, which is pretty useless.

We're getting out of scope here though. I'd be happy to continue discussing 
the single DRM driver with you, but let's do so in a separate mail thread 
then.

> > The point that matters to me is that encoders DT bindings and drivers must
> > be identical regardless of whether the master driver is the display
> > controller driver or a driver for a logical composite device. For all
> > those reasons we should use the OF graph DT bindings for the
> > simple-video-card driver should we decide to implement it, and we should
> > create DRM encoders and connectors in the master driver, not in the
> > encoder drivers.
> 
> Sorry, but I think that a DRM encoder is the hardware encoder. What else
> could it be?

As explained above, a DRM encoder is a logical entity that usually corresponds 
to a hardware encoder, but that can also correspond to several encoders 
chained (sometimes using the drm_bridge abstraction, but not always).

> At initialization time, the master driver has only to manage the relation
> between the video components. It has not to create entities which already
> exist. So my preferred scheme is:
> 
> - at starting time, each video component initializes its software and
>   hardware, and then, plugs itself into the DRM driver.
> 
> - from the central video topology, the DRM device waits for all
>   components to be plugged and then it links the components together,
>   creating the user's view of the system.

So far I agree with you, but I believe this calls for an in-kernel view of the 
components possibly different than the drm_encoder and drm_connector objects.
 
> If you create the encoders and connectors in the DRM master, the hardware
> encoder and connector devices are just like zombies. They must put something
> in the system to say they are here and they must also have a callback
> function for them to know to which video subsystem they belong.
>
> On the contrary, if the DRM encoders and connectors are created by the
> hardware devices, they are immediately alive and operational.

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector

[Laurent Pinchart]

Hi Jean-François,

Sorry for the late reply.

On Thursday 27 March 2014 12:34:49 Jean-Francois Moine wrote:
> On Wed, 26 Mar 2014 18:33:09 +0100 Laurent Pinchart wrote:
> > That could work in your case, but I don't really like that.
> > 
> > We need to describe the hardware topology, that might be the only point we
> > all agree on. There are various hardware topologies we need to support
> > with different levels of complexity, and several ways to describe them,
> > also with a wide range of complexities and features.
> > 
> > The more complex the hardware topology, the more complex its description
> > needs to be, and the more complex the code that will parse the
> > description and handle the hardware will be. I don't think there's any
> > doubt here.
>
> But also, the simpler is the topology and the simpler should be its
> description.

I wouldn't put it so strongly. I believe we can slightly relax our 
requirements regarding DT bindings complexity as long as drivers remain 
simple. There's of course no reason to use more complex bindings just for the 
sake of it.

> In your approach, the connections between endpoints are described in the
> components themselves, which makes hard for the DT maintainers to have a
> global understanding of the video subsystem.
> 
> Having the topology described in the master device is clearer, even if it is
> complex.

First of all, let's clarify what a "master device" is. In the "simple-video-
card" example you've proposed the master device is a logical device (with a DT 
node that has no corresponding hardware device). The second approach I can 
think of is to make the IP core that implements the CRTC (which I usually call 
display controller) be the master device. Let's note that the second case 
makes both the link and "global description" DT binding styles possible.

My concern with the "global description" bindings style is that the approach 
only applies to simple hardware and can't be generalized. Now, I'm not too 
concerned about using that approach for simple hardware and a link-based 
approach for more complex hardware. The "slave" components, however, need to 
use a single DT bindings style, regardless of the DT bindings used by the 
master device. That's why I believe we should try to standardize one DT 
bindings style for master devices, even if it results in a slightly more 
complex DT description than strictly needed in some cases.

> > A given device can be integrated in a wide variety of hardware with
> > different complexities. A driver can't thus always assume that the whole
> > composite display device will match a given class of topologies. This is
> > especially true for encoders and connectors, they can be hooked up
> > directly at the output of a very simple display controller, or can be
> > part of a very complex graph. Encoder and connector drivers can't assume
> > much about how they will be integrated. I want to avoid a situation where
> > using an HDMI encoder already supported in mainline with a different SoC
> > will result in having to rewrite the HDMI encoder driver.
> 
> The tda998x chips are simple enough for being used in many boards: one video
> input and one serial digital output. I don't see in which circumstance the
> driver should be rewritten.

It shouldn't, that's the whole point ;-) I wasn't talking about the tda998x 
only, but about encoder drivers in general. I have a display controller in a 
Renesas SoC that has two LVDS encoders that output LVDS signals out of the 
SoC. One LVDS port is connected to an LVDS panel (a connector from a DRM point 
of view), the second one to an LVDS receiver that outputs parallel RGB data to 
an HDMI encoder. The LVDS encoder can't thus assume any particular downstream 
topology and its driver thus can't create DRM encoder and connector instances. 
The same could be true for an HDMI encoder in theory, although an HDMI encoder 
connected on the same board directly to an HDMI decoder that outputs RGB data 
to a panel is probably a case that will be rarely seen in practice.

In the general case an encoder driver can't assume any specific upstream or 
downstream topology. As long as DRM can't expose the real hardware topology to 
userspace, someone needs to decide on how to map the hardware topology to the 
DRM topology exposed to userspace. Encoder drivers can't take that role and 
thus shouldn't create DRM encoder and connector instances themselves.

> > The story is a bit different for display controllers. While the hardware
> > topology outside (and sometimes inside as well) of the SoC can vary, a
> > display controller often approximately implies a given level of
> > complexity. A cheap SoC with a simple display controller will likely not
> > be used to drive a 4k display requiring 4 encoders running in parallel
> > for instance. While I also want to avoid having to make significant
> > changes to a display controller driver when using the SoC on a different
> > board, I believe the requirement can be slightly relaxed here, and the
> > display controller driver(s) can assume more about the hardware topology
> > than the encoder drivers.
> 
> I don't think that the display controllers or the encoders have to know
> about the topology. They have well-knwon specific functions and the way they
> are interconnected should not impact these functions. If that would be the
> case, they should offer a particular configuration interface to the master
> driver.

As explained above, part of the problem comes from the fact that we need to 
expose a logical topology to userspace that doesn't map 1:1 to the hardware 
topology. We can discuss whether or not this should be changed, but I believe 
that's out of scope. As we'll need to map logical encoders and connectors to 
real hardware in the foreseeable future we need a solution to that problem, 
and I believe the display controller should be in control of the mapping 
(possibly using helpers).

> > I've asked myself whether we needed a single, one-size-fits-them-all DT
> > representation of the hardware topology. The view of the world from an
> > external encoder point of view must not depend on the SoC it is hooked up
> > to (this would prevent using a single encoder driver with multiple SoCs),
> > which calls for at least some form of standardization. The topology
> > representation on the display controller side may vary from display
> > controller to display controller, but I believe this would just result in
> > code duplication and having to solve the same problem in multiple
> > drivers. For those reasons I believe that the OF graph proposal to
> > represent the display hardware topology would be a good choice. The
> > bindings are flexible enough to represent both simple and complex
> > hardware.
> 
> Your OF graph proposal is rather complex,

I agree it's slightly more complex than your simple-video-card proposal for 
simple hardware, but I don't see it as overly complex, as long as we can 
provide helper functions that moves the parsing complexity out of drivers (at 
least drivers for simple hardware).

> and it does not even indicate which way the data flows.

Please note that the OF graph DT bindings are not set in stone. If we need to 
indicate the data flow direction (and I assume we will need to at some point) 
that should just be discussed and implemented.

> > Now, I don't want to force all display device drivers to implement complex
> > code when they only need to support simple hardware and simple hardware
> > topologies. Not only would that be rightfully rejected, I would be among
> > the ones nack'ing that approach. My opinion is that this calls for the
> > creation of helpers to handle common cases. Several (possibly many)
> > display systems only need (or want) to support linear pipelines at their
> > output(s), made of a single encoder and a single connector. There's no
> > point in duplicating DT parsing or encoder/connector instantiation code
> > in several drivers in that case where helpers could be reused. Several
> > sets of helpers could support different kinds of topologies, with the
> > driver author selecting a set of helpers depending on the expected
> > hardware topology complexity.
> 
> I don't like this 'helper' notion. See below.
> 
> > We also need to decide on who (as in which driver) will be responsible for
> > binding all devices together. As DRM exposes a single device to userspace,
> > there needs to be a single driver that will perform front line handling of
> > the userspace API and delegate calls to the other drivers involved. I
> > believe it would be logical for that driver to also be in charge of
> > bindings the components together, as it will be the driver that delegate
> > calls to the components. For a similar reason I also believe that that
> > driver should also be the one in control of creating DRM encoders and DRM
> > connectors. The component drivers having only a narrow view of the device
> > they handle, they can't perform that task in a generic way and would thus
> > get tied to specific master drivers because of the assumptions they would
> > make.
> 
> I don't see why the encoders and connectors should be created by some
> external entity. They are declared in the DT or created by the board init,
> so, they exist at system startup time.

As explained above, the hardware encoders and connectors are declared in DT, 
but they don't map 1:1 to the logical encoders and connectors exposed by DRM 
to userspace.

Obviously, in some cases (and probably a majority of cases today), the mapping 
will be 1:1, and the DRM encoders and connectors could be created by the 
encoder drivers, but that won't scale to more complex hardware. The transition 
to DT is a disruption that I'd like to take as an opportunity to model things 
correctly. We *could* transition to DT by creating encoders and connectors in 
encoder drivers, but a second disruption will then be needed in the near 
future to transition to a model that can handle most hardware. The drm_bridge 
API was a first step in such a direction, and I think we should take it into 
account.

> > Whether the master driver is the CRTC driver or a separate driver that
> > binds standalone CRTCs, connectors and encoders doesn't in my opinion
> > change my above conclusions. Some SoCs could use a simple-video-card
> > driver like the one you've proposed, and others could implement a display
> > controller driver that would perform the same tasks for more complex
> > pipelines in addition to controlling the display controller's CRTC(s).
> > The simple-video-card driver would perform that same tasks as the helpers
> > I've previously talked about, so the two solutions are pretty similar,
> > and I don't see much added value in the general case in having a
> > simple-video-card driver compared to using helpers in the display
> > controller driver.
> 
> In fact, I wonder why there is not only one DRM driver. The global logic is
> always the same, and, actually, it is duplicated in each specific driver.

That's quite a good question, and I agree that several DRM drivers, especially 
drivers for embedded systems, are pretty similar in architecture.

The CRTC helpers are a pretty good example of an attempt to share common code 
between drivers. The Intel DRM driver used them, developers then realized that 
it made their life more difficult, and decided to implement the DRM API 
directly instead of using the helpers.

The devil is in the details. Architectures can seem similar, but subtle 
difference can make a common implementation not only complex and difficult to 
maintain (as changing it will impact all drivers), but also useless when it 
tries to cater for the needs of everybody. An abstraction for something that 
can't be abstracted is just pointless.

I'm all for sharing code. This should, in my opinion, be done by providing 
optional helpers that drivers can use. That's how the CRTC helpers work, 
that's how the V4L2 control framework and videobuf2 library work, and that 
allows drivers to provide their own implementation in the rare cases when the 
generic helpers are not good enough. I think simple helpers that work in 90% 
of the cases are much better than really complex helpers that would achieve a 
95% coverage only anyway.

Given that several embedded DRM drivers duplicate code it might be that a 
different set of CRTC helpers could be better for that class of hardware. If 
you want to experiment with you I'd be glad to discuss requirements and 
architectures.

> I had this approach in the gspca driver: one main driver and many specific
> subdrivers. Once, I even had the idea to convert your UVC driver into a
> gspca subdriver, but, at this time, I had too much work with the other
> webcams. Nevertheless, it would have been easy: all the required interfaces
> were (are still) present in the main gspca driver.

If you look into the details of the uvcvideo driver you'll find that it 
wouldn't be that easy. UVC is a special case in that its architecture is 
pretty different from the other webcam drivers. For instance the driver 
doesn't use the V4L2 control framework, as its needs regarding controls are 
quite special. I've discussed this with Hans Verkuil and we've concluded (at 
least for the time being) that implementing the features uvcvideo requires in 
the control framework would make the framework way too complex, just because 
of a single driver.

This is just a counterexample of course. It doesn't mean that code shouldn't 
be shared, quite the contrary.

> In the same order of idea, there is only one ALSA SoC driver, and you cannot
> say that the audio topology is less complex than the video one.

I can't really comment on that, I don't have enough knowledge of ALSA.

> When thinking about the unique DRM driver, there would no helper anymore:
> the driver would offer to each component the functions they need for working
> correctly.

That's where things get hairy. There will be one driver that won't fit in the 
model. There will then be two options: filling the framework with exceptions 
and corner cases for just one user, create a spaghetti mess in common code, or 
allowing drivers to opt-out. I'm a strong believer in the second option. For 
the sake of completeness I want to mention the third option, which is to make 
the framework so thin that in becomes a wrapper that mostly translates 
function calls 1:1, which is pretty useless.

We're getting out of scope here though. I'd be happy to continue discussing 
the single DRM driver with you, but let's do so in a separate mail thread 
then.

> > The point that matters to me is that encoders DT bindings and drivers must
> > be identical regardless of whether the master driver is the display
> > controller driver or a driver for a logical composite device. For all
> > those reasons we should use the OF graph DT bindings for the
> > simple-video-card driver should we decide to implement it, and we should
> > create DRM encoders and connectors in the master driver, not in the
> > encoder drivers.
> 
> Sorry, but I think that a DRM encoder is the hardware encoder. What else
> could it be?

As explained above, a DRM encoder is a logical entity that usually corresponds 
to a hardware encoder, but that can also correspond to several encoders 
chained (sometimes using the drm_bridge abstraction, but not always).

> At initialization time, the master driver has only to manage the relation
> between the video components. It has not to create entities which already
> exist. So my preferred scheme is:
> 
> - at starting time, each video component initializes its software and
>   hardware, and then, plugs itself into the DRM driver.
> 
> - from the central video topology, the DRM device waits for all
>   components to be plugged and then it links the components together,
>   creating the user's view of the system.

So far I agree with you, but I believe this calls for an in-kernel view of the 
components possibly different than the drm_encoder and drm_connector objects.
 
> If you create the encoders and connectors in the DRM master, the hardware
> encoder and connector devices are just like zombies. They must put something
> in the system to say they are here and they must also have a callback
> function for them to know to which video subsystem they belong.
>
> On the contrary, if the DRM encoders and connectors are created by the
> hardware devices, they are immediately alive and operational.

-- 
Regards,

Laurent Pinchart