diff mbox

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

Message ID 1368622349-32185-1-git-send-email-prabhakar.csengg@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lad, Prabhakar May 15, 2013, 12:52 p.m. UTC 
From: Lad, Prabhakar <prabhakar.csengg@gmail.com>

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 <prabhakar.csengg@gmail.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Rob Landley <rob@landley.net>
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: davinci-linux-open-source@linux.davincidsp.com
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../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(-)

Comments

Laurent Pinchart May 15, 2013, 1:24 p.m. UTC | #1 
Hi Prabhakar,

Thank you for the patch.

On Wednesday 15 May 2013 18:22:29 Lad Prabhakar wrote:
> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> 
> 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 <prabhakar.csengg@gmail.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Rob Landley <rob@landley.net>
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: davinci-linux-open-source@linux.davincidsp.com
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../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 */
Lad, Prabhakar May 16, 2013, 4:53 a.m. UTC | #2 
Hi Laurent,

On Wed, May 15, 2013 at 6:54 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Wednesday 15 May 2013 18:22:29 Lad Prabhakar wrote:
>> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>>
>> 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 <prabhakar.csengg@gmail.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Cc: Sakari Ailus <sakari.ailus@iki.fi>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Rob Landley <rob@landley.net>
>> Cc: devicetree-discuss@lists.ozlabs.org
>> Cc: linux-doc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: davinci-linux-open-source@linux.davincidsp.com
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  .../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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki May 16, 2013, 10:43 a.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki May 16, 2013, 10:45 a.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lad, Prabhakar May 16, 2013, 10:52 a.m. UTC | #5 
Hi Sylwester,

On Thu, May 16, 2013 at 4:13 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lad, Prabhakar May 16, 2013, 10:54 a.m. UTC | #6 
Hi Sylwester,

Thanks for the review.

On Thu, May 16, 2013 at 4:15 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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