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

From: Prabhakar Lad
Date: Mon Jun 24 2013 - 12:09:55 EST

Hi Hans,

Thanks for the review.

On Mon, Jun 24, 2013 at 1:21 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> 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.
>
OK hence forth I'll take care of it.

> I wondered why I never saw RFC v1/2, now I know :-)
[Snip]
>> clock mode.
>> +- video-sync: property indicating to sync the video on a signal in RGB.
>
> Please document what the various syncs actually mean.
>
OK

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

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

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