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

From: Hans Verkuil
Date: Mon Jun 24 2013 - 03:52:07 EST

Next message: Stanislav Kinsbursky: "[PATCH v3 0/4] SUNRPC: PipeFS races fixes"
Previous message: Kirill A. Shutemov: "[PATCH] perf: fix build with perl 5.18"
Next in thread: Prabhakar Lad: "Re: [PATCH RFC v3] media: OF: add video sync endpoint property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Stanislav Kinsbursky: "[PATCH v3 0/4] SUNRPC: PipeFS races fixes"
Previous message: Kirill A. Shutemov: "[PATCH] perf: fix build with perl 5.18"
Next in thread: Prabhakar Lad: "Re: [PATCH RFC v3] media: OF: add video sync endpoint property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]