Re: [PATCH RFC v3] media: OF: add video sync endpoint property

From: Sylwester Nawrocki
Date: Sun Jun 30 2013 - 11:54:29 EST

Hi,

On 06/22/2013 05:03 PM, Prabhakar Lad wrote:
From: "Lad, Prabhakar"<prabhakar.csengg@xxxxxxxxx>

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@xxxxxxxxx>
Cc: Hans Verkuil<hans.verkuil@xxxxxxxxx>
Cc: Laurent Pinchart<laurent.pinchart@xxxxxxxxxxxxxxxx>
Cc: Mauro Carvalho Chehab<mchehab@xxxxxxxxxx>
Cc: Guennadi Liakhovetski<g.liakhovetski@xxxxxx>
Cc: Sylwester Nawrocki<s.nawrocki@xxxxxxxxxxx>
Cc: Sakari Ailus<sakari.ailus@xxxxxx>
Cc: Grant Likely<grant.likely@xxxxxxxxxxxx>
Cc: Rob Herring<rob.herring@xxxxxxxxxxx>
Cc: Rob Landley<rob@xxxxxxxxxxx>
Cc: devicetree-discuss@xxxxxxxxxxxxxxxx
Cc: linux-doc@xxxxxxxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx
Cc: davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx
Cc: Kyungmin Park<kyungmin.park@xxxxxxxxxxx>

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.

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

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

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

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

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/