[RFC,v2] media: OF: add sync-on-green endpoint property

Commit Message

Lad, Prabhakar May 16, 2013, 1:18 p.m. UTC

From: Lad, Prabhakar <[e-mail]>

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 <[e-mail]>
Cc: Hans Verkuil <[e-mail]>
Cc: Laurent Pinchart <[e-mail]>
Cc: Mauro Carvalho Chehab <[e-mail]>
Cc: Guennadi Liakhovetski <[e-mail]>
Cc: Sylwester Nawrocki <[e-mail]>
Cc: Sakari Ailus <[e-mail]>
Cc: Grant Likely <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Rob Landley <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: Kyungmin Park <[e-mail]>
---
 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(-)

Comments

Lad, Prabhakar May 22, 2013, 5:50 a.m. UTC | #1

Hi Laurent/Sylwester,

On Thu, May 16, 2013 at 6:48 PM, Lad Prabhakar
<[e-mail]> wrote:
> From: Lad, Prabhakar <[e-mail]>
>
> 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

Sylwester Nawrocki May 24, 2013, 11:11 a.m. UTC | #2

Prabhakar,

On 05/16/2013 03:18 PM, Lad Prabhakar wrote:
> From: Lad, Prabhakar <[e-mail]>
>
> 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

Lad, Prabhakar May 25, 2013, 9:17 a.m. UTC | #3

Hi Sylwester,

On Fri, May 24, 2013 at 4:41 PM, Sylwester Nawrocki
<[e-mail]> wrote:
> Prabhakar,
>
>
> On 05/16/2013 03:18 PM, Lad Prabhakar wrote:
>>
>> From: Lad, Prabhakar <[e-mail]>
>>
>> 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

Sylwester Nawrocki May 25, 2013, 2:11 p.m. UTC | #4

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

Lad, Prabhakar May 25, 2013, 2:26 p.m. UTC | #5

Hi Sylwester,

On Sat, May 25, 2013 at 7:41 PM, Sylwester Nawrocki
<[e-mail]> 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

Sylwester Nawrocki May 25, 2013, 6:02 p.m. UTC | #6

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

Laurent Pinchart May 30, 2013, 3:21 a.m. UTC | #7

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.

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics May 31, 2013, 10:31 a.m. UTC | #8

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

Patch

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