[PATCH v2 0/2] media: i2c: tvp7002: feature enhancement

[PATCH v2 0/2] media: i2c: tvp7002: feature enhancement

[Prabhakar Lad]

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

The first patch of the series add support for asynchronous probing
and the second patch adds OF support to tvp7002 driver.

Lad, Prabhakar (2):
  media: i2c: tvp7002: add support for asynchronous probing
  media: i2c: tvp7002: add OF support

 .../devicetree/bindings/media/i2c/tvp7002.txt      |   43 ++++++++++++
 drivers/media/i2c/tvp7002.c                        |   73 ++++++++++++++++++--
 2 files changed, 109 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp7002.txt

-- 
1.7.9.5

[PATCH v2 1/2] media: i2c: tvp7002: add support for asynchronous probing

[Prabhakar Lad]

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

Both synchronous and asynchronous tvp7002 subdevice probing
is supported by this patch.

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: linux-kernel@vger.kernel.org
Cc: davinci-linux-open-source@linux.davincidsp.com
---
 drivers/media/i2c/tvp7002.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
index 36ad565..b577548 100644
--- a/drivers/media/i2c/tvp7002.c
+++ b/drivers/media/i2c/tvp7002.c
@@ -31,6 +31,7 @@
 #include <linux/module.h>
 #include <linux/v4l2-dv-timings.h>
 #include <media/tvp7002.h>
+#include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
@@ -1040,6 +1041,10 @@ static int tvp7002_probe(struct i2c_client *c, const struct i2c_device_id *id)
 	}
 	v4l2_ctrl_handler_setup(&device->hdl);
 
+	error = v4l2_async_register_subdev(&device->sd);
+	if (error)
+		goto error;
+
 	return 0;
 
 error:
@@ -1064,6 +1069,7 @@ static int tvp7002_remove(struct i2c_client *c)
 
 	v4l2_dbg(1, debug, sd, "Removing tvp7002 adapter"
 				"on address 0x%x\n", c->addr);
+	v4l2_async_unregister_subdev(&device->sd);
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	media_entity_cleanup(&device->sd.entity);
 #endif
-- 
1.7.9.5

Re: [PATCH v2 1/2] media: i2c: tvp7002: add support for asynchronous probing

[Hans Verkuil]

On Sat June 22 2013 19:44:14 Prabhakar Lad wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> 
> Both synchronous and asynchronous tvp7002 subdevice probing
> is supported by this patch.

Can I merge this patch without patch 2/2? Or should I wait with both until
the video sync properties have been approved?

	Hans

> 
> 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: linux-kernel@vger.kernel.org
> Cc: davinci-linux-open-source@linux.davincidsp.com
> ---
>  drivers/media/i2c/tvp7002.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
> index 36ad565..b577548 100644
> --- a/drivers/media/i2c/tvp7002.c
> +++ b/drivers/media/i2c/tvp7002.c
> @@ -31,6 +31,7 @@
>  #include <linux/module.h>
>  #include <linux/v4l2-dv-timings.h>
>  #include <media/tvp7002.h>
> +#include <media/v4l2-async.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ctrls.h>
> @@ -1040,6 +1041,10 @@ static int tvp7002_probe(struct i2c_client *c, const struct i2c_device_id *id)
>  	}
>  	v4l2_ctrl_handler_setup(&device->hdl);
>  
> +	error = v4l2_async_register_subdev(&device->sd);
> +	if (error)
> +		goto error;
> +
>  	return 0;
>  
>  error:
> @@ -1064,6 +1069,7 @@ static int tvp7002_remove(struct i2c_client *c)
>  
>  	v4l2_dbg(1, debug, sd, "Removing tvp7002 adapter"
>  				"on address 0x%x\n", c->addr);
> +	v4l2_async_unregister_subdev(&device->sd);
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	media_entity_cleanup(&device->sd.entity);
>  #endif
> 

Re: [PATCH v2 1/2] media: i2c: tvp7002: add support for asynchronous probing

[Prabhakar Lad]

Hi Hans,

On Mon, Jun 24, 2013 at 12:44 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Sat June 22 2013 19:44:14 Prabhakar Lad wrote:
>> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>>
>> Both synchronous and asynchronous tvp7002 subdevice probing
>> is supported by this patch.
>
> Can I merge this patch without patch 2/2? Or should I wait with both until
> the video sync properties have been approved?
>
You can go ahead and merge this one no need to wait for 2/2, I know
the video sync will take more time :)

Regards,
--Prabhakar Lad

[PATCH v2 2/2] media: i2c: tvp7002: add OF support

[Prabhakar Lad]

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

add OF support for the tvp7002 driver.

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
---
 Depends on patch https://patchwork.kernel.org/patch/2765851/
 
 .../devicetree/bindings/media/i2c/tvp7002.txt      |   43 +++++++++++++
 drivers/media/i2c/tvp7002.c                        |   67 ++++++++++++++++++--
 2 files changed, 103 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp7002.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp7002.txt b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
new file mode 100644
index 0000000..9daebe1
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
@@ -0,0 +1,43 @@
+* Texas Instruments TV7002 video decoder
+
+The TVP7002 device supports digitizing of video and graphics signal in RGB and
+YPbPr color space.
+
+Required Properties :
+- compatible : Must be "ti,tvp7002"
+
+- hsync-active: HSYNC Polarity configuration for endpoint.
+
+- vsync-active: VSYNC Polarity configuration for endpoint.
+
+- pclk-sample: Clock polarity of the endpoint.
+
+- video-sync: Video sync property of the endpoint.
+
+- ti,tvp7002-fid-polarity: Active-high Field ID polarity of the endpoint.
+
+
+For further reading of port node refer Documentation/devicetree/bindings/media/
+video-interfaces.txt.
+
+Example:
+
+	i2c0@1c22000 {
+		...
+		...
+		tvp7002@5c {
+			compatible = "ti,tvp7002";
+			reg = <0x5c>;
+
+			port {
+				tvp7002_1: endpoint {
+					hsync-active = <1>;
+					vsync-active = <1>;
+					pclk-sample = <0>;
+					video-sync = <V4L2_MBUS_VIDEO_SYNC_ON_GREEN>;
+					ti,tvp7002-fid-polarity;
+				};
+			};
+		};
+		...
+	};
diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
index b577548..4896024 100644
--- a/drivers/media/i2c/tvp7002.c
+++ b/drivers/media/i2c/tvp7002.c
@@ -35,6 +35,8 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>
+
 #include "tvp7002_reg.h"
 
 MODULE_DESCRIPTION("TI TVP7002 Video and Graphics Digitizer driver");
@@ -943,6 +945,48 @@ static const struct v4l2_subdev_ops tvp7002_ops = {
 	.pad = &tvp7002_pad_ops,
 };
 
+static struct tvp7002_config *
+tvp7002_get_pdata(struct i2c_client *client)
+{
+	struct v4l2_of_endpoint bus_cfg;
+	struct tvp7002_config *pdata;
+	struct device_node *endpoint;
+	unsigned int flags;
+
+	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
+		return client->dev.platform_data;
+
+	endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
+	if (!endpoint)
+		return NULL;
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		goto done;
+
+	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+	flags = bus_cfg.bus.parallel.flags;
+
+	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+		pdata->hs_polarity = 1;
+
+	if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
+		pdata->vs_polarity = 1;
+
+	if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+		pdata->clk_polarity = 1;
+
+	if (flags & V4L2_MBUS_VIDEO_SYNC_ON_GREEN)
+		pdata->sog_polarity = 1;
+
+	pdata->fid_polarity = of_property_read_bool(endpoint,
+						    "ti,tvp7002-fid-polarity");
+
+done:
+	of_node_put(endpoint);
+	return pdata;
+}
+
 /*
  * tvp7002_probe - Probe a TVP7002 device
  * @c: ptr to i2c_client struct
@@ -954,32 +998,32 @@ static const struct v4l2_subdev_ops tvp7002_ops = {
  */
 static int tvp7002_probe(struct i2c_client *c, const struct i2c_device_id *id)
 {
+	struct tvp7002_config *pdata = tvp7002_get_pdata(c);
 	struct v4l2_subdev *sd;
 	struct tvp7002 *device;
 	struct v4l2_dv_timings timings;
 	int polarity_a;
 	int polarity_b;
 	u8 revision;
-
 	int error;
 
+	if (pdata == NULL) {
+		dev_err(&c->dev, "No platform data\n");
+		return -EINVAL;
+	}
+
 	/* Check if the adapter supports the needed features */
 	if (!i2c_check_functionality(c->adapter,
 		I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
 		return -EIO;
 
-	if (!c->dev.platform_data) {
-		v4l_err(c, "No platform data!!\n");
-		return -ENODEV;
-	}
-
 	device = devm_kzalloc(&c->dev, sizeof(struct tvp7002), GFP_KERNEL);
 
 	if (!device)
 		return -ENOMEM;
 
 	sd = &device->sd;
-	device->pdata = c->dev.platform_data;
+	device->pdata = pdata;
 	device->current_timings = tvp7002_timings;
 
 	/* Tell v4l2 the device is ready */
@@ -1085,9 +1129,18 @@ static const struct i2c_device_id tvp7002_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, tvp7002_id);
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id tvp7002_of_match[] = {
+	{ .compatible = "ti,tvp7002", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, tvp7002_of_match);
+#endif
+
 /* I2C driver data */
 static struct i2c_driver tvp7002_driver = {
 	.driver = {
+		.of_match_table = of_match_ptr(tvp7002_of_match),
 		.owner = THIS_MODULE,
 		.name = TVP7002_MODULE_NAME,
 	},
-- 
1.7.9.5

Re: [PATCH v2 2/2] media: i2c: tvp7002: add OF support

[Sylwester Nawrocki]

Hi,

On 06/22/2013 07:44 PM, Prabhakar Lad wrote:
> From: "Lad, Prabhakar"<prabhakar.csengg@gmail.com>
>
> add OF support for the tvp7002 driver.
>
> 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
> ---
>   Depends on patch https://patchwork.kernel.org/patch/2765851/
>
>   .../devicetree/bindings/media/i2c/tvp7002.txt      |   43 +++++++++++++
>   drivers/media/i2c/tvp7002.c                        |   67 ++++++++++++++++++--
>   2 files changed, 103 insertions(+), 7 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp7002.txt b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
> new file mode 100644
> index 0000000..9daebe1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
> @@ -0,0 +1,43 @@
> +* Texas Instruments TV7002 video decoder
> +
> +The TVP7002 device supports digitizing of video and graphics signal in RGB and
> +YPbPr color space.
> +
> +Required Properties :
> +- compatible : Must be "ti,tvp7002"
> +
> +- hsync-active: HSYNC Polarity configuration for endpoint.
> +
> +- vsync-active: VSYNC Polarity configuration for endpoint.
> +
> +- pclk-sample: Clock polarity of the endpoint.
> +
> +- video-sync: Video sync property of the endpoint.
> +
> +- ti,tvp7002-fid-polarity: Active-high Field ID polarity of the endpoint.

I thought it was agreed 'field-even-active' would be used instead of
this device specific property. Did you run into any issues with that ?

> +
> +For further reading of port node refer Documentation/devicetree/bindings/media/
> +video-interfaces.txt.
> +
> +Example:
> +
> +	i2c0@1c22000 {
> +		...
> +		...
> +		tvp7002@5c {
> +			compatible = "ti,tvp7002";
> +			reg =<0x5c>;
> +
> +			port {
> +				tvp7002_1: endpoint {
> +					hsync-active =<1>;
> +					vsync-active =<1>;
> +					pclk-sample =<0>;
> +					video-sync =<V4L2_MBUS_VIDEO_SYNC_ON_GREEN>;
> +					ti,tvp7002-fid-polarity;
> +				};
> +			};
> +		};
> +		...
> +	};
> diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
> index b577548..4896024 100644
> --- a/drivers/media/i2c/tvp7002.c
> +++ b/drivers/media/i2c/tvp7002.c
> @@ -35,6 +35,8 @@
>   #include<media/v4l2-device.h>
>   #include<media/v4l2-common.h>
>   #include<media/v4l2-ctrls.h>
> +#include<media/v4l2-of.h>
> +
>   #include "tvp7002_reg.h"
>
>   MODULE_DESCRIPTION("TI TVP7002 Video and Graphics Digitizer driver");
> @@ -943,6 +945,48 @@ static const struct v4l2_subdev_ops tvp7002_ops = {
>   	.pad =&tvp7002_pad_ops,
>   };
>
> +static struct tvp7002_config *
> +tvp7002_get_pdata(struct i2c_client *client)

nit: unnecessary line break

> +{
> +	struct v4l2_of_endpoint bus_cfg;
> +	struct tvp7002_config *pdata;
> +	struct device_node *endpoint;
> +	unsigned int flags;
> +
> +	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> +		return client->dev.platform_data;
> +
> +	endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> +	if (!endpoint)
> +		return NULL;
> +
> +	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		goto done;
> +
> +	v4l2_of_parse_endpoint(endpoint,&bus_cfg);
> +	flags = bus_cfg.bus.parallel.flags;
> +
> +	if (flags&  V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> +		pdata->hs_polarity = 1;
> +
> +	if (flags&  V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> +		pdata->vs_polarity = 1;
> +
> +	if (flags&  V4L2_MBUS_PCLK_SAMPLE_RISING)
> +		pdata->clk_polarity = 1;
> +
> +	if (flags&  V4L2_MBUS_VIDEO_SYNC_ON_GREEN)
> +		pdata->sog_polarity = 1;

This clearly shows that you're using this property for something different
you have defined it for. I asked previously if what you really needed for
this TVP7002 chip is a DT property indicating sync-on-green _polarity_ or
the sync-on-green _usage_. And you clearly need the polarity information.

So I would just define a standard "sync-on-green-active" property and
V4L2_MBUS_VIDEO_SOG_ACTIVE_{HIGH,LOW} flags. Presumably you don't need
anything else, and the extra sync polarity flags would need eventually
to be defined anyway, independently of any video sync selection property,
should something like this ever need to be specified by firmware.

> +	pdata->fid_polarity = of_property_read_bool(endpoint,
> +						    "ti,tvp7002-fid-polarity");

This could be just:

	if (flags & V4L2_MBUS_FIELD_EVEN_HIGH)
		pdata->fid_polarity = 1;

if you used standard 'field-even-active' property.

And this is what we find in the TVP7002 datasheet, in the section describing
MISC Control 3 (18h) register's FID POL bit (pdata->fid_polarity is written
directly to FID POL):

"FID POL: Active-high Field ID output polarity control. Under normal
operation,  the field ID output is set to logic 1 for an odd field (field 1)
and set to logic 0 for an even field (field 0).
0 = Normal operation (default)
1 = FID output polarity inverted
NOTE: This control bit also affects the polarity of the data enable output
when selected (see Test output control [2:0] at subaddress 17h)."


And include/media/tvp70002.h:

  * fid_polarity:
  *			0 -> the field ID output is set to logic 1 for an odd
  *			     field (field 1) and set to logic 0 for an even
  *			     field (field 0).
  *			1 -> operation with polarity inverted.


Do you know if the chip automatically selects video sync source 
(sync-on-green
vs. VSYNC/HSYNC) and there is no need to configure this on the analogue 
input
side ? At least the driver seems to always select the default SOGIN_1 input
(TVP7002_IN_MUX_SEL_1 register is set only at initialization time).

Or perhaps it just outputs on SOGOUT, VSOUT, HSOUT lines whatever is fed to
its analogue inputs, and any further processing unit need to determine what
synchronization signal is present and should be used ?

I suspect that we don't need, e.g. another endpoint node to specify the
configuration of the TVP7002 analogue input interface, that would contain
a property like video-sync.

> +done:
> +	of_node_put(endpoint);
> +	return pdata;
> +}

Regards,
Sylwester

Re: [PATCH v2 2/2] media: i2c: tvp7002: add OF support

[Prabhakar Lad]

Hi Sylwester,

Thanks for the review.

On Sun, Jun 30, 2013 at 9:27 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi,
>
>
> On 06/22/2013 07:44 PM, Prabhakar Lad wrote:
>>
>> From: "Lad, Prabhakar"<prabhakar.csengg@gmail.com>
>>
>> add OF support for the tvp7002 driver.
>>
>> 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

Will take care of this as mentioned in earlier patch.

>> ---
>>   Depends on patch https://patchwork.kernel.org/patch/2765851/
>>
>>   .../devicetree/bindings/media/i2c/tvp7002.txt      |   43 +++++++++++++
>>   drivers/media/i2c/tvp7002.c                        |   67
>> ++++++++++++++++++--
>>   2 files changed, 103 insertions(+), 7 deletions(-)
>>   create mode 100644
>> Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>> b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>> new file mode 100644
>> index 0000000..9daebe1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>> @@ -0,0 +1,43 @@
>> +* Texas Instruments TV7002 video decoder
>> +
>> +The TVP7002 device supports digitizing of video and graphics signal in
>> RGB and
>> +YPbPr color space.
>> +
>> +Required Properties :
>> +- compatible : Must be "ti,tvp7002"
>> +
>> +- hsync-active: HSYNC Polarity configuration for endpoint.
>> +
>> +- vsync-active: VSYNC Polarity configuration for endpoint.
>> +
>> +- pclk-sample: Clock polarity of the endpoint.
>> +
>> +- video-sync: Video sync property of the endpoint.
>> +
>> +- ti,tvp7002-fid-polarity: Active-high Field ID polarity of the endpoint.
>
>
> I thought it was agreed 'field-even-active' would be used instead of
> this device specific property. Did you run into any issues with that ?
>
>
Argh I some how missed it out, sorry this should be 'field-even-active'

>> +
>> +For further reading of port node refer
>> Documentation/devicetree/bindings/media/
>> +video-interfaces.txt.
>> +
>> +Example:
>> +
>> +       i2c0@1c22000 {
>> +               ...
>> +               ...
>> +               tvp7002@5c {
>> +                       compatible = "ti,tvp7002";
>> +                       reg =<0x5c>;
>> +
>> +                       port {
>> +                               tvp7002_1: endpoint {
>> +                                       hsync-active =<1>;
>> +                                       vsync-active =<1>;
>> +                                       pclk-sample =<0>;
>> +                                       video-sync
>> =<V4L2_MBUS_VIDEO_SYNC_ON_GREEN>;
>> +                                       ti,tvp7002-fid-polarity;
>> +                               };
>> +                       };
>> +               };
>> +               ...
>> +       };
>> diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
>> index b577548..4896024 100644
>> --- a/drivers/media/i2c/tvp7002.c
>> +++ b/drivers/media/i2c/tvp7002.c
>> @@ -35,6 +35,8 @@
>>   #include<media/v4l2-device.h>
>>   #include<media/v4l2-common.h>
>>   #include<media/v4l2-ctrls.h>
>> +#include<media/v4l2-of.h>
>> +
>>   #include "tvp7002_reg.h"
>>
>>   MODULE_DESCRIPTION("TI TVP7002 Video and Graphics Digitizer driver");
>> @@ -943,6 +945,48 @@ static const struct v4l2_subdev_ops tvp7002_ops = {
>>         .pad =&tvp7002_pad_ops,
>>   };
>>
>> +static struct tvp7002_config *
>> +tvp7002_get_pdata(struct i2c_client *client)
>
>
> nit: unnecessary line break
>

Ok

>> +{
>> +       struct v4l2_of_endpoint bus_cfg;
>> +       struct tvp7002_config *pdata;
>> +       struct device_node *endpoint;
>> +       unsigned int flags;
>> +
>> +       if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
>> +               return client->dev.platform_data;
>> +
>> +       endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
>> +       if (!endpoint)
>> +               return NULL;
>> +
>> +       pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>> +       if (!pdata)
>> +               goto done;
>> +
>> +       v4l2_of_parse_endpoint(endpoint,&bus_cfg);
>> +       flags = bus_cfg.bus.parallel.flags;
>> +
>> +       if (flags&  V4L2_MBUS_HSYNC_ACTIVE_HIGH)
>>
>> +               pdata->hs_polarity = 1;
>> +
>> +       if (flags&  V4L2_MBUS_VSYNC_ACTIVE_HIGH)
>>
>> +               pdata->vs_polarity = 1;
>> +
>> +       if (flags&  V4L2_MBUS_PCLK_SAMPLE_RISING)
>>
>> +               pdata->clk_polarity = 1;
>> +
>> +       if (flags&  V4L2_MBUS_VIDEO_SYNC_ON_GREEN)
>>
>> +               pdata->sog_polarity = 1;
>
>
> This clearly shows that you're using this property for something different
> you have defined it for. I asked previously if what you really needed for
> this TVP7002 chip is a DT property indicating sync-on-green _polarity_ or
> the sync-on-green _usage_. And you clearly need the polarity information.
>
> So I would just define a standard "sync-on-green-active" property and
> V4L2_MBUS_VIDEO_SOG_ACTIVE_{HIGH,LOW} flags. Presumably you don't need
> anything else, and the extra sync polarity flags would need eventually
> to be defined anyway, independently of any video sync selection property,
> should something like this ever need to be specified by firmware.
>
>
OK

>> +       pdata->fid_polarity = of_property_read_bool(endpoint,
>> +
>> "ti,tvp7002-fid-polarity");
>
>
> This could be just:
>
>         if (flags & V4L2_MBUS_FIELD_EVEN_HIGH)
>                 pdata->fid_polarity = 1;
>
> if you used standard 'field-even-active' property.
>

Agreed.

> And this is what we find in the TVP7002 datasheet, in the section describing
> MISC Control 3 (18h) register's FID POL bit (pdata->fid_polarity is written
> directly to FID POL):
>
> "FID POL: Active-high Field ID output polarity control. Under normal
> operation,  the field ID output is set to logic 1 for an odd field (field 1)
> and set to logic 0 for an even field (field 0).
> 0 = Normal operation (default)
> 1 = FID output polarity inverted
> NOTE: This control bit also affects the polarity of the data enable output
> when selected (see Test output control [2:0] at subaddress 17h)."
>
>
> And include/media/tvp70002.h:
>
>  * fid_polarity:
>  *                      0 -> the field ID output is set to logic 1 for an
> odd
>  *                           field (field 1) and set to logic 0 for an even
>  *                           field (field 0).
>  *                      1 -> operation with polarity inverted.
>
>
> Do you know if the chip automatically selects video sync source
> (sync-on-green
> vs. VSYNC/HSYNC) and there is no need to configure this on the analogue
> input
> side ? At least the driver seems to always select the default SOGIN_1 input
> (TVP7002_IN_MUX_SEL_1 register is set only at initialization time).
>
Yes the driver is selecting the default SOGIN_1 input.

> Or perhaps it just outputs on SOGOUT, VSOUT, HSOUT lines whatever is fed to
> its analogue inputs, and any further processing unit need to determine what
> synchronization signal is present and should be used ?
>

Yes that correct, there is a register (Sync Detect Status) which
detects the sync for
you.

> I suspect that we don't need, e.g. another endpoint node to specify the
> configuration of the TVP7002 analogue input interface, that would contain
> a property like video-sync.
>
>
If I understand correctly you mean if there are two tvp7002 devices connected
we don’t need to specify video-sync property, but my question how do we
specify this property in common then ?

Regards,
--Prabhakar Lad

Re: [PATCH v2 2/2] media: i2c: tvp7002: add OF support

[Sylwester Nawrocki]

On 07/11/2013 07:09 PM, Prabhakar Lad wrote:
[...]
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>>> b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>>> new file mode 100644
>>> index 0000000..9daebe1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>>> @@ -0,0 +1,43 @@
>>> +* Texas Instruments TV7002 video decoder
>>> +
[...]
>>> +
>>> +- ti,tvp7002-fid-polarity: Active-high Field ID polarity of the endpoint.
>>
>> I thought it was agreed 'field-even-active' would be used instead of
>> this device specific property. Did you run into any issues with that ?
>>
>>
> Argh I some how missed it out, sorry this should be 'field-even-active'

OK.

>> And include/media/tvp70002.h:
>>
>>   * fid_polarity:
>>   *                      0 ->  the field ID output is set to logic 1 for an
>> odd
>>   *                           field (field 1) and set to logic 0 for an even
>>   *                           field (field 0).
>>   *                      1 ->  operation with polarity inverted.
>>
>>
>> Do you know if the chip automatically selects video sync source
>> (sync-on-green
>> vs. VSYNC/HSYNC) and there is no need to configure this on the analogue
>> input
>> side ? At least the driver seems to always select the default SOGIN_1 input
>> (TVP7002_IN_MUX_SEL_1 register is set only at initialization time).
>>
> Yes the driver is selecting the default SOGIN_1 input.
>
>> Or perhaps it just outputs on SOGOUT, VSOUT, HSOUT lines whatever is fed to
>> its analogue inputs, and any further processing unit need to determine what
>> synchronization signal is present and should be used ?
>>
>
> Yes that correct, there is a register (Sync Detect Status) which
> detects the sync for you.
>
>> I suspect that we don't need, e.g. another endpoint node to specify the
>> configuration of the TVP7002 analogue input interface, that would contain
>> a property like video-sync.
>>
>>
> If I understand correctly you mean if there are two tvp7002 devices connected
> we don’t need to specify video-sync property, but my question how do we
> specify this property in common then ?

No, I thought about two port sub-nodes of a single device node, one for the
TVP7002 video input and one for the output. But it seems there is no need
for that, i.e. to specify the input configuration statically in the 
firmware.
The chip detects the signals automatically, i.e. it uses whatever is 
available,
and it allows querying the selection status at run time. What would really
need to be configured statically in DT in that case then ? Some initial 
video
sync configuration ? I guess it could be well hard coded in the driver, 
since
the hardware does run time detection anyway.

It there are real use cases I gues we could add video-sync property or 
similar,
besides the existing signal polarity properties.

--
Thanks,
Sylwester

Re: [PATCH v2 2/2] media: i2c: tvp7002: add OF support

[Prabhakar Lad]

On Fri, Jul 12, 2013 at 3:34 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> On 07/11/2013 07:09 PM, Prabhakar Lad wrote:
> [...]
>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>>>> b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>>>> new file mode 100644
>>>> index 0000000..9daebe1
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp7002.txt
>>>> @@ -0,0 +1,43 @@
>>>> +* Texas Instruments TV7002 video decoder
>>>> +
>
> [...]
>
>>>> +
>>>> +- ti,tvp7002-fid-polarity: Active-high Field ID polarity of the
>>>> endpoint.
>>>
>>>
>>> I thought it was agreed 'field-even-active' would be used instead of
>>> this device specific property. Did you run into any issues with that ?
>>>
>>>
>> Argh I some how missed it out, sorry this should be 'field-even-active'
>
>
> OK.
>
>
>>> And include/media/tvp70002.h:
>>>
>>>   * fid_polarity:
>>>   *                      0 ->  the field ID output is set to logic 1 for
>>> an
>>> odd
>>>   *                           field (field 1) and set to logic 0 for an
>>> even
>>>   *                           field (field 0).
>>>   *                      1 ->  operation with polarity inverted.
>>>
>>>
>>> Do you know if the chip automatically selects video sync source
>>> (sync-on-green
>>> vs. VSYNC/HSYNC) and there is no need to configure this on the analogue
>>> input
>>> side ? At least the driver seems to always select the default SOGIN_1
>>> input
>>> (TVP7002_IN_MUX_SEL_1 register is set only at initialization time).
>>>
>> Yes the driver is selecting the default SOGIN_1 input.
>>
>>> Or perhaps it just outputs on SOGOUT, VSOUT, HSOUT lines whatever is fed
>>> to
>>> its analogue inputs, and any further processing unit need to determine
>>> what
>>> synchronization signal is present and should be used ?
>>>
>>
>> Yes that correct, there is a register (Sync Detect Status) which
>> detects the sync for you.
>>
>>> I suspect that we don't need, e.g. another endpoint node to specify the
>>> configuration of the TVP7002 analogue input interface, that would contain
>>> a property like video-sync.
>>>
>>>
>> If I understand correctly you mean if there are two tvp7002 devices
>> connected
>> we don’t need to specify video-sync property, but my question how do we
>> specify this property in common then ?
>
>
> No, I thought about two port sub-nodes of a single device node, one for the
> TVP7002 video input and one for the output. But it seems there is no need
> for that, i.e. to specify the input configuration statically in the
> firmware.
> The chip detects the signals automatically, i.e. it uses whatever is
> available,
> and it allows querying the selection status at run time. What would really
> need to be configured statically in DT in that case then ? Some initial
> video
> sync configuration ? I guess it could be well hard coded in the driver,
> since
> the hardware does run time detection anyway.
>
Yes the chip detects the signal automatically, What I want to configure in
the DT case is say if SOG signal is detected, I want to invert the polarity
of it this is what I am trying to set in DT case whether to invert or not.
0 = Normal operation (default)
1 = SOG output polarity inverted

Something similar to fid_polarity.

Regards,
--Prabhakar Lad

Re: [PATCH v2 2/2] media: i2c: tvp7002: add OF support

[Sylwester Nawrocki]

Hi Prabhakar,

On 07/12/2013 06:43 AM, Prabhakar Lad wrote:
> On Fri, Jul 12, 2013 at 3:34 AM, Sylwester Nawrocki
> <sylvester.nawrocki@gmail.com>  wrote:
>> On 07/11/2013 07:09 PM, Prabhakar Lad wrote:
[...]
>>>> And include/media/tvp70002.h:
>>>>
>>>>    * fid_polarity:
>>>>    *                      0 ->   the field ID output is set to logic 1 for
>>>> an
>>>> odd
>>>>    *                           field (field 1) and set to logic 0 for an
>>>> even
>>>>    *                           field (field 0).
>>>>    *                      1 ->   operation with polarity inverted.
>>>>
>>>>
>>>> Do you know if the chip automatically selects video sync source
>>>> (sync-on-green
>>>> vs. VSYNC/HSYNC) and there is no need to configure this on the analogue
>>>> input
>>>> side ? At least the driver seems to always select the default SOGIN_1
>>>> input
>>>> (TVP7002_IN_MUX_SEL_1 register is set only at initialization time).
>>>>
>>> Yes the driver is selecting the default SOGIN_1 input.
>>>
>>>> Or perhaps it just outputs on SOGOUT, VSOUT, HSOUT lines whatever is fed
>>>> to
>>>> its analogue inputs, and any further processing unit need to determine
>>>> what
>>>> synchronization signal is present and should be used ?
>>>>
>>>
>>> Yes that correct, there is a register (Sync Detect Status) which
>>> detects the sync for you.
>>>
>>>> I suspect that we don't need, e.g. another endpoint node to specify the
>>>> configuration of the TVP7002 analogue input interface, that would contain
>>>> a property like video-sync.
>>>>
>>>>
>>> If I understand correctly you mean if there are two tvp7002 devices
>>> connected
>>> we don’t need to specify video-sync property, but my question how do we
>>> specify this property in common then ?
>>
>>
>> No, I thought about two port sub-nodes of a single device node, one for the
>> TVP7002 video input and one for the output. But it seems there is no need
>> for that, i.e. to specify the input configuration statically in the
>> firmware.
>> The chip detects the signals automatically, i.e. it uses whatever is
>> available,
>> and it allows querying the selection status at run time. What would really
>> need to be configured statically in DT in that case then ? Some initial
>> video
>> sync configuration ? I guess it could be well hard coded in the driver,
>> since
>> the hardware does run time detection anyway.
>>
> Yes the chip detects the signal automatically, What I want to configure in
> the DT case is say if SOG signal is detected, I want to invert the polarity
> of it this is what I am trying to set in DT case whether to invert or not.
> 0 = Normal operation (default)
> 1 = SOG output polarity inverted
>
> Something similar to fid_polarity.

Then as I suggested earlier, let's just add 'sync-on-green-active' DT
property for that. I wouldn't expect the DT properties to be directly
replacing your driver platform_data members. Saying in the binding that
this is a normal operation and that is an inverted one is not very useful
in general, as you would need to dig in the binding's description what
"normal" exactly means. sync-on-green-active = <1>; seems much more
explicit than, e.g. sync-on-green-inverted. By looking at the
sync-on-green-active property each device driver would determine whether
it means normal or inverted operation for its device.

--
Thanks,
Sylwester

Re: [PATCH v2 2/2] media: i2c: tvp7002: add OF support

[Prabhakar Lad]

Hi Sylwester,

On Mon, Jul 15, 2013 at 1:12 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi Prabhakar,
>
[Snip]
>> Something similar to fid_polarity.
>
>
> Then as I suggested earlier, let's just add 'sync-on-green-active' DT
> property for that. I wouldn't expect the DT properties to be directly
> replacing your driver platform_data members. Saying in the binding that
> this is a normal operation and that is an inverted one is not very useful
> in general, as you would need to dig in the binding's description what
> "normal" exactly means. sync-on-green-active = <1>; seems much more
> explicit than, e.g. sync-on-green-inverted. By looking at the
> sync-on-green-active property each device driver would determine whether
> it means normal or inverted operation for its device.
>
Ok so I'll add 'sync-on-green-active' property and parse it in
v4l2_of_parse_parallel_bus()
and add it as part of flags. and define the following flags in mediabus.h

V4L2_MBUS_VIDEO_SOG_ACTIVE_{HIGH,LOW}.

Hope you are OK with it.

--
Regards,
Prabhakar Lad