[PATCH RFC v2] media: tvp514x: add OF support

[PATCH RFC v2] media: tvp514x: add OF support

[Prabhakar Lad]

From: Lad, Prabhakar <prabhakar.lad@ti.com>

add OF support for the tvp514x driver.

Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.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
---
 Changes for v2:
  1: Fixed review comment pointed by Heiko.
  2: Fixed review comment pointed by Laurent.

 This patch is on top of following patches:
  1: https://patchwork.kernel.org/patch/1930941/
  2: http://patchwork.linuxtv.org/patch/16193/
  3: https://patchwork.kernel.org/patch/1944901

 .../devicetree/bindings/media/i2c/tvp514x.txt      |   38 +++++++++++
 drivers/media/i2c/tvp514x.c                        |   70 ++++++++++++++++++--
 2 files changed, 103 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp514x.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp514x.txt b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
new file mode 100644
index 0000000..55d3ffd
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
@@ -0,0 +1,38 @@
+* Texas Instruments TVP514x video decoder
+
+The TVP5146/TVP5146m2/TVP5147/TVP5147m1 device is high quality, single-chip
+digital video decoder that digitizes and decodes all popular baseband analog
+video formats into digital video component. The tvp514x decoder supports analog-
+to-digital (A/D) conversion of component RGB and YPbPr signals as well as A/D
+conversion and decoding of NTSC, PAL and SECAM composite and S-video into
+component YCbCr.
+
+Required Properties :
+- compatible: Must be "ti,tvp514x-decoder"
+- hsync-active: HSYNC Polarity configuration for current interface.
+- vsync-active: VSYNC Polarity configuration for current interface.
+- data-active: Clock polarity of the current interface.
+
+Example:
+
+i2c0@1c22000 {
+	...
+	...
+
+	tvp514x@5c {
+		compatible = "ti,tvp514x-decoder";
+		reg = <0x5c>;
+
+		port {
+			tvp514x_1: endpoint {
+				/* Active high (Defaults to 0) */
+				hsync-active = <1>;
+				/* Active high (Defaults to 0) */
+				hsync-active = <1>;
+				/* Active low (Defaults to 0) */
+				data-active = <0>;
+			};
+		};
+	};
+	...
+};
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index a4f0a70..24ce759 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -12,6 +12,7 @@
  *     Hardik Shah <hardik.shah@ti.com>
  *     Manjunath Hadli <mrh@ti.com>
  *     Karicheri Muralidharan <m-karicheri2@ti.com>
+ *     Prabhakar Lad <prabhakar.lad@ti.com>
  *
  * This package is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -33,7 +34,9 @@
 #include <linux/delay.h>
 #include <linux/videodev2.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 
+#include <media/v4l2-of.h>
 #include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-common.h>
@@ -930,6 +933,58 @@ static struct tvp514x_decoder tvp514x_dev = {
 
 };
 
+#if defined(CONFIG_OF)
+static const struct of_device_id tvp514x_of_match[] = {
+	{.compatible = "ti,tvp514x-decoder", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tvp514x_of_match);
+
+static struct tvp514x_platform_data
+	*tvp514x_get_pdata(struct i2c_client *client)
+{
+	if (!client->dev.platform_data && client->dev.of_node) {
+		struct tvp514x_platform_data *pdata;
+		struct v4l2_of_endpoint bus_cfg;
+		struct device_node *endpoint;
+
+		pdata = devm_kzalloc(&client->dev,
+				sizeof(struct tvp514x_platform_data),
+				GFP_KERNEL);
+		client->dev.platform_data = pdata;
+		if (!pdata)
+			return NULL;
+
+		endpoint = of_get_child_by_name(client->dev.of_node, "port");
+		if (endpoint)
+			endpoint = of_get_child_by_name(endpoint, "endpoint");
+
+		if (!endpoint) {
+			v4l2_info(client, "Using default data!!\n");
+		} else {
+			v4l2_of_parse_parallel_bus(endpoint, &bus_cfg);
+
+			if (bus_cfg.mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+				pdata->hs_polarity = 1;
+			if (bus_cfg.mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
+				pdata->vs_polarity = 1;
+			if (bus_cfg.mbus_flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+				pdata->clk_polarity = 1;
+		}
+	}
+
+	return client->dev.platform_data;
+}
+#else
+#define tvp514x_of_match NULL
+
+static struct tvp514x_platform_data
+	*tvp514x_get_pdata(struct i2c_client *client)
+{
+	return client->dev.platform_data;
+}
+#endif
+
 /**
  * tvp514x_probe() - decoder driver i2c probe handler
  * @client: i2c driver client device structure
@@ -941,6 +996,7 @@ static struct tvp514x_decoder tvp514x_dev = {
 static int
 tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
+	struct tvp514x_platform_data *pdata;
 	struct tvp514x_decoder *decoder;
 	struct v4l2_subdev *sd;
 	int ret;
@@ -949,22 +1005,25 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -EIO;
 
+	pdata = tvp514x_get_pdata(client);
+	if (!pdata) {
+		v4l2_err(client, "No platform data!!\n");
+		return -EPROBE_DEFER;
+	}
+
 	decoder = devm_kzalloc(&client->dev, sizeof(*decoder), GFP_KERNEL);
 	if (!decoder)
 		return -ENOMEM;
 
 	/* Initialize the tvp514x_decoder with default configuration */
 	*decoder = tvp514x_dev;
-	if (!client->dev.platform_data) {
-		v4l2_err(client, "No platform data!!\n");
-		return -EPROBE_DEFER;
-	}
+
 	/* Copy default register configuration */
 	memcpy(decoder->tvp514x_regs, tvp514x_reg_list_default,
 			sizeof(tvp514x_reg_list_default));
 
 	/* Copy board specific information here */
-	decoder->pdata = client->dev.platform_data;
+	decoder->pdata = pdata;
 
 	/**
 	 * Fetch platform specific data, and configure the
@@ -1096,6 +1155,7 @@ MODULE_DEVICE_TABLE(i2c, tvp514x_id);
 
 static struct i2c_driver tvp514x_driver = {
 	.driver = {
+		.of_match_table = tvp514x_of_match,
 		.owner = THIS_MODULE,
 		.name = TVP514X_MODULE_NAME,
 	},
-- 
1.7.4.1

Re: [PATCH RFC v2] media: tvp514x: add OF support

[Sylwester Nawrocki]

Hi Prabhakar,

On 01/29/2013 02:07 PM, Prabhakar Lad wrote:
[...]
> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp514x.txt b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
> new file mode 100644
> index 0000000..55d3ffd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
> @@ -0,0 +1,38 @@
> +* Texas Instruments TVP514x video decoder
> +
> +The TVP5146/TVP5146m2/TVP5147/TVP5147m1 device is high quality, single-chip
> +digital video decoder that digitizes and decodes all popular baseband analog
> +video formats into digital video component. The tvp514x decoder supports analog-
> +to-digital (A/D) conversion of component RGB and YPbPr signals as well as A/D
> +conversion and decoding of NTSC, PAL and SECAM composite and S-video into
> +component YCbCr.
> +
> +Required Properties :
> +- compatible: Must be "ti,tvp514x-decoder"

There are no significant differences among TVP514* devices as listed above,
you would like to handle above ?

I'm just wondering if you don't need ,for instance, two separate compatible
properties, e.g. "ti,tvp5146-decoder" and "ti,tvp5147-decoder" ?

> +- hsync-active: HSYNC Polarity configuration for current interface.
> +- vsync-active: VSYNC Polarity configuration for current interface.
> +- data-active: Clock polarity of the current interface.

I guess you mean "pclk-sample: Clock polarity..." ?

> +
> +Example:
> +
> +i2c0@1c22000 {
> +	...
> +	...
> +
> +	tvp514x@5c {
> +		compatible = "ti,tvp514x-decoder";
> +		reg =<0x5c>;
> +
> +		port {
> +			tvp514x_1: endpoint {
> +				/* Active high (Defaults to 0) */
> +				hsync-active =<1>;
> +				/* Active high (Defaults to 0) */
> +				hsync-active =<1>;

Should it be vsync-active ?

> +				/* Active low (Defaults to 0) */
> +				data-active =<0>;

and this pclk-sample ?

> +			};
> +		};
> +	};
> +	...
> +};
> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> index a4f0a70..24ce759 100644
> --- a/drivers/media/i2c/tvp514x.c
> +++ b/drivers/media/i2c/tvp514x.c
> @@ -12,6 +12,7 @@
>    *     Hardik Shah<hardik.shah@ti.com>
>    *     Manjunath Hadli<mrh@ti.com>
>    *     Karicheri Muralidharan<m-karicheri2@ti.com>
> + *     Prabhakar Lad<prabhakar.lad@ti.com>
>    *
>    * This package is free software; you can redistribute it and/or modify
>    * it under the terms of the GNU General Public License version 2 as
> @@ -33,7 +34,9 @@
>   #include<linux/delay.h>
>   #include<linux/videodev2.h>
>   #include<linux/module.h>
> +#include<linux/of_device.h>
>
> +#include<media/v4l2-of.h>
>   #include<media/v4l2-async.h>
>   #include<media/v4l2-device.h>
>   #include<media/v4l2-common.h>
> @@ -930,6 +933,58 @@ static struct tvp514x_decoder tvp514x_dev = {
>
>   };
>
> +#if defined(CONFIG_OF)
> +static const struct of_device_id tvp514x_of_match[] = {
> +	{.compatible = "ti,tvp514x-decoder", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, tvp514x_of_match);
> +
> +static struct tvp514x_platform_data
> +	*tvp514x_get_pdata(struct i2c_client *client)
> +{
> +	if (!client->dev.platform_data&&  client->dev.of_node) {
> +		struct tvp514x_platform_data *pdata;
> +		struct v4l2_of_endpoint bus_cfg;
> +		struct device_node *endpoint;
> +
> +		pdata = devm_kzalloc(&client->dev,
> +				sizeof(struct tvp514x_platform_data),
> +				GFP_KERNEL);
> +		client->dev.platform_data = pdata;

Do you really need to set client->dev.platform_data this way ?
What about passing struct tvp514x_decoder pointer to this function
and initializing struct tvp514x_decoder::pdata instead ?

> +		if (!pdata)
> +			return NULL;
> +
> +		endpoint = of_get_child_by_name(client->dev.of_node, "port");
> +		if (endpoint)
> +			endpoint = of_get_child_by_name(endpoint, "endpoint");

I think you could replace these two calls above with

		endpoint = v4l2_of_get_next_endpoint(client->dev.of_node);

Now I see I should have modified this function to ensure it works also when
'port' nodes are grouped in a 'ports' node.

> +		if (!endpoint) {
> +			v4l2_info(client, "Using default data!!\n");
> +		} else {	
> +			v4l2_of_parse_parallel_bus(endpoint,&bus_cfg);
> +
> +			if (bus_cfg.mbus_flags&  V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> +				pdata->hs_polarity = 1;
> +			if (bus_cfg.mbus_flags&  V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> +				pdata->vs_polarity = 1;
> +			if (bus_cfg.mbus_flags&  V4L2_MBUS_PCLK_SAMPLE_RISING)
> +				pdata->clk_polarity = 1;
> +		}
> +	}
> +
> +	return client->dev.platform_data;
> +}
> +#else
> +#define tvp514x_of_match NULL
> +
> +static struct tvp514x_platform_data
> +	*tvp514x_get_pdata(struct i2c_client *client)
> +{
> +	return client->dev.platform_data;
> +}
> +#endif
> +
>   /**
>    * tvp514x_probe() - decoder driver i2c probe handler
>    * @client: i2c driver client device structure
> @@ -941,6 +996,7 @@ static struct tvp514x_decoder tvp514x_dev = {
>   static int
>   tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>   {
> +	struct tvp514x_platform_data *pdata;
>   	struct tvp514x_decoder *decoder;
>   	struct v4l2_subdev *sd;
>   	int ret;
> @@ -949,22 +1005,25 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>   	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>   		return -EIO;
>
> +	pdata = tvp514x_get_pdata(client);
> +	if (!pdata) {
> +		v4l2_err(client, "No platform data!!\n");
> +		return -EPROBE_DEFER;
> +	}
> +
>   	decoder = devm_kzalloc(&client->dev, sizeof(*decoder), GFP_KERNEL);
>   	if (!decoder)
>   		return -ENOMEM;
>
>   	/* Initialize the tvp514x_decoder with default configuration */
>   	*decoder = tvp514x_dev;
> -	if (!client->dev.platform_data) {
> -		v4l2_err(client, "No platform data!!\n");
> -		return -EPROBE_DEFER;
> -	}
> +
>   	/* Copy default register configuration */
>   	memcpy(decoder->tvp514x_regs, tvp514x_reg_list_default,
>   			sizeof(tvp514x_reg_list_default));
>
>   	/* Copy board specific information here */
> -	decoder->pdata = client->dev.platform_data;
> +	decoder->pdata = pdata;
>
>   	/**
>   	 * Fetch platform specific data, and configure the
> @@ -1096,6 +1155,7 @@ MODULE_DEVICE_TABLE(i2c, tvp514x_id);
>
>   static struct i2c_driver tvp514x_driver = {
>   	.driver = {
> +		.of_match_table = tvp514x_of_match,
>   		.owner = THIS_MODULE,
>   		.name = TVP514X_MODULE_NAME,
>   	},

--

Regards,
Sylwester

Re: [PATCH RFC v2] media: tvp514x: add OF support

[Prabhakar Lad]

Hi Sylwester,

Thanks for the review.

On Sat, Feb 2, 2013 at 4:07 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi Prabhakar,
>
> On 01/29/2013 02:07 PM, Prabhakar Lad wrote:
> [...]
>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>> b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>> new file mode 100644
>> index 0000000..55d3ffd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>> @@ -0,0 +1,38 @@
>> +* Texas Instruments TVP514x video decoder
>> +
>> +The TVP5146/TVP5146m2/TVP5147/TVP5147m1 device is high quality,
>> single-chip
>> +digital video decoder that digitizes and decodes all popular baseband
>> analog
>> +video formats into digital video component. The tvp514x decoder supports
>> analog-
>> +to-digital (A/D) conversion of component RGB and YPbPr signals as well as
>> A/D
>> +conversion and decoding of NTSC, PAL and SECAM composite and S-video into
>> +component YCbCr.
>> +
>> +Required Properties :
>> +- compatible: Must be "ti,tvp514x-decoder"
>
>
> There are no significant differences among TVP514* devices as listed above,
> you would like to handle above ?
>
> I'm just wondering if you don't need ,for instance, two separate compatible
> properties, e.g. "ti,tvp5146-decoder" and "ti,tvp5147-decoder" ?
>
There are few differences in init/power sequence tough, I would still
like to have
single compatible property "ti,tvp514x-decoder", If you feel we need separate
property I will change it please let me know on this.

>
>> +- hsync-active: HSYNC Polarity configuration for current interface.
>> +- vsync-active: VSYNC Polarity configuration for current interface.
>> +- data-active: Clock polarity of the current interface.
>
>
> I guess you mean "pclk-sample: Clock polarity..." ?
>
>
yeah this needs to be 'pclk-sample' property, thanks for pointing it.

>> +
>> +Example:
>> +
>> +i2c0@1c22000 {
>> +       ...
>> +       ...
>> +
>> +       tvp514x@5c {
>> +               compatible = "ti,tvp514x-decoder";
>> +               reg =<0x5c>;
>> +
>> +               port {
>> +                       tvp514x_1: endpoint {
>> +                               /* Active high (Defaults to 0) */
>> +                               hsync-active =<1>;
>> +                               /* Active high (Defaults to 0) */
>> +                               hsync-active =<1>;
>
>
> Should it be vsync-active ?
>
Yes, I'll change it.
>
>> +                               /* Active low (Defaults to 0) */
>> +                               data-active =<0>;
>
>
> and this pclk-sample ?
>
Yes.

>> +                       };
>> +               };
>> +       };
>> +       ...
>> +};
>> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
>> index a4f0a70..24ce759 100644
>> --- a/drivers/media/i2c/tvp514x.c
>> +++ b/drivers/media/i2c/tvp514x.c
>> @@ -12,6 +12,7 @@
>>    *     Hardik Shah<hardik.shah@ti.com>
>>    *     Manjunath Hadli<mrh@ti.com>
>>    *     Karicheri Muralidharan<m-karicheri2@ti.com>
>> + *     Prabhakar Lad<prabhakar.lad@ti.com>
>>    *
>>    * This package is free software; you can redistribute it and/or modify
>>    * it under the terms of the GNU General Public License version 2 as
>> @@ -33,7 +34,9 @@
>>   #include<linux/delay.h>
>>   #include<linux/videodev2.h>
>>   #include<linux/module.h>
>> +#include<linux/of_device.h>
>>
>> +#include<media/v4l2-of.h>
>>   #include<media/v4l2-async.h>
>>   #include<media/v4l2-device.h>
>>   #include<media/v4l2-common.h>
>> @@ -930,6 +933,58 @@ static struct tvp514x_decoder tvp514x_dev = {
>>
>>   };
>>
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id tvp514x_of_match[] = {
>> +       {.compatible = "ti,tvp514x-decoder", },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, tvp514x_of_match);
>> +
>> +static struct tvp514x_platform_data
>> +       *tvp514x_get_pdata(struct i2c_client *client)
>> +{
>> +       if (!client->dev.platform_data&&  client->dev.of_node) {
>>
>> +               struct tvp514x_platform_data *pdata;
>> +               struct v4l2_of_endpoint bus_cfg;
>> +               struct device_node *endpoint;
>> +
>> +               pdata = devm_kzalloc(&client->dev,
>> +                               sizeof(struct tvp514x_platform_data),
>> +                               GFP_KERNEL);
>> +               client->dev.platform_data = pdata;
>
>
> Do you really need to set client->dev.platform_data this way ?
> What about passing struct tvp514x_decoder pointer to this function
> and initializing struct tvp514x_decoder::pdata instead ?
>
>
Yes that can work too, I'll do the same.

>> +               if (!pdata)
>> +                       return NULL;
>> +
>> +               endpoint = of_get_child_by_name(client->dev.of_node,
>> "port");
>> +               if (endpoint)
>> +                       endpoint = of_get_child_by_name(endpoint,
>> "endpoint");
>
>
> I think you could replace these two calls above with
>
>                 endpoint = v4l2_of_get_next_endpoint(client->dev.of_node);
>
Ok

> Now I see I should have modified this function to ensure it works also when
> 'port' nodes are grouped in a 'ports' node.
>
So V5 series of V4l OF parser doesn't have this fix ?

Regards,
--Prabhakar

>> +               if (!endpoint) {
>> +                       v4l2_info(client, "Using default data!!\n");
>> +               } else {
>> +                       v4l2_of_parse_parallel_bus(endpoint,&bus_cfg);
>> +
>> +                       if (bus_cfg.mbus_flags&
>> V4L2_MBUS_HSYNC_ACTIVE_HIGH)
>>
>> +                               pdata->hs_polarity = 1;
>> +                       if (bus_cfg.mbus_flags&
>> V4L2_MBUS_VSYNC_ACTIVE_HIGH)
>>
>> +                               pdata->vs_polarity = 1;
>> +                       if (bus_cfg.mbus_flags&
>> V4L2_MBUS_PCLK_SAMPLE_RISING)
>>
>> +                               pdata->clk_polarity = 1;
>> +               }
>> +       }
>> +
>> +       return client->dev.platform_data;
>> +}
>> +#else
>> +#define tvp514x_of_match NULL
>> +
>> +static struct tvp514x_platform_data
>> +       *tvp514x_get_pdata(struct i2c_client *client)
>> +{
>> +       return client->dev.platform_data;
>> +}
>> +#endif
>> +
>>   /**
>>    * tvp514x_probe() - decoder driver i2c probe handler
>>    * @client: i2c driver client device structure
>> @@ -941,6 +996,7 @@ static struct tvp514x_decoder tvp514x_dev = {
>>   static int
>>   tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>   {
>> +       struct tvp514x_platform_data *pdata;
>>         struct tvp514x_decoder *decoder;
>>         struct v4l2_subdev *sd;
>>         int ret;
>> @@ -949,22 +1005,25 @@ tvp514x_probe(struct i2c_client *client, const
>> struct i2c_device_id *id)
>>         if (!i2c_check_functionality(client->adapter,
>> I2C_FUNC_SMBUS_BYTE_DATA))
>>                 return -EIO;
>>
>> +       pdata = tvp514x_get_pdata(client);
>> +       if (!pdata) {
>> +               v4l2_err(client, "No platform data!!\n");
>> +               return -EPROBE_DEFER;
>> +       }
>> +
>>         decoder = devm_kzalloc(&client->dev, sizeof(*decoder),
>> GFP_KERNEL);
>>         if (!decoder)
>>                 return -ENOMEM;
>>
>>         /* Initialize the tvp514x_decoder with default configuration */
>>         *decoder = tvp514x_dev;
>> -       if (!client->dev.platform_data) {
>> -               v4l2_err(client, "No platform data!!\n");
>> -               return -EPROBE_DEFER;
>> -       }
>> +
>>         /* Copy default register configuration */
>>         memcpy(decoder->tvp514x_regs, tvp514x_reg_list_default,
>>                         sizeof(tvp514x_reg_list_default));
>>
>>         /* Copy board specific information here */
>> -       decoder->pdata = client->dev.platform_data;
>> +       decoder->pdata = pdata;
>>
>>         /**
>>          * Fetch platform specific data, and configure the
>> @@ -1096,6 +1155,7 @@ MODULE_DEVICE_TABLE(i2c, tvp514x_id);
>>
>>   static struct i2c_driver tvp514x_driver = {
>>         .driver = {
>> +               .of_match_table = tvp514x_of_match,
>>                 .owner = THIS_MODULE,
>>                 .name = TVP514X_MODULE_NAME,
>>         },
>
>
> --
>
> Regards,
> Sylwester

Re: [PATCH RFC v2] media: tvp514x: add OF support

[Sekhar Nori]

On 2/3/2013 3:43 PM, Prabhakar Lad wrote:
> Hi Sylwester,
> 
> Thanks for the review.
> 
> On Sat, Feb 2, 2013 at 4:07 AM, Sylwester Nawrocki
> <sylvester.nawrocki@gmail.com> wrote:
>> Hi Prabhakar,
>>
>> On 01/29/2013 02:07 PM, Prabhakar Lad wrote:
>> [...]
>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>>> b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>>> new file mode 100644
>>> index 0000000..55d3ffd
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>>> @@ -0,0 +1,38 @@
>>> +* Texas Instruments TVP514x video decoder
>>> +
>>> +The TVP5146/TVP5146m2/TVP5147/TVP5147m1 device is high quality,
>>> single-chip
>>> +digital video decoder that digitizes and decodes all popular baseband
>>> analog
>>> +video formats into digital video component. The tvp514x decoder supports
>>> analog-
>>> +to-digital (A/D) conversion of component RGB and YPbPr signals as well as
>>> A/D
>>> +conversion and decoding of NTSC, PAL and SECAM composite and S-video into
>>> +component YCbCr.
>>> +
>>> +Required Properties :
>>> +- compatible: Must be "ti,tvp514x-decoder"
>>
>>
>> There are no significant differences among TVP514* devices as listed above,
>> you would like to handle above ?
>>
>> I'm just wondering if you don't need ,for instance, two separate compatible
>> properties, e.g. "ti,tvp5146-decoder" and "ti,tvp5147-decoder" ?
>>
> There are few differences in init/power sequence tough, I would still
> like to have
> single compatible property "ti,tvp514x-decoder", If you feel we need separate
> property I will change it please let me know on this.

Compatible properties should not use generic part numbers. See one past
discussion here: http://en.usenet.digipedia.org/thread/18472/20788/

Thanks,
Sekhar

Re: [PATCH RFC v2] media: tvp514x: add OF support

[Laurent Pinchart]

Hi Prabhakar,

On Sunday 03 February 2013 15:43:49 Prabhakar Lad wrote:
> On Sat, Feb 2, 2013 at 4:07 AM, Sylwester Nawrocki wrote:
> > On 01/29/2013 02:07 PM, Prabhakar Lad wrote:
> > [...]
> > 
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
> >> b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
> >> new file mode 100644
> >> index 0000000..55d3ffd
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
> >> @@ -0,0 +1,38 @@
> >> +* Texas Instruments TVP514x video decoder
> >> +
> >> +The TVP5146/TVP5146m2/TVP5147/TVP5147m1 device is high quality,
> >> +single-chip digital video decoder that digitizes and decodes all popular
> >> +baseband analog video formats into digital video component. The tvp514x
> >> +decoder supports analog-to-digital (A/D) conversion of component RGB and
> >> +YPbPr signals as well as A/D conversion and decoding of NTSC, PAL and
> >> +SECAM composite and S-video into component YCbCr.
> >> +
> >> +Required Properties :
> >> +- compatible: Must be "ti,tvp514x-decoder"
> > 
> > There are no significant differences among TVP514* devices as listed
> > above, you would like to handle above ?
> > 
> > I'm just wondering if you don't need, for instance, two separate
> > compatible properties, e.g. "ti,tvp5146-decoder" and "ti,tvp5147-decoder"
> > ?
> 
> There are few differences in init/power sequence tough, I would still
> like to have single compatible property "ti,tvp514x-decoder", If you feel we
> need separate property I will change it please let me know on this.

If there's any difference between the chips that need to be handled in the 
driver, using two compatible properties is a good practice. Your driver will 
then be able to easily differentiate between the two chips, and there's no 
real drawback in doing so.

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC v2] media: tvp514x: add OF support

[Sylwester Nawrocki]

Hi Prabhakar,

On 02/03/2013 11:13 AM, Prabhakar Lad wrote:
[...]
>>> +Required Properties :
>>> +- compatible: Must be "ti,tvp514x-decoder"
>>
>>
>> There are no significant differences among TVP514* devices as listed above,
>> you would like to handle above ?

Sorry for the mangled sentence, I intended to write "in the driver" instead
of the last "above".

>> I'm just wondering if you don't need ,for instance, two separate compatible
>> properties, e.g. "ti,tvp5146-decoder" and "ti,tvp5147-decoder" ?
>>
> There are few differences in init/power sequence tough, I would still
> like to have
> single compatible property "ti,tvp514x-decoder", If you feel we need separate
> property I will change it please let me know on this.

As Sekhar already mentioned, wildcards in the compatible property should
not be used. You could just use exact part names in the compatible
properties and list them all in the tvp514x_of_match[] array. Even though
this driver doesn't care about the differences between various tvp514?
chips, there might be others that do.

[...]
>>> +#if defined(CONFIG_OF)
>>> +static const struct of_device_id tvp514x_of_match[] = {
>>> +       {.compatible = "ti,tvp514x-decoder", },
>>> +       {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, tvp514x_of_match);
>>> +
>>> +static struct tvp514x_platform_data
>>> +       *tvp514x_get_pdata(struct i2c_client *client)
>>> +{
>>> +       if (!client->dev.platform_data&&   client->dev.of_node) {
>>>
>>> +               struct tvp514x_platform_data *pdata;
>>> +               struct v4l2_of_endpoint bus_cfg;
>>> +               struct device_node *endpoint;
>>> +
>>> +               pdata = devm_kzalloc(&client->dev,
>>> +                               sizeof(struct tvp514x_platform_data),
>>> +                               GFP_KERNEL);
>>> +               client->dev.platform_data = pdata;
>>
>>
>> Do you really need to set client->dev.platform_data this way ?
>> What about passing struct tvp514x_decoder pointer to this function
>> and initializing struct tvp514x_decoder::pdata instead ?
>>
>>
> Yes that can work too, I'll do the same.

Ok, thanks.

>>> +               if (!pdata)
>>> +                       return NULL;
>>> +
>>> +               endpoint = of_get_child_by_name(client->dev.of_node,
>>> "port");
>>> +               if (endpoint)
>>> +                       endpoint = of_get_child_by_name(endpoint,
>>> "endpoint");
>>
>>
>> I think you could replace these two calls above with
>>
>>                  endpoint = v4l2_of_get_next_endpoint(client->dev.of_node);
>>
> Ok
>
>> Now I see I should have modified this function to ensure it works also when
>> 'port' nodes are grouped in a 'ports' node.
>>
> So V5 series of V4l OF parser doesn't have this fix ?

No, it doesn't. I think we need something along the lines of:

diff --git a/drivers/media/v4l2-core/v4l2-of.c 
b/drivers/media/v4l2-core/v4l2-of.c
index e1f570b..8a286f1 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -185,10 +185,15 @@ struct device_node 
*v4l2_of_get_next_endpoint(const struct device_node *parent,
                  * This is the first call, we have to find a port within
                  * this node.
                  */
-               for_each_child_of_node(parent, port) {
+               while (port = of_get_next_child(parent, port)) {
                         if (!of_node_cmp(port->name, "port"))
                                 break;
-               }
+                       if (!of_node_cmp(port->name, "ports")) {
+                               parent = port;
+                               of_node_put(port);
+                               port = NULL:
+                       }
+               };
                 if (port) {
                         /* Found a port, get an endpoint. */
                         endpoint = of_get_next_child(port, NULL);

However this shouldn't affect you, as you don't use the 'ports' node...
I will likely post v6 including this fix tomorrow.

--

Regards,
Sylwester

Re: [PATCH RFC v2] media: tvp514x: add OF support

[Prabhakar Lad]

Sekhar,

On Sun, Feb 3, 2013 at 6:40 PM, Sekhar Nori <nsekhar@ti.com> wrote:
> On 2/3/2013 3:43 PM, Prabhakar Lad wrote:
>> Hi Sylwester,
>>
>> Thanks for the review.
>>
>> On Sat, Feb 2, 2013 at 4:07 AM, Sylwester Nawrocki
>> <sylvester.nawrocki@gmail.com> wrote:
>>> Hi Prabhakar,
>>>
>>> On 01/29/2013 02:07 PM, Prabhakar Lad wrote:
>>> [...]
>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>>>> b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>>>> new file mode 100644
>>>> index 0000000..55d3ffd
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>>>> @@ -0,0 +1,38 @@
>>>> +* Texas Instruments TVP514x video decoder
>>>> +
>>>> +The TVP5146/TVP5146m2/TVP5147/TVP5147m1 device is high quality,
>>>> single-chip
>>>> +digital video decoder that digitizes and decodes all popular baseband
>>>> analog
>>>> +video formats into digital video component. The tvp514x decoder supports
>>>> analog-
>>>> +to-digital (A/D) conversion of component RGB and YPbPr signals as well as
>>>> A/D
>>>> +conversion and decoding of NTSC, PAL and SECAM composite and S-video into
>>>> +component YCbCr.
>>>> +
>>>> +Required Properties :
>>>> +- compatible: Must be "ti,tvp514x-decoder"
>>>
>>>
>>> There are no significant differences among TVP514* devices as listed above,
>>> you would like to handle above ?
>>>
>>> I'm just wondering if you don't need ,for instance, two separate compatible
>>> properties, e.g. "ti,tvp5146-decoder" and "ti,tvp5147-decoder" ?
>>>
>> There are few differences in init/power sequence tough, I would still
>> like to have
>> single compatible property "ti,tvp514x-decoder", If you feel we need separate
>> property I will change it please let me know on this.
>
> Compatible properties should not use generic part numbers. See one past
> discussion here: http://en.usenet.digipedia.org/thread/18472/20788/
>
Thanks for the link, I'll have separate compatible properties.

Regards,
--Prabhakar

> Thanks,
> Sekhar

Re: [PATCH RFC v2] media: tvp514x: add OF support

[Prabhakar Lad]

Hi Laurent,

On Sun, Feb 3, 2013 at 6:44 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Prabhakar,
>
> On Sunday 03 February 2013 15:43:49 Prabhakar Lad wrote:
>> On Sat, Feb 2, 2013 at 4:07 AM, Sylwester Nawrocki wrote:
>> > On 01/29/2013 02:07 PM, Prabhakar Lad wrote:
>> > [...]
>> >
>> >> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>> >> b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>> >> new file mode 100644
>> >> index 0000000..55d3ffd
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
>> >> @@ -0,0 +1,38 @@
>> >> +* Texas Instruments TVP514x video decoder
>> >> +
>> >> +The TVP5146/TVP5146m2/TVP5147/TVP5147m1 device is high quality,
>> >> +single-chip digital video decoder that digitizes and decodes all popular
>> >> +baseband analog video formats into digital video component. The tvp514x
>> >> +decoder supports analog-to-digital (A/D) conversion of component RGB and
>> >> +YPbPr signals as well as A/D conversion and decoding of NTSC, PAL and
>> >> +SECAM composite and S-video into component YCbCr.
>> >> +
>> >> +Required Properties :
>> >> +- compatible: Must be "ti,tvp514x-decoder"
>> >
>> > There are no significant differences among TVP514* devices as listed
>> > above, you would like to handle above ?
>> >
>> > I'm just wondering if you don't need, for instance, two separate
>> > compatible properties, e.g. "ti,tvp5146-decoder" and "ti,tvp5147-decoder"
>> > ?
>>
>> There are few differences in init/power sequence tough, I would still
>> like to have single compatible property "ti,tvp514x-decoder", If you feel we
>> need separate property I will change it please let me know on this.
>
> If there's any difference between the chips that need to be handled in the
> driver, using two compatible properties is a good practice. Your driver will
> then be able to easily differentiate between the two chips, and there's no
> real drawback in doing so.
>
Ok I'll will have separate compatible properties.

Regards,
--Prabhakar

> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH RFC v2] media: tvp514x: add OF support

[Prabhakar Lad]

Hi Sylwester,

On Sun, Feb 3, 2013 at 6:57 PM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi Prabhakar,
>
> On 02/03/2013 11:13 AM, Prabhakar Lad wrote:
> [...]
>
>>>> +Required Properties :
>>>> +- compatible: Must be "ti,tvp514x-decoder"
>>>
>>>
>>>
>>> There are no significant differences among TVP514* devices as listed
>>> above,
>>> you would like to handle above ?
>
>
> Sorry for the mangled sentence, I intended to write "in the driver" instead
> of the last "above".
>
>
Not a problem I got what you intended :)

>>> I'm just wondering if you don't need ,for instance, two separate
>>> compatible
>>> properties, e.g. "ti,tvp5146-decoder" and "ti,tvp5147-decoder" ?
>>>
>> There are few differences in init/power sequence tough, I would still
>> like to have
>> single compatible property "ti,tvp514x-decoder", If you feel we need
>> separate
>> property I will change it please let me know on this.
>
>
> As Sekhar already mentioned, wildcards in the compatible property should
> not be used. You could just use exact part names in the compatible
> properties and list them all in the tvp514x_of_match[] array. Even though
> this driver doesn't care about the differences between various tvp514?
> chips, there might be others that do.
>
> [...]
>
Ok, I'll have separate compatible properties.

>>>> +#if defined(CONFIG_OF)
>>>> +static const struct of_device_id tvp514x_of_match[] = {
>>>> +       {.compatible = "ti,tvp514x-decoder", },
>>>> +       {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, tvp514x_of_match);
>>>> +
>>>> +static struct tvp514x_platform_data
>>>> +       *tvp514x_get_pdata(struct i2c_client *client)
>>>> +{
>>>> +       if (!client->dev.platform_data&&   client->dev.of_node) {
>>>>
>>>> +               struct tvp514x_platform_data *pdata;
>>>> +               struct v4l2_of_endpoint bus_cfg;
>>>> +               struct device_node *endpoint;
>>>> +
>>>> +               pdata = devm_kzalloc(&client->dev,
>>>> +                               sizeof(struct tvp514x_platform_data),
>>>> +                               GFP_KERNEL);
>>>> +               client->dev.platform_data = pdata;
>>>
>>>
>>>
>>> Do you really need to set client->dev.platform_data this way ?
>>> What about passing struct tvp514x_decoder pointer to this function
>>> and initializing struct tvp514x_decoder::pdata instead ?
>>>
>>>
>> Yes that can work too, I'll do the same.
>
>
> Ok, thanks.
>
>
>>>> +               if (!pdata)
>>>> +                       return NULL;
>>>> +
>>>> +               endpoint = of_get_child_by_name(client->dev.of_node,
>>>> "port");
>>>> +               if (endpoint)
>>>> +                       endpoint = of_get_child_by_name(endpoint,
>>>> "endpoint");
>>>
>>>
>>>
>>> I think you could replace these two calls above with
>>>
>>>                  endpoint =
>>> v4l2_of_get_next_endpoint(client->dev.of_node);
>>>
>> Ok
>>
>>> Now I see I should have modified this function to ensure it works also
>>> when
>>> 'port' nodes are grouped in a 'ports' node.
>>>
>> So V5 series of V4l OF parser doesn't have this fix ?
>
>
> No, it doesn't. I think we need something along the lines of:
>
> diff --git a/drivers/media/v4l2-core/v4l2-of.c
> b/drivers/media/v4l2-core/v4l2-of.c
> index e1f570b..8a286f1 100644
> --- a/drivers/media/v4l2-core/v4l2-of.c
> +++ b/drivers/media/v4l2-core/v4l2-of.c
> @@ -185,10 +185,15 @@ struct device_node *v4l2_of_get_next_endpoint(const
> struct device_node *parent,
>                  * This is the first call, we have to find a port within
>                  * this node.
>                  */
> -               for_each_child_of_node(parent, port) {
> +               while (port = of_get_next_child(parent, port)) {
>                         if (!of_node_cmp(port->name, "port"))
>                                 break;
> -               }
> +                       if (!of_node_cmp(port->name, "ports")) {
> +                               parent = port;
> +                               of_node_put(port);
> +                               port = NULL:
> +                       }
> +               };
>                 if (port) {
>                         /* Found a port, get an endpoint. */
>                         endpoint = of_get_next_child(port, NULL);
>
> However this shouldn't affect you, as you don't use the 'ports' node...
> I will likely post v6 including this fix tomorrow.
>
yes I don't need it, Just asked so that I can test my driver on latest
version :)

Regards,
--Prabhakar

> --
>
> Regards,
> Sylwester