Hi Sylwester,

Oops some how missed this mail, sorry for the late response.

On Sun, Jun 30, 2013 at 9:23 PM, Sylwester Nawrocki
[off-list ref] wrote:

Hi,


On 06/22/2013 05:03 PM, 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>
Cc: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab<redacted>
Cc: Guennadi Liakhovetski<redacted>
Cc: Sylwester Nawrocki<s.nawrocki@samsung.com>
Cc: Sakari Ailus<sakari.ailus@iki.fi>
Cc: Grant Likely<redacted>
Cc: Rob Herring<redacted>
Cc: Rob Landley<redacted>
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: davinci-linux-open-source@linux.davincidsp.com
Cc: Kyungmin Park<kyungmin.park@samsung.com>

Do you really need such a long Cc list here ? I think it would be better
to just add relevant e-mail addresses when sending the patch, otherwise
when this patch is applied in this form all those addresses are going to
be spammed with the patch management notifications, which might not be
what some ones are really interested in.

Ok I'll take care of it in the next version.

quoted

---
  This patch has 10 warnings for line over 80 characters
  for which I think can be ignored.

  RFC v2 https://patchwork.kernel.org/patch/2578091/
  RFC V1 https://patchwork.kernel.org/patch/2572341/
  Changes for v3:
  1: Fixed review comments pointed by Laurent and Sylwester.

  .../devicetree/bindings/media/video-interfaces.txt |    1 +
  drivers/media/v4l2-core/v4l2-of.c                  |   20
++++++++++++++++++
  include/dt-bindings/media/video-interfaces.h       |   17
+++++++++++++++
  include/media/v4l2-mediabus.h                      |   22
+++++++++++---------
  4 files changed, 50 insertions(+), 10 deletions(-)
  create mode 100644 include/dt-bindings/media/video-interfaces.h

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt

b/Documentation/devicetree/bindings/media/video-interfaces.txt
index e022d2d..2081278 100644

--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt

@@ -101,6 +101,7 @@ Optional endpoint properties
    array contains only one entry.
  - clock-noncontinuous: a boolean property to allow MIPI CSI-2

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


  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;

I'm not convinced all those video sync types is something that really
belongs
to the flags field. In my understanding this field is supposed to hold only
the _signal polarity_ information.

Ok, so there should be a function say v4l2_of_parse_signal_polarity()
to get the polarity alone then.

quoted

+                       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)

You should never be putting anything Linux specific in those dt-bindings
headers. It just happens now include/dt-bindings is a part of the Linux
kernel, but it could well be in some separate repository, or could be
a part of the DT bindings specification, which is only specific to the
hardware and doesn't depend on Linux OS at all. Thus V4L2_MBUS_ prefix
shouldn't be used here.

Ok

quoted

+#define V4L2_MBUS_VIDEO_COMPOSITE_SYNC         (1<<  3)
+#define V4L2_MBUS_VIDEO_SYNC_ON_COMPOSITE      (1<<  4)
+#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

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))

No, please don't do that. We shouldn't combine the DT and Linux kernel
definitions like this.

Ok.

quoted

+#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))

Please see my review of your 'media: i2c: tvp7002: add OF support" patch.
AFAICS it should be sufficient to add only V4L2_MBUS_SOG_ACTIVE_{LOW,HIGH}
flags and 'sync-on-green-active' DT property.

Ok.

Regards,
--Prabhakar Lad