Hi Hans,

Thanks for the review.

On Mon, Jun 24, 2013 at 1:21 PM, Hans Verkuil [off-list ref] wrote:

Hi Prabhakar,

On Sat June 22 2013 17:03:03 Prabhakar Lad wrote:

quoted

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

This patch adds video sync properties as part of endpoint
properties and also support to parse them in the parser.

Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
Cc: Hans Verkuil <redacted>

FYI: using my private email when CC-ing me generally works better.
I often skip v4l2-related emails to my work address since I assume those
have either been CC-ed to my private email and/or linux-media.

OK hence forth I'll take care of it.

I wondered why I never saw RFC v1/2, now I know :-)

[Snip]

quoted

   clock mode.
+- video-sync: property indicating to sync the video on a signal in RGB.

Please document what the various syncs actually mean.

OK

quoted

 Example

diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index aa59639..1a54530 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c

@@ -100,6 +100,26 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
      if (!of_property_read_u32(node, "data-shift", &v))
              bus->data_shift = v;

+     if (!of_property_read_u32(node, "video-sync", &v)) {
+             switch (v) {
+             case V4L2_MBUS_VIDEO_SEPARATE_SYNC:
+                     flags |= V4L2_MBUS_VIDEO_SEPARATE_SYNC;
+                     break;
+             case V4L2_MBUS_VIDEO_COMPOSITE_SYNC:
+                     flags |= V4L2_MBUS_VIDEO_COMPOSITE_SYNC;
+                     break;
+             case V4L2_MBUS_VIDEO_SYNC_ON_COMPOSITE:
+                     flags |= V4L2_MBUS_VIDEO_SYNC_ON_COMPOSITE;
+                     break;
+             case V4L2_MBUS_VIDEO_SYNC_ON_GREEN:
+                     flags |= V4L2_MBUS_VIDEO_SYNC_ON_GREEN;
+                     break;
+             case V4L2_MBUS_VIDEO_SYNC_ON_LUMINANCE:
+                     flags |= V4L2_MBUS_VIDEO_SYNC_ON_LUMINANCE;
+                     break;
+             }
+     }
+
      bus->flags = flags;

 }

diff --git a/include/dt-bindings/media/video-interfaces.h b/include/dt-bindings/media/video-interfaces.h
new file mode 100644
index 0000000..1a083dd
--- /dev/null
+++ b/include/dt-bindings/media/video-interfaces.h

@@ -0,0 +1,17 @@
+/*
+ * This header provides constants for video interface.
+ *
+ */
+
+#ifndef _DT_BINDINGS_VIDEO_INTERFACES_H
+#define _DT_BINDINGS_VIDEO_INTERFACES_H
+
+#define V4L2_MBUS_VIDEO_SEPARATE_SYNC                (1 << 2)
+#define V4L2_MBUS_VIDEO_COMPOSITE_SYNC               (1 << 3)
+#define V4L2_MBUS_VIDEO_SYNC_ON_COMPOSITE    (1 << 4)

What on earth is the difference between "COMPOSITE_SYNC" and "SYNC_ON_COMPOSITE"?!

Ahh my bad.

quoted

+#define V4L2_MBUS_VIDEO_SYNC_ON_GREEN                (1 << 5)
+#define V4L2_MBUS_VIDEO_SYNC_ON_LUMINANCE    (1 << 6)
+
+#define V4L2_MBUS_VIDEO_INTERFACES_END               6
+
+#endif

Why would this be here? It isn't Device Tree specific, the same defines can
be used without DT as well.

This is in here because we cannot include header files from other folder in
device tree files.

quoted

diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 83ae07e..a4676dd 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h

@@ -11,6 +11,8 @@
 #ifndef V4L2_MEDIABUS_H
 #define V4L2_MEDIABUS_H

+#include <dt-bindings/media/video-interfaces.h>
+
 #include <linux/v4l2-mediabus.h>

 /* Parallel flags */

@@ -28,18 +30,18 @@
  * V4L2_MBUS_[HV]SYNC* flags should be also used for specifying
  * configuration of hardware that uses [HV]REF signals
  */
-#define V4L2_MBUS_HSYNC_ACTIVE_HIGH          (1 << 2)
-#define V4L2_MBUS_HSYNC_ACTIVE_LOW           (1 << 3)
-#define V4L2_MBUS_VSYNC_ACTIVE_HIGH          (1 << 4)
-#define V4L2_MBUS_VSYNC_ACTIVE_LOW           (1 << 5)
-#define V4L2_MBUS_PCLK_SAMPLE_RISING         (1 << 6)
-#define V4L2_MBUS_PCLK_SAMPLE_FALLING                (1 << 7)
-#define V4L2_MBUS_DATA_ACTIVE_HIGH           (1 << 8)
-#define V4L2_MBUS_DATA_ACTIVE_LOW            (1 << 9)
+#define V4L2_MBUS_HSYNC_ACTIVE_HIGH          (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 1))
+#define V4L2_MBUS_HSYNC_ACTIVE_LOW           (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 2))
+#define V4L2_MBUS_VSYNC_ACTIVE_HIGH          (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 3))
+#define V4L2_MBUS_VSYNC_ACTIVE_LOW           (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 4))
+#define V4L2_MBUS_PCLK_SAMPLE_RISING         (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 5))
+#define V4L2_MBUS_PCLK_SAMPLE_FALLING                (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 6))
+#define V4L2_MBUS_DATA_ACTIVE_HIGH           (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 7))
+#define V4L2_MBUS_DATA_ACTIVE_LOW            (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 8))
 /* FIELD = 0/1 - Field1 (odd)/Field2 (even) */
-#define V4L2_MBUS_FIELD_EVEN_HIGH            (1 << 10)
+#define V4L2_MBUS_FIELD_EVEN_HIGH            (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 9))
 /* FIELD = 1/0 - Field1 (odd)/Field2 (even) */
-#define V4L2_MBUS_FIELD_EVEN_LOW             (1 << 11)
+#define V4L2_MBUS_FIELD_EVEN_LOW             (1 << (V4L2_MBUS_VIDEO_INTERFACES_END + 10))

And that would also do away with the ugly V4L2_MBUS_VIDEO_INTERFACES_END
define. It's just asking for problems when some of the flags are defined
in one header, and some in another header.

I know it was discussed before, but I am uncomfortable with adding all these
different sync types when the only one used in tvp7002 is SYNC_ON_GREEN.

The only sync types I see in practice are Separate Sync (using separate H and V
sync signals), Embedded Sync (using SAV/EAV codes) and in very rare cases
Sync-on-Green. Sync-on-Luminance is I expect really identical to Sync-on-Green,
only instead of using RGB it uses YUV where Y maps to the Green pin.

Laurent, Sylwester  what do you suggest ?

Regards,
--Prabhakar Lad