Hi Sylwester, Oops some how missed this mail, sorry for the late response. On Sun, Jun 30, 2013 at 9:23 PM, Sylwester Nawrocki <sylvester.nawrocki@xxxxxxxxx> wrote: > 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. > > Ok I'll take care of it in the next version. >> --- >> 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. >> + 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 >> +#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. >> +#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 -- 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
