MC fixes, improvements and cleanups

Tuners actually have at least one connector on its input.

Add a PAD to connect it.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/dvb-core/dvbdev.c +4 -1

index f00f1a5f279c..a8e7e2398f7a 100644
@@ -34,6 +34,9 @@
 #include <linux/mutex.h>
 #include "dvbdev.h"
 
+/* Due to enum tuner_pad_index */
+#include <media/tuner.h>
+
 static DEFINE_MUTEX(dvbdev_mutex);
 static int dvbdev_debug;
 

@@ -552,7 +555,7 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
 	}
 
 	if (tuner && demod)
-		media_create_pad_link(tuner, 0, demod, 0, 0);
+		media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, demod, 0, 0);
 
 	if (demod && demux)
 		media_create_pad_link(demod, 1, demux, 0, MEDIA_LNK_FL_ENABLED);

drivers/media/usb/au0828/au0828-core.c +4 -1

index e28cabe65934..f54c7d10f350 100644
@@ -27,6 +27,9 @@
 #include <media/v4l2-common.h>
 #include <linux/mutex.h>
 
+/* Due to enum tuner_pad_index */
+#include <media/tuner.h>
+
 /*
  * 1 = General debug messages
  * 2 = USB handling

@@ -260,7 +263,7 @@ static void au0828_create_media_graph(struct au0828_dev *dev)
 		return;
 
 	if (tuner)
-		media_create_pad_link(tuner, 0, decoder, 0,
+		media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0,
 				      MEDIA_LNK_FL_ENABLED);
 	media_create_pad_link(decoder, 1, &dev->vdev.entity, 0,
 			      MEDIA_LNK_FL_ENABLED);

drivers/media/usb/cx231xx/cx231xx-cards.c +1 -1

index 3b5c9ae39ad3..1070d87efc65 100644
@@ -1264,7 +1264,7 @@ static void cx231xx_create_media_graph(struct cx231xx *dev)
 		return;
 
 	if (tuner)
-		media_create_pad_link(tuner, 0, decoder, 0,
+		media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0,
 					 MEDIA_LNK_FL_ENABLED);
 	media_create_pad_link(decoder, 1, &dev->vdev.entity, 0,
 				 MEDIA_LNK_FL_ENABLED);

drivers/media/v4l2-core/tuner-core.c +5 -3

index 100b8f069640..b90f2a52db96 100644
@@ -134,8 +134,9 @@ struct tuner {
 	unsigned int        type; /* chip type id */
 	void                *config;
 	const char          *name;
+
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	struct media_pad	pad;
+	struct media_pad	pad[TUNER_NUM_PADS];
 #endif
 };
 

@@ -695,11 +696,12 @@ static int tuner_probe(struct i2c_client *client,
 	/* Should be just before return */
 register_client:
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	t->pad.flags = MEDIA_PAD_FL_SOURCE;
+	t->pad[TUNER_PAD_RF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	t->pad[TUNER_PAD_IF_OUTPUT].flags = MEDIA_PAD_FL_SOURCE;
 	t->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_TUNER;
 	t->sd.entity.name = t->name;
 
-	ret = media_entity_init(&t->sd.entity, 1, &t->pad);
+	ret = media_entity_init(&t->sd.entity, TUNER_NUM_PADS, &t->pad[0]);
 	if (ret < 0) {
 		tuner_err("failed to initialize media entity!\n");
 		kfree(t);

include/media/tuner.h +8 -0

index b46ebb48fe74..95835c8069dd 100644
@@ -25,6 +25,14 @@
 
 #include <linux/videodev2.h>
 
+/* Tuner PADs */
+/* FIXME: is this the right place for it? */
+enum tuner_pad_index {
+	TUNER_PAD_RF_INPUT,
+	TUNER_PAD_IF_OUTPUT,
+	TUNER_NUM_PADS
+};
+
 #define ADDR_UNSET (255)
 
 #define TUNER_TEMIC_PAL			0        /* 4002 FH5 (3X 7756, 9483) */

Tuners actually have at least one connector on its input.

Add a PAD to connect it.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(quoted diff) +0 -0

b/drivers/media/dvb-core/dvbdev.c index f00f1a5f279c..a8e7e2398f7a 100644

drivers/media/dvb-core/dvbdev.c +4 -1

@@ -34,6 +34,9 @@
 #include <linux/mutex.h>
 #include "dvbdev.h"
 
+/* Due to enum tuner_pad_index */
+#include <media/tuner.h>
+
 static DEFINE_MUTEX(dvbdev_mutex);
 static int dvbdev_debug;
 

@@ -552,7 +555,7 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
 	}
 
 	if (tuner && demod)
-		media_create_pad_link(tuner, 0, demod, 0, 0);
+		media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, demod, 0, 0);
 
 	if (demod && demux)
 		media_create_pad_link(demod, 1, demux, 0, MEDIA_LNK_FL_ENABLED);

(quoted diff) +0 -0

b/drivers/media/usb/au0828/au0828-core.c index e28cabe65934..f54c7d10f350 100644

drivers/media/usb/au0828/au0828-core.c +4 -1

@@ -27,6 +27,9 @@
 #include <media/v4l2-common.h>
 #include <linux/mutex.h>
 
+/* Due to enum tuner_pad_index */
+#include <media/tuner.h>
+
 /*
  * 1 = General debug messages
  * 2 = USB handling

@@ -260,7 +263,7 @@ static void au0828_create_media_graph(struct au0828_dev
 *dev) return;
 
 	if (tuner)
-		media_create_pad_link(tuner, 0, decoder, 0,
+		media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0,
 				      MEDIA_LNK_FL_ENABLED);
 	media_create_pad_link(decoder, 1, &dev->vdev.entity, 0,
 			      MEDIA_LNK_FL_ENABLED);

(quoted diff) +0 -0

b/drivers/media/usb/cx231xx/cx231xx-cards.c index
3b5c9ae39ad3..1070d87efc65 100644

drivers/media/usb/cx231xx/cx231xx-cards.c +1 -1

@@ -1264,7 +1264,7 @@ static void cx231xx_create_media_graph(struct cx231xx
 *dev) return;
 
 	if (tuner)
-		media_create_pad_link(tuner, 0, decoder, 0,
+		media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0,
 					 MEDIA_LNK_FL_ENABLED);
 	media_create_pad_link(decoder, 1, &dev->vdev.entity, 0,
 				 MEDIA_LNK_FL_ENABLED);

(quoted diff) +0 -0

b/drivers/media/v4l2-core/tuner-core.c index 100b8f069640..b90f2a52db96 100644

drivers/media/v4l2-core/tuner-core.c +5 -3

@@ -134,8 +134,9 @@ struct tuner {
 	unsigned int        type; /* chip type id */
 	void                *config;
 	const char          *name;
+
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	struct media_pad	pad;
+	struct media_pad	pad[TUNER_NUM_PADS];
 #endif
 };
 

@@ -695,11 +696,12 @@ static int tuner_probe(struct i2c_client *client,
 	/* Should be just before return */
 register_client:
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	t->pad.flags = MEDIA_PAD_FL_SOURCE;
+	t->pad[TUNER_PAD_RF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	t->pad[TUNER_PAD_IF_OUTPUT].flags = MEDIA_PAD_FL_SOURCE;
 	t->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_TUNER;
 	t->sd.entity.name = t->name;
 
-	ret = media_entity_init(&t->sd.entity, 1, &t->pad);
+	ret = media_entity_init(&t->sd.entity, TUNER_NUM_PADS, &t->pad[0]);
 	if (ret < 0) {
 		tuner_err("failed to initialize media entity!\n");
 		kfree(t);

include/media/tuner.h +8 -0

index b46ebb48fe74..95835c8069dd 100644
@@ -25,6 +25,14 @@
 
 #include <linux/videodev2.h>
 
+/* Tuner PADs */
+/* FIXME: is this the right place for it? */
+enum tuner_pad_index {
+	TUNER_PAD_RF_INPUT,
+	TUNER_PAD_IF_OUTPUT,
+	TUNER_NUM_PADS
+};
+
 #define ADDR_UNSET (255)
 
 #define TUNER_TEMIC_PAL			0        /* 4002 FH5 (3X 7756, 9483) */

Hi Mauro,

Thank you for the patch.

On Sunday 06 September 2015 14:30:44 Mauro Carvalho Chehab wrote:

Quoted 4 lines

Tuners actually have at least one connector on its input.

Add a PAD to connect it.

The patch looks fine to me, but have you checked that there are no driver instantiating a tuner that would get confused by this additional pad, for instance in graph traversal code ?

Additionally it should be documented somewhere that drivers instantiating tuners are responsible for creating and linking a connector to the tuner input.

Quoted 114 lines

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(quoted diff) +0 -0

b/drivers/media/dvb-core/dvbdev.c index f00f1a5f279c..a8e7e2398f7a 100644

drivers/media/dvb-core/dvbdev.c +4 -1

@@ -34,6 +34,9 @@
 #include <linux/mutex.h>
 #include "dvbdev.h"
 
+/* Due to enum tuner_pad_index */
+#include <media/tuner.h>
+
 static DEFINE_MUTEX(dvbdev_mutex);
 static int dvbdev_debug;
 

@@ -552,7 +555,7 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
 	}
 
 	if (tuner && demod)
-		media_create_pad_link(tuner, 0, demod, 0, 0);
+		media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, demod, 0, 0);
 
 	if (demod && demux)
 		media_create_pad_link(demod, 1, demux, 0, MEDIA_LNK_FL_ENABLED);

(quoted diff) +0 -0

b/drivers/media/usb/au0828/au0828-core.c index e28cabe65934..f54c7d10f350 100644

drivers/media/usb/au0828/au0828-core.c +4 -1

@@ -27,6 +27,9 @@
 #include <media/v4l2-common.h>
 #include <linux/mutex.h>
 
+/* Due to enum tuner_pad_index */
+#include <media/tuner.h>
+
 /*
  * 1 = General debug messages
  * 2 = USB handling

@@ -260,7 +263,7 @@ static void au0828_create_media_graph(struct au0828_dev
 *dev) return;
 
 	if (tuner)
-		media_create_pad_link(tuner, 0, decoder, 0,
+		media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0,
 				      MEDIA_LNK_FL_ENABLED);
 	media_create_pad_link(decoder, 1, &dev->vdev.entity, 0,
 			      MEDIA_LNK_FL_ENABLED);

(quoted diff) +0 -0

b/drivers/media/usb/cx231xx/cx231xx-cards.c index
3b5c9ae39ad3..1070d87efc65 100644

drivers/media/usb/cx231xx/cx231xx-cards.c +1 -1

@@ -1264,7 +1264,7 @@ static void cx231xx_create_media_graph(struct cx231xx
 *dev) return;
 
 	if (tuner)
-		media_create_pad_link(tuner, 0, decoder, 0,
+		media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0,
 					 MEDIA_LNK_FL_ENABLED);
 	media_create_pad_link(decoder, 1, &dev->vdev.entity, 0,
 				 MEDIA_LNK_FL_ENABLED);

(quoted diff) +0 -0

b/drivers/media/v4l2-core/tuner-core.c index 100b8f069640..b90f2a52db96 100644

drivers/media/v4l2-core/tuner-core.c +5 -3

@@ -134,8 +134,9 @@ struct tuner {
 	unsigned int        type; /* chip type id */
 	void                *config;
 	const char          *name;
+
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	struct media_pad	pad;
+	struct media_pad	pad[TUNER_NUM_PADS];
 #endif
 };
 

@@ -695,11 +696,12 @@ static int tuner_probe(struct i2c_client *client,
 	/* Should be just before return */
 register_client:
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	t->pad.flags = MEDIA_PAD_FL_SOURCE;
+	t->pad[TUNER_PAD_RF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	t->pad[TUNER_PAD_IF_OUTPUT].flags = MEDIA_PAD_FL_SOURCE;
 	t->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_TUNER;
 	t->sd.entity.name = t->name;
 
-	ret = media_entity_init(&t->sd.entity, 1, &t->pad);
+	ret = media_entity_init(&t->sd.entity, TUNER_NUM_PADS, &t->pad[0]);
 	if (ret < 0) {
 		tuner_err("failed to initialize media entity!\n");
 		kfree(t);

include/media/tuner.h +8 -0

index b46ebb48fe74..95835c8069dd 100644
@@ -25,6 +25,14 @@
 
 #include <linux/videodev2.h>
 
+/* Tuner PADs */
+/* FIXME: is this the right place for it? */
+enum tuner_pad_index {
+	TUNER_PAD_RF_INPUT,
+	TUNER_PAD_IF_OUTPUT,
+	TUNER_NUM_PADS
+};
+
 #define ADDR_UNSET (255)
 
 #define TUNER_TEMIC_PAL			0        /* 4002 FH5 (3X 7756, 9483) */

Depending on the input, an au0828 may have a different number of connectors. add entities to represent them.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/usb/au0828/au0828-core.c +15 -0

index f54c7d10f350..fe9a60484343 100644
@@ -153,11 +153,26 @@ static void au0828_usb_release(struct au0828_dev *dev)
 }
 
 #ifdef CONFIG_VIDEO_AU0828_V4L2
+
+static void au0828_usb_v4l2_media_release(struct au0828_dev *dev)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+	int i;
+
+	for (i = 0; i < AU0828_MAX_INPUT; i++) {
+		if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED)
+			return;
+		media_device_unregister_entity(&dev->input_ent[i]);
+	}
+#endif
+}
+
 static void au0828_usb_v4l2_release(struct v4l2_device *v4l2_dev)
 {
 	struct au0828_dev *dev =
 		container_of(v4l2_dev, struct au0828_dev, v4l2_dev);
 
+	au0828_usb_v4l2_media_release(dev);
 	v4l2_ctrl_handler_free(&dev->v4l2_ctrl_hdl);
 	v4l2_device_unregister(&dev->v4l2_dev);
 	au0828_usb_release(dev);

drivers/media/usb/au0828/au0828-video.c +65 -11

index 4511e2893282..806b8d320bae 100644
@@ -1793,6 +1793,69 @@ static int au0828_vb2_setup(struct au0828_dev *dev)
 	return 0;
 }
 
+static void au0828_analog_create_entities(struct au0828_dev *dev)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	static const char *inames[] = {
+		[AU0828_VMUX_COMPOSITE] = "Composite",
+		[AU0828_VMUX_SVIDEO] = "S-Video",
+		[AU0828_VMUX_CABLE] = "Cable TV",
+		[AU0828_VMUX_TELEVISION] = "Television",
+		[AU0828_VMUX_DVB] = "DVB",
+		[AU0828_VMUX_DEBUG] = "tv debug"
+	};
+	int ret, i;
+
+	/* Initialize Video and VBI pads */
+	dev->video_pad.flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_init(&dev->vdev.entity, 1, &dev->video_pad);
+	if (ret < 0)
+		pr_err("failed to initialize video media entity!\n");
+
+	dev->vbi_pad.flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_init(&dev->vbi_dev.entity, 1, &dev->vbi_pad);
+	if (ret < 0)
+		pr_err("failed to initialize vbi media entity!\n");
+
+	/* Create entities for each input connector */
+	for (i = 0; i < AU0828_MAX_INPUT; i++) {
+		struct media_entity *ent = &dev->input_ent[i];
+
+		if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED)
+			break;
+
+		ent->name = inames[AUVI_INPUT(i).type];
+		ent->flags = MEDIA_ENT_FL_CONNECTOR;
+		dev->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
+
+		switch(AUVI_INPUT(i).type) {
+		case AU0828_VMUX_COMPOSITE:
+			ent->type = MEDIA_ENT_T_CONN_COMPOSITE;
+			break;
+		case AU0828_VMUX_SVIDEO:
+			ent->type = MEDIA_ENT_T_CONN_SVIDEO;
+			break;
+		case AU0828_VMUX_CABLE:
+		case AU0828_VMUX_TELEVISION:
+		case AU0828_VMUX_DVB:
+			ent->type = MEDIA_ENT_T_CONN_RF;
+			break;
+		default: /* AU0828_VMUX_DEBUG */
+			ent->type = MEDIA_ENT_T_CONN_TEST;
+			break;
+		}
+
+		ret = media_entity_init(ent, 1, &dev->input_pad[i]);
+		if (ret < 0)
+			pr_err("failed to initialize input pad[%d]!\n", i);
+
+		ret = media_device_register_entity(dev->media_dev, ent);
+		if (ret < 0)
+			pr_err("failed to register input entity %d!\n", i);
+	}
+#endif
+}
+
 /**************************************************************************/
 
 int au0828_analog_register(struct au0828_dev *dev,

@@ -1881,17 +1944,8 @@ int au0828_analog_register(struct au0828_dev *dev,
 	dev->vbi_dev.queue->lock = &dev->vb_vbi_queue_lock;
 	strcpy(dev->vbi_dev.name, "au0828a vbi");
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	dev->video_pad.flags = MEDIA_PAD_FL_SINK;
-	ret = media_entity_init(&dev->vdev.entity, 1, &dev->video_pad);
-	if (ret < 0)
-		pr_err("failed to initialize video media entity!\n");
-
-	dev->vbi_pad.flags = MEDIA_PAD_FL_SINK;
-	ret = media_entity_init(&dev->vbi_dev.entity, 1, &dev->vbi_pad);
-	if (ret < 0)
-		pr_err("failed to initialize vbi media entity!\n");
-#endif
+	/* Init entities at the Media Controller */
+	au0828_analog_create_entities(dev);
 
 	/* initialize videobuf2 stuff */
 	retval = au0828_vb2_setup(dev);

drivers/media/usb/au0828/au0828.h +2 -1

index d3644b3fe6fa..b7940c54d006 100644
@@ -93,7 +93,6 @@ struct au0828_board {
 	unsigned char has_ir_i2c:1;
 	unsigned char has_analog:1;
 	struct au0828_input input[AU0828_MAX_INPUT];
-
 };
 
 struct au0828_dvb {

@@ -281,6 +280,8 @@ struct au0828_dev {
 	struct media_device *media_dev;
 	struct media_pad video_pad, vbi_pad;
 	struct media_entity *decoder;
+	struct media_entity input_ent[AU0828_MAX_INPUT];
+	struct media_pad input_pad[AU0828_MAX_INPUT];
 #endif
 };
 

Quoted 2 lines

Depending on the input, an au0828 may have a different number of connectors. add entities to represent them.

Hmm, this patch uses the new connector defines that are only added in patch 6! So this doesn't compile.

Is there a reason why the connector support is needed now? I would prefer to have this in a separate follow-up patch series.

Regards,

Hans

Quoted 142 lines

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/usb/au0828/au0828-core.c +15 -0

index f54c7d10f350..fe9a60484343 100644
@@ -153,11 +153,26 @@ static void au0828_usb_release(struct au0828_dev *dev)
 }
 
 #ifdef CONFIG_VIDEO_AU0828_V4L2
+
+static void au0828_usb_v4l2_media_release(struct au0828_dev *dev)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+	int i;
+
+	for (i = 0; i < AU0828_MAX_INPUT; i++) {
+		if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED)
+			return;
+		media_device_unregister_entity(&dev->input_ent[i]);
+	}
+#endif
+}
+
 static void au0828_usb_v4l2_release(struct v4l2_device *v4l2_dev)
 {
 	struct au0828_dev *dev =
 		container_of(v4l2_dev, struct au0828_dev, v4l2_dev);
 
+	au0828_usb_v4l2_media_release(dev);
 	v4l2_ctrl_handler_free(&dev->v4l2_ctrl_hdl);
 	v4l2_device_unregister(&dev->v4l2_dev);
 	au0828_usb_release(dev);

drivers/media/usb/au0828/au0828-video.c +65 -11

index 4511e2893282..806b8d320bae 100644
@@ -1793,6 +1793,69 @@ static int au0828_vb2_setup(struct au0828_dev *dev)
 	return 0;
 }
 
+static void au0828_analog_create_entities(struct au0828_dev *dev)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	static const char *inames[] = {
+		[AU0828_VMUX_COMPOSITE] = "Composite",
+		[AU0828_VMUX_SVIDEO] = "S-Video",
+		[AU0828_VMUX_CABLE] = "Cable TV",
+		[AU0828_VMUX_TELEVISION] = "Television",
+		[AU0828_VMUX_DVB] = "DVB",
+		[AU0828_VMUX_DEBUG] = "tv debug"
+	};
+	int ret, i;
+
+	/* Initialize Video and VBI pads */
+	dev->video_pad.flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_init(&dev->vdev.entity, 1, &dev->video_pad);
+	if (ret < 0)
+		pr_err("failed to initialize video media entity!\n");
+
+	dev->vbi_pad.flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_init(&dev->vbi_dev.entity, 1, &dev->vbi_pad);
+	if (ret < 0)
+		pr_err("failed to initialize vbi media entity!\n");
+
+	/* Create entities for each input connector */
+	for (i = 0; i < AU0828_MAX_INPUT; i++) {
+		struct media_entity *ent = &dev->input_ent[i];
+
+		if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED)
+			break;
+
+		ent->name = inames[AUVI_INPUT(i).type];
+		ent->flags = MEDIA_ENT_FL_CONNECTOR;
+		dev->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
+
+		switch(AUVI_INPUT(i).type) {
+		case AU0828_VMUX_COMPOSITE:
+			ent->type = MEDIA_ENT_T_CONN_COMPOSITE;
+			break;
+		case AU0828_VMUX_SVIDEO:
+			ent->type = MEDIA_ENT_T_CONN_SVIDEO;
+			break;
+		case AU0828_VMUX_CABLE:
+		case AU0828_VMUX_TELEVISION:
+		case AU0828_VMUX_DVB:
+			ent->type = MEDIA_ENT_T_CONN_RF;
+			break;
+		default: /* AU0828_VMUX_DEBUG */
+			ent->type = MEDIA_ENT_T_CONN_TEST;
+			break;
+		}
+
+		ret = media_entity_init(ent, 1, &dev->input_pad[i]);
+		if (ret < 0)
+			pr_err("failed to initialize input pad[%d]!\n", i);
+
+		ret = media_device_register_entity(dev->media_dev, ent);
+		if (ret < 0)
+			pr_err("failed to register input entity %d!\n", i);
+	}
+#endif
+}
+
 /**************************************************************************/
 
 int au0828_analog_register(struct au0828_dev *dev,

@@ -1881,17 +1944,8 @@ int au0828_analog_register(struct au0828_dev *dev,
 	dev->vbi_dev.queue->lock = &dev->vb_vbi_queue_lock;
 	strcpy(dev->vbi_dev.name, "au0828a vbi");
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	dev->video_pad.flags = MEDIA_PAD_FL_SINK;
-	ret = media_entity_init(&dev->vdev.entity, 1, &dev->video_pad);
-	if (ret < 0)
-		pr_err("failed to initialize video media entity!\n");
-
-	dev->vbi_pad.flags = MEDIA_PAD_FL_SINK;
-	ret = media_entity_init(&dev->vbi_dev.entity, 1, &dev->vbi_pad);
-	if (ret < 0)
-		pr_err("failed to initialize vbi media entity!\n");
-#endif
+	/* Init entities at the Media Controller */
+	au0828_analog_create_entities(dev);
 
 	/* initialize videobuf2 stuff */
 	retval = au0828_vb2_setup(dev);

drivers/media/usb/au0828/au0828.h +0 -1

index d3644b3fe6fa..b7940c54d006 100644
@@ -93,7 +93,6 @@ struct au0828_board {
 	unsigned char has_ir_i2c:1;
 	unsigned char has_analog:1;
 	struct au0828_input input[AU0828_MAX_INPUT];
-
 };
 
 struct au0828_dvb {

drivers/media/usb/au0828/au0828.h struct au0828_dev { · line 287

Quoted 8 lines

drivers/media/usb/au0828/au0828.h +2 -0

@@ -281,6 +280,8 @@ struct au0828_dev {
 	struct media_device *media_dev;
 	struct media_pad video_pad, vbi_pad;
 	struct media_entity *decoder;
+	struct media_entity input_ent[AU0828_MAX_INPUT];
+	struct media_pad input_pad[AU0828_MAX_INPUT];
 #endif
 };
 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Now that entities have a main function, expose it via MEDIA_IOC_G_TOPOLOGY ioctl.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/media-device.c +1 -0

index ccef9621d147..32090030c342 100644
@@ -263,6 +263,7 @@ static long __media_device_get_topology(struct media_device *mdev,
 		/* Copy fields to userspace struct if not error */
 		memset(&uentity, 0, sizeof(uentity));
 		uentity.id = entity->graph_obj.id;
+		uentity.function = entity->function;
 		strncpy(uentity.name, entity->name,
 			sizeof(uentity.name));
 

include/uapi/linux/media.h +2 -1

index 69433405aec2..d232cc680c67 100644
@@ -284,7 +284,8 @@ struct media_links_enum {
 struct media_v2_entity {
 	__u32 id;
 	char name[64];		/* FIXME: move to a property? (RFC says so) */
-	__u16 reserved[14];
+	__u32 function;		/* Main function of the entity */
+	__u16 reserved[12];
 };
 
 /* Should match the specific fields at media_intf_devnode */

Now that entities have a main function, expose it via MEDIA_IOC_G_TOPOLOGY ioctl.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/media-device.c +0 -0

index ccef9621d147..32090030c342 100644
@@ -263,6 +263,7 @@ static long __media_device_get_topology(struct

media_device *mdev, /* Copy fields to userspace struct if not error */

drivers/media/media-device.c +1 -0

 		memset(&uentity, 0, sizeof(uentity));
 		uentity.id = entity->graph_obj.id;
+		uentity.function = entity->function;
 		strncpy(uentity.name, entity->name,
 			sizeof(uentity.name));

include/uapi/linux/media.h +0 -0

index 69433405aec2..d232cc680c67 100644

include/uapi/linux/media.h +1 -0

+	__u16 reserved[12];
 };
 
 /* Should match the specific fields at media_intf_devnode */

Hi Mauro,

Thank you for the patch.

Quoted 17 lines · On Sunday 06 September 2015 14:30:57 Mauro Carvalho Chehab wrote:

Now that entities have a main function, expose it via MEDIA_IOC_G_TOPOLOGY ioctl.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/media-device.c +0 -0

index ccef9621d147..32090030c342 100644
@@ -263,6 +263,7 @@ static long __media_device_get_topology(struct

media_device *mdev, /* Copy fields to userspace struct if not error */

drivers/media/media-device.c +1 -0

 		memset(&uentity, 0, sizeof(uentity));
 		uentity.id = entity->graph_obj.id;
+		uentity.function = entity->function;
 		strncpy(uentity.name, entity->name,
 			sizeof(uentity.name));

include/uapi/linux/media.h struct media_links_enum { · line 287

Quoted 10 lines

include/uapi/linux/media.h +1 -1

index 69433405aec2..d232cc680c67 100644
@@ -284,7 +284,8 @@ struct media_links_enum {
 struct media_v2_entity {
 	__u32 id;
 	char name[64];		/* FIXME: move to a property? (RFC says so) */
-	__u16 reserved[14];
+	__u32 function;		/* Main function of the entity */

Shouldn't we use kerneldoc instead of inline comments ?

Also, as this is the main function only, I'd mention that in the subject line.

The implementation itself looks fine to me, I'll discuss the API over the documentation patch.

Quoted 5 lines

include/uapi/linux/media.h +1 -0

+	__u16 reserved[12];
 };
 
 /* Should match the specific fields at media_intf_devnode */

@@
@@

(quoted diff) +1 -1

-MEDIA_ENT_T_DVB_CA
+MEDIA_ENT_F_DTV_CA

@@
@@

(quoted diff) +1 -1

-MEDIA_ENT_T_DVB_NET_DECAP
+MEDIA_ENT_F_DTV_NET_DECAP

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/dvb-core/dvbdev.c +1 -1

index e925909bc99e..8527fc40e6a0 100644
@@ -229,7 +229,7 @@ static int dvb_create_tsout_entity(struct dvb_device *dvbdev,
 		if (!entity->name)
 			return ret;
 
-		entity->function = MEDIA_ENT_T_DVB_TSOUT;
+		entity->function = MEDIA_ENT_F_IO;
 		pads->flags = MEDIA_PAD_FL_SINK;
 
 		ret = media_entity_init(entity, 1, pads);

include/uapi/linux/media.h +1 -1

index d232cc680c67..90e90a6e62bf 100644
@@ -46,87 +46,86 @@ struct media_device_info {
  * Initial value to be used when a new entity is created
  * Drivers should change it to something useful
  */
-#define MEDIA_ENT_T_UNKNOWN	0x00000000
+#define MEDIA_ENT_F_UNKNOWN	0x00000000
 
 /*

include/uapi/linux/media.h +51 -55

+ * the ranges to identify a group of function types, as newer
+ * functions can be added with any name within the full u32 range.
  */
-#define MEDIA_ENT_T_DVB_BASE		0x00000000
-#define MEDIA_ENT_T_V4L2_BASE		0x00010000
-#define MEDIA_ENT_T_V4L2_SUBDEV_BASE	0x00020000
-#define MEDIA_ENT_T_CONNECTOR_BASE	0x00030000
+#define MEDIA_ENT_F_BASE		0x00000000
+#define MEDIA_ENT_F_OLD_BASE		0x00010000
+#define MEDIA_ENT_F_OLD_SUBDEV_BASE	0x00020000
 
 /*
- * V4L2 entities - Those are used for DMA (mmap/DMABUF) and
- *	read()/write() data I/O associated with the V4L2 devnodes.
+ * DVB entities
  */
-#define MEDIA_ENT_T_V4L2_VIDEO		(MEDIA_ENT_T_V4L2_BASE + 1)
-	/*
-	 * Please notice that numbers between MEDIA_ENT_T_V4L2_BASE + 2 and
-	 * MEDIA_ENT_T_V4L2_BASE + 4 can't be used, as those values used
-	 * to be declared for FB, ALSA and DVB entities.
-	 * As those values were never actually used in practice, we're just
-	 * adding them as backward compatibility macros and keeping the
-	 * numberspace clean here. This way, we avoid breaking compilation,
-	 * in the case of having some userspace application using the old
-	 * symbols.
-	 */
-#define MEDIA_ENT_T_V4L2_VBI		(MEDIA_ENT_T_V4L2_BASE + 5)
-#define MEDIA_ENT_T_V4L2_SWRADIO	(MEDIA_ENT_T_V4L2_BASE + 6)
-
-/* V4L2 Sub-device entities */
-
-	/*
-	 * Subdevs are initialized with MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN,
-	 * in order to preserve backward compatibility.
-	 * Drivers should change to the proper subdev type before
-	 * registering the entity.
-	 */
-#define MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_T_V4L2_SUBDEV_BASE
-
-#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 1)
-#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 2)
-#define MEDIA_ENT_T_V4L2_SUBDEV_LENS	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 3)
-	/* A converter of analogue video to its digital representation. */
-#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 4)
-	/* Tuner entity is actually both V4L2 and DVB subdev */
-#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 5)
+#define MEDIA_ENT_F_DTV_DEMOD		(MEDIA_ENT_F_BASE + 1)
+#define MEDIA_ENT_F_MPEG_TS_DEMUX	(MEDIA_ENT_F_BASE + 2)
+#define MEDIA_ENT_F_DTV_CA		(MEDIA_ENT_F_BASE + 3)
+#define MEDIA_ENT_F_DTV_NET_DECAP	(MEDIA_ENT_F_BASE + 4)
 
-/* DVB entities */
-#define MEDIA_ENT_T_DVB_DEMOD		(MEDIA_ENT_T_DVB_BASE + 1)
-#define MEDIA_ENT_T_DVB_DEMUX		(MEDIA_ENT_T_DVB_BASE + 2)
-#define MEDIA_ENT_T_DVB_TSOUT		(MEDIA_ENT_T_DVB_BASE + 3)
-#define MEDIA_ENT_T_DVB_CA		(MEDIA_ENT_T_DVB_BASE + 4)
-#define MEDIA_ENT_T_DVB_NET_DECAP	(MEDIA_ENT_T_DVB_BASE + 5)
-
-/* Connectors */
-#define MEDIA_ENT_T_CONN_RF		(MEDIA_ENT_T_CONNECTOR_BASE)
-#define MEDIA_ENT_T_CONN_SVIDEO		(MEDIA_ENT_T_CONNECTOR_BASE + 1)
-#define MEDIA_ENT_T_CONN_COMPOSITE	(MEDIA_ENT_T_CONNECTOR_BASE + 2)
+/*
+ * Connectors
+ */
+#define MEDIA_ENT_F_CONN_RF		(MEDIA_ENT_F_BASE + 21)
+#define MEDIA_ENT_F_CONN_SVIDEO		(MEDIA_ENT_F_BASE + 22)
+#define MEDIA_ENT_F_CONN_COMPOSITE	(MEDIA_ENT_F_BASE + 23)
 	/* For internal test signal generators and other debug connectors */
-#define MEDIA_ENT_T_CONN_TEST		(MEDIA_ENT_T_CONNECTOR_BASE + 3)
+#define MEDIA_ENT_F_CONN_TEST		(MEDIA_ENT_F_BASE + 24)
+
+/*
+ * Don't touch on those. The ranges MEDIA_ENT_F_OLD_BASE and
+ * MEDIA_ENT_F_OLD_SUBDEV_BASE are kept to keep backward compatibility
+ * with the legacy v1 API.The number range is out of range by purpose:
+ * several previously reserved numbers got excluded from this range.
+ *
+ * Subdevs are initialized with MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN,
+ * in order to preserve backward compatibility.
+ * Drivers should change to the proper subdev type before
+ * registering the entity.
+ */
+
+#define MEDIA_ENT_F_IO  		(MEDIA_ENT_F_OLD_BASE + 1)
+
+#define MEDIA_ENT_F_CAM_SENSOR		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 1)
+#define MEDIA_ENT_F_FLASH		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 2)
+#define MEDIA_ENT_F_LENS		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
+#define MEDIA_ENT_F_ATV_DECODER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
+#define MEDIA_ENT_F_TUNER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
+
+#define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_F_OLD_SUBDEV_BASE
 
 #ifndef __KERNEL__
-/* Legacy symbols used to avoid userspace compilation breakages */
+
+/*
+ * Legacy symbols used to avoid userspace compilation breakages
+ *
+ * Those symbols map the entity function into types and should be
+ * used only on legacy programs for legacy hardware. Don't rely
+ * on those for MEDIA_IOC_G_TOPOLOGY.
+ */
 #define MEDIA_ENT_TYPE_SHIFT		16
 #define MEDIA_ENT_TYPE_MASK		0x00ff0000
 #define MEDIA_ENT_SUBTYPE_MASK		0x0000ffff
 
-#define MEDIA_ENT_T_DEVNODE		MEDIA_ENT_T_V4L2_BASE
-#define MEDIA_ENT_T_V4L2_SUBDEV		MEDIA_ENT_T_V4L2_SUBDEV_BASE
-
-#define MEDIA_ENT_T_DEVNODE_V4L		MEDIA_ENT_T_V4L2_VIDEO
-
+#define MEDIA_ENT_T_DEVNODE		MEDIA_ENT_F_OLD_BASE
+#define MEDIA_ENT_T_DEVNODE_V4L		MEDIA_ENT_F_IO
 #define MEDIA_ENT_T_DEVNODE_FB		(MEDIA_ENT_T_DEVNODE + 2)
 #define MEDIA_ENT_T_DEVNODE_ALSA	(MEDIA_ENT_T_DEVNODE + 3)
 #define MEDIA_ENT_T_DEVNODE_DVB		(MEDIA_ENT_T_DEVNODE + 4)
+
+#define MEDIA_ENT_T_UNKNOWN		MEDIA_ENT_F_UNKNOWN

Quoted 6 lines

Rename the userspace types from MEDIA_ENT_T_ to MEDIA_ENT_F_ and add the backward compatibility bits.

The changes at the .c files was generated by the following coccinelle script:

Review

Quoted 3 lines

@@

(quoted diff) +1 -1

-MEDIA_ENT_T_DVB_DEMUX
+MEDIA_ENT_F_MPEG_TS_DEMUX

I'm not sure about the 'MPEG_' part here. I think that in general a transport stream can contain non-MPEG streams as well. Why not just say _F_TS_DEMUX?

Review

Quoted 5 lines

@@
@@

(quoted diff) +1 -1

-MEDIA_ENT_T_DVB_TSOUT
+MEDIA_ENT_F_DTV_TSOUT

Shouldn't this be MEDIA_ENT_F_IO?

Quoted 37 lines

@@
@@

(quoted diff) +1 -1

-MEDIA_ENT_T_DVB_CA
+MEDIA_ENT_F_DTV_CA

@@
@@

(quoted diff) +1 -1

-MEDIA_ENT_T_DVB_NET_DECAP
+MEDIA_ENT_F_DTV_NET_DECAP

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/dvb-core/dvbdev.c +1 -1

index e925909bc99e..8527fc40e6a0 100644
@@ -229,7 +229,7 @@ static int dvb_create_tsout_entity(struct dvb_device *dvbdev,
 		if (!entity->name)
 			return ret;
 
-		entity->function = MEDIA_ENT_T_DVB_TSOUT;
+		entity->function = MEDIA_ENT_F_IO;
 		pads->flags = MEDIA_PAD_FL_SINK;
 
 		ret = media_entity_init(entity, 1, pads);

include/uapi/linux/media.h +1 -1

index d232cc680c67..90e90a6e62bf 100644
@@ -46,87 +46,86 @@ struct media_device_info {
  * Initial value to be used when a new entity is created
  * Drivers should change it to something useful
  */
-#define MEDIA_ENT_T_UNKNOWN	0x00000000
+#define MEDIA_ENT_F_UNKNOWN	0x00000000
 
 /*

include/uapi/linux/media.h struct media_device_info { · line 54

Quoted 10 lines

include/uapi/linux/media.h +2 -7

- * Base numbers for entity types
+ * Base number ranges for entity functions
  *
- * Please notice that the huge gap of 16 bits for each base is overkill!
- * 8 bits is more than enough to avoid starving entity types for each
- * subsystem.
- *
- * However, It is kept this way just to avoid binary breakages with the
- * namespace provided on legacy versions of this header.
+ * NOTE: those ranges and entity function number are spased just to

s/spased/phased/

include/uapi/linux/media.h struct media_device_info { · line 55

Quoted 1 line

include/uapi/linux/media.h +1 -0

+ * make easier to maintain this file. Userspace should not rely on

s/make/make it/

Quoted 130 lines

include/uapi/linux/media.h +58 -55

+ * the ranges to identify a group of function types, as newer
+ * functions can be added with any name within the full u32 range.
  */
-#define MEDIA_ENT_T_DVB_BASE		0x00000000
-#define MEDIA_ENT_T_V4L2_BASE		0x00010000
-#define MEDIA_ENT_T_V4L2_SUBDEV_BASE	0x00020000
-#define MEDIA_ENT_T_CONNECTOR_BASE	0x00030000
+#define MEDIA_ENT_F_BASE		0x00000000
+#define MEDIA_ENT_F_OLD_BASE		0x00010000
+#define MEDIA_ENT_F_OLD_SUBDEV_BASE	0x00020000
 
 /*
- * V4L2 entities - Those are used for DMA (mmap/DMABUF) and
- *	read()/write() data I/O associated with the V4L2 devnodes.
+ * DVB entities
  */
-#define MEDIA_ENT_T_V4L2_VIDEO		(MEDIA_ENT_T_V4L2_BASE + 1)
-	/*
-	 * Please notice that numbers between MEDIA_ENT_T_V4L2_BASE + 2 and
-	 * MEDIA_ENT_T_V4L2_BASE + 4 can't be used, as those values used
-	 * to be declared for FB, ALSA and DVB entities.
-	 * As those values were never actually used in practice, we're just
-	 * adding them as backward compatibility macros and keeping the
-	 * numberspace clean here. This way, we avoid breaking compilation,
-	 * in the case of having some userspace application using the old
-	 * symbols.
-	 */
-#define MEDIA_ENT_T_V4L2_VBI		(MEDIA_ENT_T_V4L2_BASE + 5)
-#define MEDIA_ENT_T_V4L2_SWRADIO	(MEDIA_ENT_T_V4L2_BASE + 6)
-
-/* V4L2 Sub-device entities */
-
-	/*
-	 * Subdevs are initialized with MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN,
-	 * in order to preserve backward compatibility.
-	 * Drivers should change to the proper subdev type before
-	 * registering the entity.
-	 */
-#define MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_T_V4L2_SUBDEV_BASE
-
-#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 1)
-#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 2)
-#define MEDIA_ENT_T_V4L2_SUBDEV_LENS	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 3)
-	/* A converter of analogue video to its digital representation. */
-#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 4)
-	/* Tuner entity is actually both V4L2 and DVB subdev */
-#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 5)
+#define MEDIA_ENT_F_DTV_DEMOD		(MEDIA_ENT_F_BASE + 1)
+#define MEDIA_ENT_F_MPEG_TS_DEMUX	(MEDIA_ENT_F_BASE + 2)
+#define MEDIA_ENT_F_DTV_CA		(MEDIA_ENT_F_BASE + 3)
+#define MEDIA_ENT_F_DTV_NET_DECAP	(MEDIA_ENT_F_BASE + 4)
 
-/* DVB entities */
-#define MEDIA_ENT_T_DVB_DEMOD		(MEDIA_ENT_T_DVB_BASE + 1)
-#define MEDIA_ENT_T_DVB_DEMUX		(MEDIA_ENT_T_DVB_BASE + 2)
-#define MEDIA_ENT_T_DVB_TSOUT		(MEDIA_ENT_T_DVB_BASE + 3)
-#define MEDIA_ENT_T_DVB_CA		(MEDIA_ENT_T_DVB_BASE + 4)
-#define MEDIA_ENT_T_DVB_NET_DECAP	(MEDIA_ENT_T_DVB_BASE + 5)
-
-/* Connectors */
-#define MEDIA_ENT_T_CONN_RF		(MEDIA_ENT_T_CONNECTOR_BASE)
-#define MEDIA_ENT_T_CONN_SVIDEO		(MEDIA_ENT_T_CONNECTOR_BASE + 1)
-#define MEDIA_ENT_T_CONN_COMPOSITE	(MEDIA_ENT_T_CONNECTOR_BASE + 2)
+/*
+ * Connectors
+ */
+#define MEDIA_ENT_F_CONN_RF		(MEDIA_ENT_F_BASE + 21)
+#define MEDIA_ENT_F_CONN_SVIDEO		(MEDIA_ENT_F_BASE + 22)
+#define MEDIA_ENT_F_CONN_COMPOSITE	(MEDIA_ENT_F_BASE + 23)
 	/* For internal test signal generators and other debug connectors */
-#define MEDIA_ENT_T_CONN_TEST		(MEDIA_ENT_T_CONNECTOR_BASE + 3)
+#define MEDIA_ENT_F_CONN_TEST		(MEDIA_ENT_F_BASE + 24)
+
+/*
+ * Don't touch on those. The ranges MEDIA_ENT_F_OLD_BASE and
+ * MEDIA_ENT_F_OLD_SUBDEV_BASE are kept to keep backward compatibility
+ * with the legacy v1 API.The number range is out of range by purpose:
+ * several previously reserved numbers got excluded from this range.
+ *
+ * Subdevs are initialized with MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN,
+ * in order to preserve backward compatibility.
+ * Drivers should change to the proper subdev type before
+ * registering the entity.
+ */
+
+#define MEDIA_ENT_F_IO  		(MEDIA_ENT_F_OLD_BASE + 1)
+
+#define MEDIA_ENT_F_CAM_SENSOR		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 1)
+#define MEDIA_ENT_F_FLASH		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 2)
+#define MEDIA_ENT_F_LENS		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
+#define MEDIA_ENT_F_ATV_DECODER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
+#define MEDIA_ENT_F_TUNER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
+
+#define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_F_OLD_SUBDEV_BASE
 
 #ifndef __KERNEL__
-/* Legacy symbols used to avoid userspace compilation breakages */
+
+/*
+ * Legacy symbols used to avoid userspace compilation breakages
+ *
+ * Those symbols map the entity function into types and should be
+ * used only on legacy programs for legacy hardware. Don't rely
+ * on those for MEDIA_IOC_G_TOPOLOGY.
+ */
 #define MEDIA_ENT_TYPE_SHIFT		16
 #define MEDIA_ENT_TYPE_MASK		0x00ff0000
 #define MEDIA_ENT_SUBTYPE_MASK		0x0000ffff
 
-#define MEDIA_ENT_T_DEVNODE		MEDIA_ENT_T_V4L2_BASE
-#define MEDIA_ENT_T_V4L2_SUBDEV		MEDIA_ENT_T_V4L2_SUBDEV_BASE
-
-#define MEDIA_ENT_T_DEVNODE_V4L		MEDIA_ENT_T_V4L2_VIDEO
-
+#define MEDIA_ENT_T_DEVNODE		MEDIA_ENT_F_OLD_BASE
+#define MEDIA_ENT_T_DEVNODE_V4L		MEDIA_ENT_F_IO
 #define MEDIA_ENT_T_DEVNODE_FB		(MEDIA_ENT_T_DEVNODE + 2)
 #define MEDIA_ENT_T_DEVNODE_ALSA	(MEDIA_ENT_T_DEVNODE + 3)
 #define MEDIA_ENT_T_DEVNODE_DVB		(MEDIA_ENT_T_DEVNODE + 4)
+
+#define MEDIA_ENT_T_UNKNOWN		MEDIA_ENT_F_UNKNOWN
+#define MEDIA_ENT_T_V4L2_VIDEO		MEDIA_ENT_F_IO
+#define MEDIA_ENT_T_V4L2_SUBDEV		MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN
+#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR	MEDIA_ENT_F_CAM_SENSOR
+#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH	MEDIA_ENT_F_FLASH
+#define MEDIA_ENT_T_V4L2_SUBDEV_LENS	MEDIA_ENT_F_LENS
+#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	MEDIA_ENT_F_ATV_DECODER
+#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	MEDIA_ENT_F_TUNER
 #endif
 

include/uapi/linux/media.h struct media_device_info { · line 131

Quoted 2 lines

include/uapi/linux/media.h +0 -0

 /* Entity flags */

Overall I really like this.

Due to the graph traversal algorithm currently in usage, we need a copy of all data links. Those backlinks should not be send to userspace, as otherwise, all links there will be duplicated.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/media-device.c +3 -0

index 0238885fcc74..97eb97d9b662 100644
@@ -333,6 +333,9 @@ static long __media_device_get_topology(struct media_device *mdev,
 	/* Get links and number of links */
 	i = 0;
 	media_device_for_each_link(link, mdev) {
+		if (link->is_backlink)
+			continue;
+
 		i++;
 
 		if (ret || !topo->links)

drivers/media/media-entity.c +1 -0

index cd4d767644df..4868b8269204 100644
@@ -648,6 +648,7 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
 	backlink->source = &source->pads[source_pad];
 	backlink->sink = &sink->pads[sink_pad];
 	backlink->flags = flags;
+	backlink->is_backlink = true;
 
 	/* Initialize graph object embedded at the new link */
 	media_gobj_init(sink->graph_obj.mdev, MEDIA_GRAPH_LINK,

include/media/media-entity.h +2 -0

index e1a89899deef..3d389f142a1d 100644
@@ -96,6 +96,7 @@ struct media_pipeline {
  * @reverse:	Pointer to the link for the reverse direction of a pad to pad
  *		link.
  * @flags:	Link flags, as defined at uapi/media.h (MEDIA_LNK_FL_*)
+ * @is_backlink: Indicate if the link is a backlink.
  */
 struct media_link {
 	struct media_gobj graph_obj;

@@ -112,6 +113,7 @@ struct media_link {
 	};
 	struct media_link *reverse;
 	unsigned long flags;
+	bool is_backlink;
 };
 
 /**

Due to the graph traversal algorithm currently in usage, we need a copy of all data links. Those backlinks should not be send to userspace, as otherwise, all links there will be duplicated.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/media-device.c +0 -0

index 0238885fcc74..97eb97d9b662 100644
@@ -333,6 +333,9 @@ static long __media_device_get_topology(struct

media_device *mdev, /* Get links and number of links */

drivers/media/media-device.c +3 -0

 	i = 0;
 	media_device_for_each_link(link, mdev) {
+		if (link->is_backlink)
+			continue;
+
 		i++;
 
 		if (ret || !topo->links)

drivers/media/media-entity.c +1 -0

index cd4d767644df..4868b8269204 100644
@@ -648,6 +648,7 @@ media_create_pad_link(struct media_entity *source, u16
 source_pad, backlink->source = &source->pads[source_pad];
 	backlink->sink = &sink->pads[sink_pad];
 	backlink->flags = flags;
+	backlink->is_backlink = true;
 
 	/* Initialize graph object embedded at the new link */
 	media_gobj_init(sink->graph_obj.mdev, MEDIA_GRAPH_LINK,

include/media/media-entity.h +0 -0

index e1a89899deef..3d389f142a1d 100644

include/media/media-entity.h +0 -0

 };
 
 /**

Hi Mauro,

Thank you for the patch.

Quoted 38 lines · On Sunday 06 September 2015 14:30:47 Mauro Carvalho Chehab wrote:

Due to the graph traversal algorithm currently in usage, we need a copy of all data links. Those backlinks should not be send to userspace, as otherwise, all links there will be duplicated.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/media-device.c +0 -0

index 0238885fcc74..97eb97d9b662 100644
@@ -333,6 +333,9 @@ static long __media_device_get_topology(struct

media_device *mdev, /* Get links and number of links */

drivers/media/media-device.c +3 -0

 	i = 0;
 	media_device_for_each_link(link, mdev) {
+		if (link->is_backlink)
+			continue;
+
 		i++;
 
 		if (ret || !topo->links)

drivers/media/media-entity.c +1 -0

index cd4d767644df..4868b8269204 100644
@@ -648,6 +648,7 @@ media_create_pad_link(struct media_entity *source, u16
 source_pad, backlink->source = &source->pads[source_pad];
 	backlink->sink = &sink->pads[sink_pad];
 	backlink->flags = flags;
+	backlink->is_backlink = true;
 
 	/* Initialize graph object embedded at the new link */
 	media_gobj_init(sink->graph_obj.mdev, MEDIA_GRAPH_LINK,

include/media/media-entity.h +0 -0

index e1a89899deef..3d389f142a1d 100644

include/media/media-entity.h struct media_pipeline { · line 96

Quoted 1 line

include/media/media-entity.h +0 -0

@@ -96,6 +96,7 @@ struct media_pipeline {
  * @reverse:	Pointer to the link for the reverse direction of a pad to 

pad

include/media/media-entity.h struct media_link { · line 116

Quoted 11 lines

include/media/media-entity.h +2 -0

  *		link.
  * @flags:	Link flags, as defined at uapi/media.h (MEDIA_LNK_FL_*)
+ * @is_backlink: Indicate if the link is a backlink.
  */
 struct media_link {
 	struct media_gobj graph_obj;

@@ -112,6 +113,7 @@ struct media_link {
 	};
 	struct media_link *reverse;
 	unsigned long flags;
+	bool is_backlink;

I agree with the purpose of this patch (and as stated for other patches in the series I believe you should squash it with the patch that introduces the G_TOPOLOGY ioctl) but I won't whether you couldn't do with the additional variable by adding a flag for backlinks (flag that wouldn't be shown to userspace).

Now that I think about it an even better implementation could be to avoid creating backlinks at all. As links are now dynamically allocated you could have two struct list_head in the link structure, one for the source and one for the sink. It sounds too easy to be true, I wonder if I'm overlooking something.

Interface links are normally enabled, meaning that the interfaces are bound to the entities. So, any ioctl send to the interface are reflected at the entities managed by the interface.

However, when a device is usage, other interfaces for the same hardware

could be decoupled from the entities linked to them, because the hardware may have some parts busy.

That's for example, what happens when an hybrid TV device is in usage.

If it is streaming analog TV or capturing signals from S-Video/Composite connectors, typically the digital part of the hardware can't be used and vice-versa.

This is generally due to some internal hardware or firmware limitation, that it is not easily mapped via data pipelines.

What the Kernel drivers do internally is that they decouple the hardware from the interface. So, all changes, if allowed, are done only at some interface cache, but not physically changed at the hardware.

The usage is similar to the usage of the MEDIA_LNK_FL_ENABLED on data links. So, let's use the same flag to indicate if ether the interface

to entity link is bound/enabled or not.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/dvb-core/dvbdev.c +11 -6

index a8e7e2398f7a..5c4fb41060b4 100644
@@ -396,7 +396,8 @@ static void dvb_register_media_device(struct dvb_device *dvbdev,
 	if (!dvbdev->entity || !dvbdev->intf_devnode)
 		return;
 
-	media_create_intf_link(dvbdev->entity, &dvbdev->intf_devnode->intf, 0);
+	media_create_intf_link(dvbdev->entity, &dvbdev->intf_devnode->intf,
+			       MEDIA_LNK_FL_ENABLED);
 
 #endif
 }

@@ -583,20 +584,24 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
 	/* Create indirect interface links for FE->tuner, DVR->demux and CA->ca */
 	media_device_for_each_intf(intf, mdev) {
 		if (intf->type == MEDIA_INTF_T_DVB_CA && ca)
-			media_create_intf_link(ca, intf, 0);
+			media_create_intf_link(ca, intf, MEDIA_LNK_FL_ENABLED);
 
 		if (intf->type == MEDIA_INTF_T_DVB_FE && tuner)
-			media_create_intf_link(tuner, intf, 0);
+			media_create_intf_link(tuner, intf,
+					       MEDIA_LNK_FL_ENABLED);
 
 		if (intf->type == MEDIA_INTF_T_DVB_DVR && demux)
-			media_create_intf_link(demux, intf, 0);
+			media_create_intf_link(demux, intf,
+					       MEDIA_LNK_FL_ENABLED);
 
 		media_device_for_each_entity(entity, mdev) {
 			if (entity->type == MEDIA_ENT_T_DVB_TSOUT) {
 				if (!strcmp(entity->name, DVR_TSOUT))
-					media_create_intf_link(entity, intf, 0);
+					media_create_intf_link(entity, intf,
+							       MEDIA_LNK_FL_ENABLED);
 				if (!strcmp(entity->name, DEMUX_TSOUT))
-					media_create_intf_link(entity, intf, 0);
+					media_create_intf_link(entity, intf,
+							       MEDIA_LNK_FL_ENABLED);
 				break;
 			}
 		}

drivers/media/v4l2-core/v4l2-dev.c +2 -1

index 07123dd569c4..8429da66754a 100644
@@ -788,7 +788,8 @@ static int video_register_media_controller(struct video_device *vdev, int type)
 		struct media_link *link;
 
 		link = media_create_intf_link(&vdev->entity,
-					      &vdev->intf_devnode->intf, 0);
+					      &vdev->intf_devnode->intf,
+					      MEDIA_LNK_FL_ENABLED);
 		if (!link) {
 			media_devnode_remove(vdev->intf_devnode);
 			media_device_unregister_entity(&vdev->entity);

drivers/media/v4l2-core/v4l2-device.c +1 -1

index e788a085ba96..bb58d90fde5e 100644
@@ -256,7 +256,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 
 			link = media_create_intf_link(&sd->entity,
 						      &vdev->intf_devnode->intf,
-						      0);
+						      MEDIA_LNK_FL_ENABLED);
 			if (!link)
 				goto clean_up;
 		}

Interface links are normally enabled, meaning that the interfaces are bound to the entities. So, any ioctl send to the interface are reflected

at the entities managed by the interface.

However, when a device is usage, other interfaces for the same hardware

could be decoupled from the entities linked to them, because the hardware may have some parts busy.

That's for example, what happens when an hybrid TV device is in usage.

If it is streaming analog TV or capturing signals from S-Video/Composite connectors, typically the digital part of the hardware can't be used and vice-versa.

This is generally due to some internal hardware or firmware limitation, that it is not easily mapped via data pipelines.

What the Kernel drivers do internally is that they decouple the hardware from the interface. So, all changes, if allowed, are done only at some interface cache, but not physically changed at the hardware.

The usage is similar to the usage of the MEDIA_LNK_FL_ENABLED on data links. So, let's use the same flag to indicate if ether the interface

to entity link is bound/enabled or not.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(quoted diff) +0 -0

b/drivers/media/dvb-core/dvbdev.c index a8e7e2398f7a..5c4fb41060b4 100644

drivers/media/dvb-core/dvbdev.c +2 -1

@@ -396,7 +396,8 @@ static void dvb_register_media_device(struct dvb_device
 *dvbdev, if (!dvbdev->entity || !dvbdev->intf_devnode)
 		return;
 
-	media_create_intf_link(dvbdev->entity, &dvbdev->intf_devnode->intf, 0);
+	media_create_intf_link(dvbdev->entity, &dvbdev->intf_devnode->intf,
+			       MEDIA_LNK_FL_ENABLED);
 
 #endif
 }

drivers/media/dvb-core/dvbdev.c +9 -5

 media_device_for_each_intf(intf, mdev) {
 		if (intf->type == MEDIA_INTF_T_DVB_CA && ca)
-			media_create_intf_link(ca, intf, 0);
+			media_create_intf_link(ca, intf, MEDIA_LNK_FL_ENABLED);
 
 		if (intf->type == MEDIA_INTF_T_DVB_FE && tuner)
-			media_create_intf_link(tuner, intf, 0);
+			media_create_intf_link(tuner, intf,
+					       MEDIA_LNK_FL_ENABLED);
 
 		if (intf->type == MEDIA_INTF_T_DVB_DVR && demux)
-			media_create_intf_link(demux, intf, 0);
+			media_create_intf_link(demux, intf,
+					       MEDIA_LNK_FL_ENABLED);
 
 		media_device_for_each_entity(entity, mdev) {
 			if (entity->type == MEDIA_ENT_T_DVB_TSOUT) {
 				if (!strcmp(entity->name, DVR_TSOUT))
-					media_create_intf_link(entity, intf, 0);
+					media_create_intf_link(entity, intf,
+							       MEDIA_LNK_FL_ENABLED);
 				if (!strcmp(entity->name, DEMUX_TSOUT))
-					media_create_intf_link(entity, intf, 0);
+					media_create_intf_link(entity, intf,
+							       MEDIA_LNK_FL_ENABLED);
 				break;
 			}
 		}

(quoted diff) +0 -0

b/drivers/media/v4l2-core/v4l2-dev.c index 07123dd569c4..8429da66754a 100644

drivers/media/v4l2-core/v4l2-dev.c +2 -1

@@ -788,7 +788,8 @@ static int video_register_media_controller(struct
 video_device *vdev, int type) struct media_link *link;
 
 		link = media_create_intf_link(&vdev->entity,
-					      &vdev->intf_devnode->intf, 0);
+					      &vdev->intf_devnode->intf,
+					      MEDIA_LNK_FL_ENABLED);
 		if (!link) {
 			media_devnode_remove(vdev->intf_devnode);
 			media_device_unregister_entity(&vdev->entity);

(quoted diff) +0 -0

b/drivers/media/v4l2-core/v4l2-device.c index e788a085ba96..bb58d90fde5e 100644

drivers/media/v4l2-core/v4l2-device.c +1 -1

@@ -256,7 +256,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device
 *v4l2_dev)
 
 			link = media_create_intf_link(&sd->entity,
 						      &vdev->intf_devnode->intf,
-						      0);
+						      MEDIA_LNK_FL_ENABLED);
 			if (!link)
 				goto clean_up;

}

Hi Mauro,

Thank you for the patch.

Quoted 2 lines · On Sunday 06 September 2015 14:30:48 Mauro Carvalho Chehab wrote:

Interface links are normally enabled, meaning that the interfaces are bound to the entities. So, any ioctl send to the interface are reflected

s/send/sent/

Quoted 3 lines

at the entities managed by the interface.

However, when a device is usage, other interfaces for the same hardware

s/usage/in use/

Quoted 4 lines

could be decoupled from the entities linked to them, because the hardware may have some parts busy.

That's for example, what happens when an hybrid TV device is in usage.

s/usage/use/

Quoted 7 lines

If it is streaming analog TV or capturing signals from S-Video/Composite connectors, typically the digital part of the hardware can't be used and vice-versa.

This is generally due to some internal hardware or firmware limitation, that it is not easily mapped via data pipelines.

Quoted 6 lines

What the Kernel drivers do internally is that they decouple the hardware from the interface. So, all changes, if allowed, are done only at some interface cache, but not physically changed at the hardware.

The usage is similar to the usage of the MEDIA_LNK_FL_ENABLED on data links. So, let's use the same flag to indicate if ether the interface

s/ether/either/

Quoted 1 line

to entity link is bound/enabled or not.

I believe we'll need to experiment with the interface links to see what's really needed there. As a general rule I'd like to avoid exposing too much information to userspace without a clear indication that the information is actually needed, as it's always easier to expose additional information later than to remove information already exposed.

For this reason I'd like to see as a first step how we would do in userspace without making those links dynamic. If we then realize that we're lacking information we'll decide on the best course of action and on exactly what to expose and how to expose it, using concrete userspace use cases.

Quoted 18 lines

Signed-off-by Mauro Carvalho Chehab <[email protected]>

(quoted diff) +0 -0

b/drivers/media/dvb-core/dvbdev.c index a8e7e2398f7a..5c4fb41060b4 100644

drivers/media/dvb-core/dvbdev.c +2 -1

@@ -396,7 +396,8 @@ static void dvb_register_media_device(struct dvb_device
 *dvbdev, if (!dvbdev->entity || !dvbdev->intf_devnode)
 		return;
 
-	media_create_intf_link(dvbdev->entity, &dvbdev->intf_devnode->intf, 0);
+	media_create_intf_link(dvbdev->entity, &dvbdev->intf_devnode->intf,
+			       MEDIA_LNK_FL_ENABLED);
 
 #endif
 }

drivers/media/dvb-core/dvbdev.c void dvb_create_media_graph(struct dvb_adapter *adap) · line 584

Quoted 1 line

drivers/media/dvb-core/dvbdev.c +0 -0

@@ -583,20 +584,24 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
 	/* Create indirect interface links for FE->tuner, DVR->demux and CA->ca 

*/

Quoted 58 lines

drivers/media/dvb-core/dvbdev.c +9 -5

 media_device_for_each_intf(intf, mdev) {
 		if (intf->type == MEDIA_INTF_T_DVB_CA && ca)
-			media_create_intf_link(ca, intf, 0);
+			media_create_intf_link(ca, intf, MEDIA_LNK_FL_ENABLED);
 
 		if (intf->type == MEDIA_INTF_T_DVB_FE && tuner)
-			media_create_intf_link(tuner, intf, 0);
+			media_create_intf_link(tuner, intf,
+					       MEDIA_LNK_FL_ENABLED);
 
 		if (intf->type == MEDIA_INTF_T_DVB_DVR && demux)
-			media_create_intf_link(demux, intf, 0);
+			media_create_intf_link(demux, intf,
+					       MEDIA_LNK_FL_ENABLED);
 
 		media_device_for_each_entity(entity, mdev) {
 			if (entity->type == MEDIA_ENT_T_DVB_TSOUT) {
 				if (!strcmp(entity->name, DVR_TSOUT))
-					media_create_intf_link(entity, intf, 0);
+					media_create_intf_link(entity, intf,
+							       MEDIA_LNK_FL_ENABLED);
 				if (!strcmp(entity->name, DEMUX_TSOUT))
-					media_create_intf_link(entity, intf, 0);
+					media_create_intf_link(entity, intf,
+							       MEDIA_LNK_FL_ENABLED);
 				break;
 			}
 		}

(quoted diff) +0 -0

b/drivers/media/v4l2-core/v4l2-dev.c index 07123dd569c4..8429da66754a 100644

drivers/media/v4l2-core/v4l2-dev.c +2 -1

@@ -788,7 +788,8 @@ static int video_register_media_controller(struct
 video_device *vdev, int type) struct media_link *link;
 
 		link = media_create_intf_link(&vdev->entity,
-					      &vdev->intf_devnode->intf, 0);
+					      &vdev->intf_devnode->intf,
+					      MEDIA_LNK_FL_ENABLED);
 		if (!link) {
 			media_devnode_remove(vdev->intf_devnode);
 			media_device_unregister_entity(&vdev->entity);

(quoted diff) +0 -0

b/drivers/media/v4l2-core/v4l2-device.c index e788a085ba96..bb58d90fde5e 100644

drivers/media/v4l2-core/v4l2-device.c +1 -1

@@ -256,7 +256,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device
 *v4l2_dev)
 
 			link = media_create_intf_link(&sd->entity,
 						      &vdev->intf_devnode->intf,
-						      0);
+						      MEDIA_LNK_FL_ENABLED);
 			if (!link)
 				goto clean_up;

}