2013-05-15 12:52:56

by Lad, Prabhakar

[permalink] [raw]
Subject: [PATCH RFC] media: OF: add field-active and sync-on-green endpoint properties

From: Lad, Prabhakar <[email protected]>

This patch adds "field-active" and "sync-on-green" 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]>
---
.../devicetree/bindings/media/video-interfaces.txt | 4 ++++
drivers/media/v4l2-core/v4l2-of.c | 6 ++++++
include/media/v4l2-mediabus.h | 2 ++
3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index e022d2d..6bf87d0 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -101,6 +101,10 @@ Optional endpoint properties
array contains only one entry.
- clock-noncontinuous: a boolean property to allow MIPI CSI-2 non-continuous
clock mode.
+-field-active: a boolean property indicating active high filed ID output
+ polarity is inverted.
+-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..1d59455 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -100,6 +100,12 @@ 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, "field-active", &v))
+ flags |= V4L2_MBUS_FIELD_ACTIVE;
+
+ if (of_get_property(node, "sync-on-green", &v))
+ flags |= V4L2_MBUS_SOG;
+
bus->flags = flags;

}
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 83ae07e..b95553d 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -40,6 +40,8 @@
#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_FIELD_ACTIVE (1 << 12)
+#define V4L2_MBUS_SOG (1 << 13)

/* Serial flags */
/* How many lanes the client can use */
--
1.7.4.1


2013-05-15 13:24:07

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH RFC] media: OF: add field-active and sync-on-green endpoint properties

Hi Prabhakar,

Thank you for the patch.

On Wednesday 15 May 2013 18:22:29 Lad Prabhakar wrote:
> From: Lad, Prabhakar <[email protected]>
>
> This patch adds "field-active" and "sync-on-green" 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]>
> ---
> .../devicetree/bindings/media/video-interfaces.txt | 4 ++++
> drivers/media/v4l2-core/v4l2-of.c | 6 ++++++
> include/media/v4l2-mediabus.h | 2 ++
> 3 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
> b/Documentation/devicetree/bindings/media/video-interfaces.txt index
> e022d2d..6bf87d0 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -101,6 +101,10 @@ Optional endpoint properties
> array contains only one entry.
> - clock-noncontinuous: a boolean property to allow MIPI CSI-2
> non-continuous clock mode.
> +-field-active: a boolean property indicating active high filed ID output
> + polarity is inverted.

Looks like we already have field-even-active property to describe the level of
the field signal. Could you please check whether it fulfills your use cases ?
Sorry for not pointing you to it earlier.

> +-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..1d59455 100644
> --- a/drivers/media/v4l2-core/v4l2-of.c
> +++ b/drivers/media/v4l2-core/v4l2-of.c
> @@ -100,6 +100,12 @@ 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, "field-active", &v))
> + flags |= V4L2_MBUS_FIELD_ACTIVE;
> +
> + if (of_get_property(node, "sync-on-green", &v))
> + flags |= V4L2_MBUS_SOG;
> +
> bus->flags = flags;
>
> }
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 83ae07e..b95553d 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -40,6 +40,8 @@
> #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_FIELD_ACTIVE (1 << 12)
> +#define V4L2_MBUS_SOG (1 << 13)
>
> /* Serial flags */
> /* How many lanes the client can use */
--
Regards,

Laurent Pinchart

2013-05-16 04:53:59

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH RFC] media: OF: add field-active and sync-on-green endpoint properties

Hi Laurent,

On Wed, May 15, 2013 at 6:54 PM, Laurent Pinchart
<[email protected]> wrote:
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Wednesday 15 May 2013 18:22:29 Lad Prabhakar wrote:
>> From: Lad, Prabhakar <[email protected]>
>>
>> This patch adds "field-active" and "sync-on-green" 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]>
>> ---
>> .../devicetree/bindings/media/video-interfaces.txt | 4 ++++
>> drivers/media/v4l2-core/v4l2-of.c | 6 ++++++
>> include/media/v4l2-mediabus.h | 2 ++
>> 3 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
>> b/Documentation/devicetree/bindings/media/video-interfaces.txt index
>> e022d2d..6bf87d0 100644
>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> @@ -101,6 +101,10 @@ Optional endpoint properties
>> array contains only one entry.
>> - clock-noncontinuous: a boolean property to allow MIPI CSI-2
>> non-continuous clock mode.
>> +-field-active: a boolean property indicating active high filed ID output
>> + polarity is inverted.
>
> Looks like we already have field-even-active property to describe the level of
> the field signal. Could you please check whether it fulfills your use cases ?
> Sorry for not pointing you to it earlier.
>
I had looked at it earlier it only means "field signal level during the even
field data transmission" it only speaks of even filed. Ideally the field ID
output is set to logic 1 for odd field and set to 0 for even field, what I
want is to invert the FID out polarity when "field-active" property is set.

May be we rename "field-active" to "fid-pol" ?

Regards,
--Prabhakar Lad

2013-05-16 10:43:11

by Sylwester Nawrocki

[permalink] [raw]
Subject: Re: [PATCH RFC] media: OF: add field-active and sync-on-green endpoint properties

Hi,

On 05/16/2013 06:53 AM, Prabhakar Lad wrote:
>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
>>> >> b/Documentation/devicetree/bindings/media/video-interfaces.txt index
>>> >> e022d2d..6bf87d0 100644
>>> >> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
>>> >> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
>>> >> @@ -101,6 +101,10 @@ Optional endpoint properties
>>> >> array contains only one entry.
>>> >> - clock-noncontinuous: a boolean property to allow MIPI CSI-2
>>> >> non-continuous clock mode.
>>> >> +-field-active: a boolean property indicating active high filed ID output
>>> >> + polarity is inverted.
>> >
>> > Looks like we already have field-even-active property to describe the level of
>> > the field signal. Could you please check whether it fulfills your use cases ?
>> > Sorry for not pointing you to it earlier.
>> >
> I had looked at it earlier it only means "field signal level during the even
> field data transmission" it only speaks of even filed. Ideally the field ID
> output is set to logic 1 for odd field and set to 0 for even field, what I
> want is to invert the FID out polarity when "field-active" property is set.
>
> May be we rename "field-active" to "fid-pol" ?

I guess we failed to clearly describe the 'field-even-active' property then.
It seems to be exactly what you need.

It is not enough to say e.g. field-active = <1>;, because it would not have
been clear which field it refers to, odd or even ? Unlike VSYNC, HSYNC both
levels of the FIELD signal are "active", there is no "idle" state for FIELD.

So field-even-active = <1>; means the FIELD signal at logic high level
indicates EVEN field _and_ this implies FIELD = 0 indicates ODD field, i.e.

FIELD = 0 => odd field
FIELD = 1 => even field

For field-even-active = <0>; it is the other way around:

FIELD = 0 => even field
FIELD = 1 => odd field

It looks like only "sync-on-green" property is missing. BTW, is it really
commonly used ? What drivers would need it ?
I'm not against making it a common property, it's just first time I see it.

Thanks,
Sylwester

2013-05-16 10:45:55

by Sylwester Nawrocki

[permalink] [raw]
Subject: Re: [PATCH RFC] media: OF: add field-active and sync-on-green endpoint properties

On 05/15/2013 02:52 PM, Lad Prabhakar wrote:
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index e022d2d..6bf87d0 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -101,6 +101,10 @@ Optional endpoint properties
> array contains only one entry.
> - clock-noncontinuous: a boolean property to allow MIPI CSI-2 non-continuous
> clock mode.
> +-field-active: a boolean property indicating active high filed ID output
> + polarity is inverted.

You can drop this property and use the existing 'field-even-active' property
instead.

> +-sync-on-green: a boolean property indicating to sync with the green signal in
> + RGB.

> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 83ae07e..b95553d 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -40,6 +40,8 @@
> #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_FIELD_ACTIVE (1<< 12)
> +#define V4L2_MBUS_SOG (1<< 13)

How about V4L2_MBUS_SYNC_ON_GREEN ?

Thanks,
Sylwester

2013-05-16 10:52:45

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH RFC] media: OF: add field-active and sync-on-green endpoint properties

Hi Sylwester,

On Thu, May 16, 2013 at 4:13 PM, Sylwester Nawrocki
<[email protected]> wrote:
> Hi,
>
>
> On 05/16/2013 06:53 AM, Prabhakar Lad wrote:
>>>>
[Snip]
>> May be we rename "field-active" to "fid-pol" ?
>
>
> I guess we failed to clearly describe the 'field-even-active' property then.
> It seems to be exactly what you need.
>
> It is not enough to say e.g. field-active = <1>;, because it would not have
> been clear which field it refers to, odd or even ? Unlike VSYNC, HSYNC both
> levels of the FIELD signal are "active", there is no "idle" state for FIELD.
>
> So field-even-active = <1>; means the FIELD signal at logic high level
> indicates EVEN field _and_ this implies FIELD = 0 indicates ODD field, i.e.
>
> FIELD = 0 => odd field
> FIELD = 1 => even field
>
> For field-even-active = <0>; it is the other way around:
>
> FIELD = 0 => even field
> FIELD = 1 => odd field
>
Thanks that makes it clear :)

> It looks like only "sync-on-green" property is missing. BTW, is it really
> commonly used ? What drivers would need it ?
> I'm not against making it a common property, it's just first time I see it.
>
I have comes across a decoder tvp7002 which uses it, may be Laurent/Hans/Sakari
may point much more devices.

Regards,
--Prabhakar Lad

2013-05-16 10:54:43

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH RFC] media: OF: add field-active and sync-on-green endpoint properties

Hi Sylwester,

Thanks for the review.

On Thu, May 16, 2013 at 4:15 PM, Sylwester Nawrocki
<[email protected]> wrote:
> On 05/15/2013 02:52 PM, Lad Prabhakar wrote:
>>
>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
>> b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> index e022d2d..6bf87d0 100644
>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> @@ -101,6 +101,10 @@ Optional endpoint properties
>> array contains only one entry.
>> - clock-noncontinuous: a boolean property to allow MIPI CSI-2
>> non-continuous
>> clock mode.
>> +-field-active: a boolean property indicating active high filed ID output
>> + polarity is inverted.
>
>
> You can drop this property and use the existing 'field-even-active' property
> instead.
>
OK

>
>> +-sync-on-green: a boolean property indicating to sync with the green
>> signal in
>> + RGB.
>
>
>> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
>> index 83ae07e..b95553d 100644
>> --- a/include/media/v4l2-mediabus.h
>> +++ b/include/media/v4l2-mediabus.h
>> @@ -40,6 +40,8 @@
>> #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_FIELD_ACTIVE (1<< 12)
>> +#define V4L2_MBUS_SOG (1<< 13)
>
>
> How about V4L2_MBUS_SYNC_ON_GREEN ?
>
OK makes it more readable.

Regards,
--Prabhakar Lad