Hi Prabhakar, On Sat June 22 2013 17:03:03 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> 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. I wondered why I never saw RFC v1/2, now I know :-) > 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> > --- > 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. Please document what the various syncs actually mean. > > > 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"?! > +#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. > 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. Anyway, this needs more work I'm afraid. Regards, Hans > > /* Serial flags */ > /* How many lanes the client can use */ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
