From: Lad, Prabhakar <[email protected]>
This patch adds "sync-on-green" property as part of
endpoint properties and also support to parse them in the parser.
Signed-off-by: Lad, Prabhakar <[email protected]>
Cc: Hans Verkuil <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Guennadi Liakhovetski <[email protected]>
Cc: Sylwester Nawrocki <[email protected]>
Cc: Sakari Ailus <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Rob Landley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Kyungmin Park <[email protected]>
---
Changes for v2:
1: Dropped "field-active" property as it same as existing
"field-even-active" property.
.../devicetree/bindings/media/video-interfaces.txt | 2 ++
drivers/media/v4l2-core/v4l2-of.c | 3 +++
include/media/v4l2-mediabus.h | 1 +
3 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index e022d2d..f91821f 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -101,6 +101,8 @@ Optional endpoint properties
array contains only one entry.
- clock-noncontinuous: a boolean property to allow MIPI CSI-2 non-continuous
clock mode.
+-sync-on-green: a boolean property indicating to sync with the green signal in
+ RGB.
Example
diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index aa59639..b51d61f 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -100,6 +100,9 @@ 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_get_property(node, "sync-on-green", &v))
+ flags |= V4L2_MBUS_SYNC_ON_GREEN;
+
bus->flags = flags;
}
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 83ae07e..cf2c66f9 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -40,6 +40,7 @@
#define V4L2_MBUS_FIELD_EVEN_HIGH (1 << 10)
/* FIELD = 1/0 - Field1 (odd)/Field2 (even) */
#define V4L2_MBUS_FIELD_EVEN_LOW (1 << 11)
+#define V4L2_MBUS_SYNC_ON_GREEN (1 << 12)
/* Serial flags */
/* How many lanes the client can use */
--
1.7.4.1
Hi Laurent/Sylwester,
On Thu, May 16, 2013 at 6:48 PM, Lad Prabhakar
<[email protected]> wrote:
> From: Lad, Prabhakar <[email protected]>
>
> This patch adds "sync-on-green" property as part of
> endpoint properties and also support to parse them in the parser.
>
If you are ok with the patch can I send non RFC version of it ?
Regards,
--Prabhakar Lad
Prabhakar,
On 05/16/2013 03:18 PM, Lad Prabhakar wrote:
> From: Lad, Prabhakar <[email protected]>
>
> This patch adds "sync-on-green" property as part of
> endpoint properties and also support to parse them in the parser.
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -101,6 +101,8 @@ Optional endpoint properties
> array contains only one entry.
> - clock-noncontinuous: a boolean property to allow MIPI CSI-2 non-continuous
> clock mode.
> +-sync-on-green: a boolean property indicating to sync with the green signal in
> + RGB.
Are you sure this is what you need for the TVP7002 chip ?
I think we should differentiate between analog and digital signals and
the related
device's configuration. AFAIU for the analog part there can be various video
sychronisation methods, i.e. ways in which the synchronisation signals are
transmitted along side the video component (RGB or luma/chroma) signals.
According
to [1] (presumably not most reliable source of information) there are
following
methods of transmitting sync signals:
- Separate sync
- Composite sync
- Sync-on-green (SOG)
- Sync-on-luminance
- Sync-on-composite
And all these seem to refer to analog video signal.
From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's
datasheet
([2], p. 52) and your initial TVP7002 patches it looks like what you
want is to
specify polarity of the SOGOUT signal, so the processor that receives
this signal
can properly interpret it, is it correct ?
If so then wouldn't it be more appropriate to define e.g. 'sog-active'
property
and media bus flags:
V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW
V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH
?
And for synchronisation method on the analog part we could perhaps define
'component-sync' or similar property that would enumerate all possible
synchronisation methods. We might as well use separate boolean properties,
but I'm a bit concerned about the increasing number of properties that need
to be parsed for each parallel video bus "endpoint".
Anyway I'm not sure if we would already have use cases for the
'component-sync'
property.
[1] http://en.wikipedia.org/wiki/Component_video_sync
[2]
http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=tvp7002&fileType=pdf
> Example
> diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
> index aa59639..b51d61f 100644
> --- a/drivers/media/v4l2-core/v4l2-of.c
> +++ b/drivers/media/v4l2-core/v4l2-of.c
> @@ -100,6 +100,9 @@ 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_get_property(node, "sync-on-green",&v))
> + flags |= V4L2_MBUS_SYNC_ON_GREEN;
> +
> bus->flags = flags;
>
> }
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 83ae07e..cf2c66f9 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -40,6 +40,7 @@
> #define V4L2_MBUS_FIELD_EVEN_HIGH (1<< 10)
> /* FIELD = 1/0 - Field1 (odd)/Field2 (even) */
> #define V4L2_MBUS_FIELD_EVEN_LOW (1<< 11)
> +#define V4L2_MBUS_SYNC_ON_GREEN (1<< 12)
--
Regards,
Sylwester
Hi Sylwester,
On Fri, May 24, 2013 at 4:41 PM, Sylwester Nawrocki
<[email protected]> wrote:
> Prabhakar,
>
>
> On 05/16/2013 03:18 PM, Lad Prabhakar wrote:
>>
>> From: Lad, Prabhakar <[email protected]>
>>
>> This patch adds "sync-on-green" property as part of
>> endpoint properties and also support to parse them in the parser.
>
>
>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> @@ -101,6 +101,8 @@ Optional endpoint properties
>> array contains only one entry.
>> - clock-noncontinuous: a boolean property to allow MIPI CSI-2
>> non-continuous
>> clock mode.
>> +-sync-on-green: a boolean property indicating to sync with the green
>> signal in
>> + RGB.
>
>
> Are you sure this is what you need for the TVP7002 chip ?
>
Yes
> I think we should differentiate between analog and digital signals and the
> related
> device's configuration. AFAIU for the analog part there can be various video
> sychronisation methods, i.e. ways in which the synchronisation signals are
> transmitted along side the video component (RGB or luma/chroma) signals.
> According
> to [1] (presumably not most reliable source of information) there are
> following
> methods of transmitting sync signals:
>
> - Separate sync
> - Composite sync
> - Sync-on-green (SOG)
> - Sync-on-luminance
> - Sync-on-composite
>
> And all these seem to refer to analog video signal.
>
I was about to add all these but as per Laurent mentioned we can add this
whenever there is a need of it.
> From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's
> datasheet
> ([2], p. 52) and your initial TVP7002 patches it looks like what you want is
> to
> specify polarity of the SOGOUT signal, so the processor that receives this
> signal
> can properly interpret it, is it correct ?
>
Yes
> If so then wouldn't it be more appropriate to define e.g. 'sog-active'
> property
> and media bus flags:
> V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW
> V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH
> ?
>
Agreed I'll add these flags.
> And for synchronisation method on the analog part we could perhaps define
> 'component-sync' or similar property that would enumerate all possible
> synchronisation methods. We might as well use separate boolean properties,
> but I'm a bit concerned about the increasing number of properties that need
> to be parsed for each parallel video bus "endpoint".
>
I am not clear on it can please elaborate more on this.
Regards,
--Prabhakar Lad
Hi,
On 05/25/2013 11:17 AM, Prabhakar Lad wrote:
>> From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's
>> > datasheet
>> > ([2], p. 52) and your initial TVP7002 patches it looks like what you want is
>> > to
>> > specify polarity of the SOGOUT signal, so the processor that receives this
>> > signal
>> > can properly interpret it, is it correct ?
>> >
> Yes
>> > If so then wouldn't it be more appropriate to define e.g. 'sog-active'
>> > property
>> > and media bus flags:
>> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW
>> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH
>> > ?
>> >
> Agreed I'll add these flags.
>
>> > And for synchronisation method on the analog part we could perhaps define
>> > 'component-sync' or similar property that would enumerate all possible
>> > synchronisation methods. We might as well use separate boolean properties,
>> > but I'm a bit concerned about the increasing number of properties that need
>> > to be parsed for each parallel video bus "endpoint".
>> >
> I am not clear on it can please elaborate more on this.
I thought about two possible options:
1. single property 'component-sync' or 'video-sync' that would have values:
#define VIDEO_SEPARATE_SYNC 0x01
#define VIDEO_COMPOSITE_SYNC 0x02
#define VIDEO_SYNC_ON_COMPOSITE 0x04
#define VIDEO_SYNC_ON_GREEN 0x08
#define VIDEO_SYNC_ON_LUMINANCE 0x10
And we could put these definitions into a separate header, e.g.
<dt-bindings/video-interfaces.h>
Then in a device tree source file one could have, e.g.
video-sync = <VIDEO_SYNC_ON_GREEN>;
2. Separate boolean property for each video sync type, e.g.
"video-composite-sync"
"video-sync-on-composite"
"video-sync-on-green"
"video-sync-on-luminance"
Separate sync, with separate VSYNC, HSYNC lines, would be the default, when
none of the above is specified and 'vsync-active', 'hsync-active' properties
are present.
However, I suppose the better would be to deduce the video synchronisation
method from the sync signal polarity flags. Then, for instance, when an
endpoint node contains "composite-sync-active" property the parser would
determine the "composite sync" synchronisation type is used.
Thus it might make sense to have only following integer properties (added
as needed):
composite-sync-active
sync-on-green-active
sync-on-comp-active
sync-on-luma-active
This would allow to specify polarity of each signal and at the same time
the parsing code could derive synchronisation type. A new field could be
added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled
within v4l2_of_parse_endpoint().
What do you think ?
Thanks,
Sylwester
Hi Sylwester,
On Sat, May 25, 2013 at 7:41 PM, Sylwester Nawrocki
<[email protected]> wrote:
> Hi,
>
>
> On 05/25/2013 11:17 AM, Prabhakar Lad wrote:
>>>
>>> From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's
>>> > datasheet
>>> > ([2], p. 52) and your initial TVP7002 patches it looks like what you
>>> > want is
>>> > to
>>> > specify polarity of the SOGOUT signal, so the processor that receives
>>> > this
>>> > signal
>>> > can properly interpret it, is it correct ?
>>> >
>>
>> Yes
>>>
>>> > If so then wouldn't it be more appropriate to define e.g. 'sog-active'
>>> > property
>>> > and media bus flags:
>>> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW
>>> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH
>>> > ?
>>> >
>>
>> Agreed I'll add these flags.
>>
>>> > And for synchronisation method on the analog part we could perhaps
>>> > define
>>> > 'component-sync' or similar property that would enumerate all possible
>>> > synchronisation methods. We might as well use separate boolean
>>> > properties,
>>> > but I'm a bit concerned about the increasing number of properties that
>>> > need
>>> > to be parsed for each parallel video bus "endpoint".
>>> >
>>
>> I am not clear on it can please elaborate more on this.
>
>
> I thought about two possible options:
>
> 1. single property 'component-sync' or 'video-sync' that would have values:
>
> #define VIDEO_SEPARATE_SYNC 0x01
> #define VIDEO_COMPOSITE_SYNC 0x02
> #define VIDEO_SYNC_ON_COMPOSITE 0x04
> #define VIDEO_SYNC_ON_GREEN 0x08
> #define VIDEO_SYNC_ON_LUMINANCE 0x10
>
> And we could put these definitions into a separate header, e.g.
> <dt-bindings/video-interfaces.h>
>
> Then in a device tree source file one could have, e.g.
>
> video-sync = <VIDEO_SYNC_ON_GREEN>;
>
>
> 2. Separate boolean property for each video sync type, e.g.
>
> "video-composite-sync"
> "video-sync-on-composite"
> "video-sync-on-green"
> "video-sync-on-luminance"
>
> Separate sync, with separate VSYNC, HSYNC lines, would be the default, when
> none of the above is specified and 'vsync-active', 'hsync-active' properties
> are present.
>
> However, I suppose the better would be to deduce the video synchronisation
> method from the sync signal polarity flags. Then, for instance, when an
> endpoint node contains "composite-sync-active" property the parser would
> determine the "composite sync" synchronisation type is used.
>
> Thus it might make sense to have only following integer properties (added
> as needed):
>
> composite-sync-active
> sync-on-green-active
> sync-on-comp-active
> sync-on-luma-active
>
> This would allow to specify polarity of each signal and at the same time
> the parsing code could derive synchronisation type. A new field could be
> added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled
> within v4l2_of_parse_endpoint().
>
I am OK with this option. and I hope you meant "struct
v4l2_of_bus_parallel" instead
of " struct v4l2_of_parallel_bus" and to fill sync_type within
v4l2_of_parse_parallel_bus()
and not in v4l2_of_parse_endpoint().
Regards,
--Prabhakar Lad
On 05/25/2013 04:26 PM, Prabhakar Lad wrote:
>> Thus it might make sense to have only following integer properties (added
>> > as needed):
>> >
>> > composite-sync-active
>> > sync-on-green-active
>> > sync-on-comp-active
>> > sync-on-luma-active
>> >
>> > This would allow to specify polarity of each signal and at the same time
>> > the parsing code could derive synchronisation type. A new field could be
>> > added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled
>> > within v4l2_of_parse_endpoint().
>> >
> I am OK with this option. and I hope you meant "struct
> v4l2_of_bus_parallel" instead
> of " struct v4l2_of_parallel_bus" and to fill sync_type within
> v4l2_of_parse_parallel_bus()
> and not in v4l2_of_parse_endpoint().
Yes, that's what I meant, sorry for this confusion.
Regards,
Sylwester
Hi Sylwester,
On Saturday 25 May 2013 16:11:52 Sylwester Nawrocki wrote:
> On 05/25/2013 11:17 AM, Prabhakar Lad wrote:
> >> > From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's
> >> > datasheet ([2], p. 52) and your initial TVP7002 patches it looks like
> >> > what you want is to specify polarity of the SOGOUT signal, so the
> >> > processor that receives this signal can properly interpret it, is it
> >> > correct ?
> >
> > Yes
> >
> >> > If so then wouldn't it be more appropriate to define e.g. 'sog-active'
> >> > property and media bus flags:
> >> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW
> >> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH
> >> >
> >> > ?
> >
> > Agreed I'll add these flags.
> >
> >> > And for synchronisation method on the analog part we could perhaps
> >> > define 'component-sync' or similar property that would enumerate all
> >> > possible synchronisation methods. We might as well use separate
> >> > boolean properties, but I'm a bit concerned about the increasing
> >> > number of properties that need to be parsed for each parallel video
> >> > bus "endpoint".
> >
> > I am not clear on it can please elaborate more on this.
>
> I thought about two possible options:
>
> 1. single property 'component-sync' or 'video-sync' that would have values:
>
> #define VIDEO_SEPARATE_SYNC 0x01
> #define VIDEO_COMPOSITE_SYNC 0x02
> #define VIDEO_SYNC_ON_COMPOSITE 0x04
> #define VIDEO_SYNC_ON_GREEN 0x08
> #define VIDEO_SYNC_ON_LUMINANCE 0x10
>
> And we could put these definitions into a separate header, e.g.
> <dt-bindings/video-interfaces.h>
>
> Then in a device tree source file one could have, e.g.
>
> video-sync = <VIDEO_SYNC_ON_GREEN>;
>
>
> 2. Separate boolean property for each video sync type, e.g.
>
> "video-composite-sync"
> "video-sync-on-composite"
> "video-sync-on-green"
> "video-sync-on-luminance"
>
> Separate sync, with separate VSYNC, HSYNC lines, would be the default, when
> none of the above is specified and 'vsync-active', 'hsync-active' properties
> are present.
I prefer 1. over 2.
> However, I suppose the better would be to deduce the video synchronisation
> method from the sync signal polarity flags. Then, for instance, when an
> endpoint node contains "composite-sync-active" property the parser would
> determine the "composite sync" synchronisation type is used.
>
> Thus it might make sense to have only following integer properties (added
> as needed):
>
> composite-sync-active
> sync-on-green-active
> sync-on-comp-active
> sync-on-luma-active
>
> This would allow to specify polarity of each signal and at the same time
> the parsing code could derive synchronisation type. A new field could be
> added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled
> within v4l2_of_parse_endpoint().
>
> What do you think ?
My gut feeling is that we should have separate properties for the video sync
type and the synchronization signals polarities. We could have a chip that
supports sync-on-green on the analog (input) side and outputs separate hsync
and vsync signals only on the digital (output) side. There would be no sync-
on-green polarity in that case.
--
Regards,
Laurent Pinchart
On 05/30/2013 05:21 AM, Laurent Pinchart wrote:
> Hi Sylwester,
>
> On Saturday 25 May 2013 16:11:52 Sylwester Nawrocki wrote:
>> On 05/25/2013 11:17 AM, Prabhakar Lad wrote:
[...]
>>>>> And for synchronisation method on the analog part we could perhaps
>>>>> define 'component-sync' or similar property that would enumerate all
>>>>> possible synchronisation methods. We might as well use separate
>>>>> boolean properties, but I'm a bit concerned about the increasing
>>>>> number of properties that need to be parsed for each parallel video
>>>>> bus "endpoint".
>>>
>>> I am not clear on it can please elaborate more on this.
>>
>> I thought about two possible options:
>>
>> 1. single property 'component-sync' or 'video-sync' that would have values:
>>
>> #define VIDEO_SEPARATE_SYNC 0x01
>> #define VIDEO_COMPOSITE_SYNC 0x02
>> #define VIDEO_SYNC_ON_COMPOSITE 0x04
>> #define VIDEO_SYNC_ON_GREEN 0x08
>> #define VIDEO_SYNC_ON_LUMINANCE 0x10
>>
>> And we could put these definitions into a separate header, e.g.
>> <dt-bindings/video-interfaces.h>
>>
>> Then in a device tree source file one could have, e.g.
>>
>> video-sync = <VIDEO_SYNC_ON_GREEN>;
>>
>>
>> 2. Separate boolean property for each video sync type, e.g.
>>
>> "video-composite-sync"
>> "video-sync-on-composite"
>> "video-sync-on-green"
>> "video-sync-on-luminance"
>>
>> Separate sync, with separate VSYNC, HSYNC lines, would be the default, when
>> none of the above is specified and 'vsync-active', 'hsync-active' properties
>> are present.
>
> I prefer 1. over 2.
>
>> However, I suppose the better would be to deduce the video synchronisation
>> method from the sync signal polarity flags. Then, for instance, when an
>> endpoint node contains "composite-sync-active" property the parser would
>> determine the "composite sync" synchronisation type is used.
>>
>> Thus it might make sense to have only following integer properties (added
>> as needed):
>>
>> composite-sync-active
>> sync-on-green-active
>> sync-on-comp-active
>> sync-on-luma-active
>>
>> This would allow to specify polarity of each signal and at the same time
>> the parsing code could derive synchronisation type. A new field could be
>> added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled
>> within v4l2_of_parse_endpoint().
>>
>> What do you think ?
>
> My gut feeling is that we should have separate properties for the video sync
> type and the synchronization signals polarities. We could have a chip that
> supports sync-on-green on the analog (input) side and outputs separate hsync
> and vsync signals only on the digital (output) side. There would be no sync-
> on-green polarity in that case.
Yes, agreed. I've had some doubts that using single DT property for defining
really 2 distinct H/W properties like this might not be flexible enough.
The option 1. seems most correct then.
Regards,
Sylwester