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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux