[PATCH RFC v2 09/16] media: use media_graph_obj for link endpoints

quoted

quoted

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 403019035424..f6e2136480f1 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h

@@ -43,6 +43,17 @@ enum media_graph_type {
 	MEDIA_GRAPH_LINK,
 };
 
+/**
+ * enum media_graph_link_dir - direction of a link
+ *
+ * @MEDIA_LINK_DIR_BIDIRECTIONAL	Link is bidirectional
+ * @MEDIA_LINK_DIR_PAD_0_TO_1		Link is unidirectional,
+ *					from port 0 (source) to port 1 (sink)
+ */
+enum media_graph_link_dir {
+	MEDIA_LINK_DIR_BIDIRECTIONAL,
+	MEDIA_LINK_DIR_PORT0_TO_PORT1,
+};

1) the comment and the actual enum are out-of-sync

Ah, yes. I was in doubt about using PAD or PORT here. I ended by using
port at the links, as the endpoints can either be an interface/entity
or a pad. So, I decided to use port.

It's either bi-directional (between interface and entity) or directional
(between two pads), so I think PAD is better here. We don't use the term
port anywhere else in the MC, so I think it is a bit confusing to
introduce a new name here.

quoted

2) why not just make a 'BIRECTIONAL' link flag instead of inventing
   a new enum? Adding yet another field seems overkill to me. Have a
   'BIDIRECTIONAL' flag seems perfectly OK to me (and useful for the
   application as well).

Yeah, we can use flags, instead. I decided to use an enum here just
to make it clearer about the two possible options.

I was actually considering to rename media_link source/sink to
port0/port1, as using "source"/"sink" names on a bidirection link
doesn't make sense. I'm still in doubt about such rename, though,
as it would make harder to inspect the graph traversal routines.

Right. I really wouldn't rename it. As suggested below using an
anonymous union would allow you to create proper names.

Also, I want to force all places that create a link to choose
between either BIRECTIONAL or PORT0_TO_PORT1, as this makes easier
to review if the code is doing the right thing when inspecting it.

By creating two different functions? I think that would be very useful.
E.g. make_pad_link() and make_intf_to_ent_link() or something like
that. That would also hide the link direction. I still prefer a flag,
though :-) That's mostly personal preference, though.

In summary, I would prefer to keep this internally as a separate
 enum, at least for now. We can latter simplify it and use a flag
for that (or maybe two flags?).

quoted

quoted

 
 /* Structs to represent the objects that belong to a media graph */
 

@@ -72,9 +83,9 @@ struct media_pipeline {
 
 struct media_link {
 	struct list_head list;
-	struct media_graph_obj			graph_obj;
-	struct media_pad *source;	/* Source pad */
-	struct media_pad *sink;		/* Sink pad  */
+	struct media_graph_obj		graph_obj;
+	enum media_graph_link_dir	dir;
+	struct media_graph_obj		*source, *sink;

I'm not too keen about all the gobj_to_foo(obj) macros that this requires. It
is rather ugly code.

What about this:

	union {
		struct media_graph_obj *source;
		struct media_pad *source_pad;
		struct media_interface *source_intf;
	};
	union {
		struct media_graph_obj *sink;
		struct media_pad *sink_pad;
		struct media_entity *sink_ent;
	};

Now the code can just use ->source_pad etc.

good idea. Will do that on a version 3. I think that, in this case, the
best is to write a note that the first element at pad/entity/interface
should be the graph_obj.

I would actually call port0_intf and port1_ent on the above structs,
as it makes no sense to call sink/source for interface->entity links.

How about this:

 	union {
 		struct media_graph_obj *port0;
 		struct media_interface *port0_intf;	// perhaps just intf or interface?
 		struct media_pad *source;
 	};
 	union {
 		struct media_graph_obj *port1;
 		struct media_entity *port1_ent;	// perhaps just ent or entity?
 		struct media_pad *sink;
 	};

This has the advantage that the source/sink pads are still called source and
sink and you don't have to rename the existing code.

Regards,

	Hans