* [PATCH] media: i2c: tvp514x: add OF support
@ 2013-04-26 11:23 Prabhakar Lad
2013-04-29 13:57 ` Laurent Pinchart
0 siblings, 1 reply; 3+ messages in thread
From: Prabhakar Lad @ 2013-04-26 11:23 UTC (permalink / raw)
To: LMML
Cc: Lad, Prabhakar, Hans Verkuil, Laurent Pinchart,
Mauro Carvalho Chehab, Guennadi Liakhovetski, Sylwester Nawrocki,
Sakari Ailus, Grant Likely, Rob Herring, Rob Landley,
devicetree-discuss, linux-doc, linux-kernel,
davinci-linux-open-source
From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
add OF support for the tvp514x 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
---
RFC v1: https://patchwork.kernel.org/patch/2030061/
RFC v2: https://patchwork.kernel.org/patch/2061811/
Changes for current version from RFC v2:
1: Fixed review comments pointed by Sylwester.
.../devicetree/bindings/media/i2c/tvp514x.txt | 38 +++++++++++
drivers/media/i2c/tvp514x.c | 67 +++++++++++++++++--
2 files changed, 98 insertions(+), 7 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..618640a
--- /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.
+- pclk-sample: 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) */
+ vsync-active = <1>;
+ /* Active low (Defaults to 0) */
+ pclk-sample = <0>;
+ };
+ };
+ };
+ ...
+};
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index 887bd93..d37b85e 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -35,7 +35,9 @@
#include <linux/videodev2.h>
#include <linux/module.h>
#include <linux/v4l2-mediabus.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>
@@ -1056,6 +1058,58 @@ static struct tvp514x_decoder tvp514x_dev = {
};
+#if defined(CONFIG_OF)
+static const struct of_device_id tvp514x_of_match[] = {
+ {.compatible = "ti,tvp5146-decoder", },
+ {.compatible = "ti,tvp5146m2-decoder", },
+ {.compatible = "ti,tvp5147-decoder", },
+ {.compatible = "ti,tvp5147m1-decoder", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, tvp514x_of_match);
+
+static void tvp514x_get_pdata(struct i2c_client *client,
+ struct tvp514x_decoder *decoder)
+{
+ if (!client->dev.platform_data && client->dev.of_node) {
+ struct device_node *endpoint;
+
+ endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
+ if (endpoint) {
+ struct tvp514x_platform_data *pdata;
+ struct v4l2_of_endpoint bus_cfg;
+ unsigned int flags;
+
+ pdata =
+ devm_kzalloc(&client->dev,
+ sizeof(struct tvp514x_platform_data),
+ GFP_KERNEL);
+ if (!pdata)
+ return;
+
+ 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;
+ decoder->pdata = pdata;
+ }
+ }
+}
+#else
+#define tvp514x_of_match NULL
+
+static void tvp514x_get_pdata(struct i2c_client *client,
+ struct tvp514x_decoder *decoder)
+{
+ decoder->pdata = client->dev.platform_data;
+}
+#endif
+
/**
* tvp514x_probe() - decoder driver i2c probe handler
* @client: i2c driver client device structure
@@ -1075,11 +1129,6 @@ 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;
- if (!client->dev.platform_data) {
- v4l2_err(client, "No platform data!!\n");
- return -ENODEV;
- }
-
decoder = devm_kzalloc(&client->dev, sizeof(*decoder), GFP_KERNEL);
if (!decoder)
return -ENOMEM;
@@ -1090,8 +1139,11 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
memcpy(decoder->tvp514x_regs, tvp514x_reg_list_default,
sizeof(tvp514x_reg_list_default));
- /* Copy board specific information here */
- decoder->pdata = client->dev.platform_data;
+ tvp514x_get_pdata(client, decoder);
+ if (!decoder->pdata) {
+ v4l2_err(client, "No platform data!!\n");
+ return -EPROBE_DEFER;
+ }
/**
* Fetch platform specific data, and configure the
@@ -1242,6 +1294,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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] media: i2c: tvp514x: add OF support
2013-04-26 11:23 [PATCH] media: i2c: tvp514x: add OF support Prabhakar Lad
@ 2013-04-29 13:57 ` Laurent Pinchart
2013-04-29 17:23 ` Prabhakar Lad
0 siblings, 1 reply; 3+ messages in thread
From: Laurent Pinchart @ 2013-04-29 13:57 UTC (permalink / raw)
To: Prabhakar Lad
Cc: LMML, Hans Verkuil, Mauro Carvalho Chehab, Guennadi Liakhovetski,
Sylwester Nawrocki, Sakari Ailus, Grant Likely, Rob Herring,
Rob Landley, devicetree-discuss, linux-doc, linux-kernel,
davinci-linux-open-source
Hi Prabhakar,
Thank you for the patch. Please see below for a couple of comments. Most of
them apply to your adv7343 patch as well.
On Friday 26 April 2013 16:53:50 Prabhakar Lad wrote:
> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>
> add OF support for the tvp514x 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
> ---
> RFC v1: https://patchwork.kernel.org/patch/2030061/
> RFC v2: https://patchwork.kernel.org/patch/2061811/
>
> Changes for current version from RFC v2:
> 1: Fixed review comments pointed by Sylwester.
>
> .../devicetree/bindings/media/i2c/tvp514x.txt | 38 +++++++++++
> drivers/media/i2c/tvp514x.c | 67 +++++++++++++++--
> 2 files changed, 98 insertions(+), 7 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..618640a
> --- /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"
According to the code below, it must be one of
"ti,tvp5146-decoder"
"ti,tvp5146m2-decoder"
"ti,tvp5147-decoder"
"ti,tvp5147m1-decoder"
Couldn't you remove "-decoder" ?
You should add a reference to the V4L2 DT bindings documentation to explain
what the port and endpoint nodes are for.
> +- hsync-active: HSYNC Polarity configuration for current interface.
> +- vsync-active: VSYNC Polarity configuration for current interface.
> +- pclk-sample: Clock polarity of the current interface.
s/current interface/endpoint/ ?
> +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) */
> + vsync-active = <1>;
> + /* Active low (Defaults to 0) */
> + pclk-sample = <0>;
> + };
> + };
> + };
> + ...
> +};
> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> index 887bd93..d37b85e 100644
> --- a/drivers/media/i2c/tvp514x.c
> +++ b/drivers/media/i2c/tvp514x.c
> @@ -35,7 +35,9 @@
> #include <linux/videodev2.h>
> #include <linux/module.h>
> #include <linux/v4l2-mediabus.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>
> @@ -1056,6 +1058,58 @@ static struct tvp514x_decoder tvp514x_dev = {
>
> };
>
> +#if defined(CONFIG_OF)
> +static const struct of_device_id tvp514x_of_match[] = {
> + {.compatible = "ti,tvp5146-decoder", },
> + {.compatible = "ti,tvp5146m2-decoder", },
> + {.compatible = "ti,tvp5147-decoder", },
> + {.compatible = "ti,tvp5147m1-decoder", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, tvp514x_of_match);
> +
> +static void tvp514x_get_pdata(struct i2c_client *client,
> + struct tvp514x_decoder *decoder)
> +{
> + if (!client->dev.platform_data && client->dev.of_node) {
> + struct device_node *endpoint;
> +
> + endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> + if (endpoint) {
> + struct tvp514x_platform_data *pdata;
> + struct v4l2_of_endpoint bus_cfg;
> + unsigned int flags;
> +
> + pdata =
> + devm_kzalloc(&client->dev,
> + sizeof(struct tvp514x_platform_data),
sizeof(*pdata) ? That's the preferred style, as it makes sure that you will
still allocate the right amount of memory if the type of the pdata variable
changes (that's pretty unlikely here of course, but the general principle
holds true).
> + GFP_KERNEL);
> + if (!pdata)
> + return;
> +
> + 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;
> + decoder->pdata = pdata;
> + }
> + }
As pointed out by Sascha in his review of your mt9p031 patch, you need to use
client->dev.platform_data if client->dev.of_node is NULL.
> +}
> +#else
> +#define tvp514x_of_match NULL
> +
> +static void tvp514x_get_pdata(struct i2c_client *client,
> + struct tvp514x_decoder *decoder)
> +{
> + decoder->pdata = client->dev.platform_data;
> +}
> +#endif
> +
> /**
> * tvp514x_probe() - decoder driver i2c probe handler
> * @client: i2c driver client device structure
> @@ -1075,11 +1129,6 @@ 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;
>
> - if (!client->dev.platform_data) {
> - v4l2_err(client, "No platform data!!\n");
> - return -ENODEV;
> - }
> -
> decoder = devm_kzalloc(&client->dev, sizeof(*decoder), GFP_KERNEL);
> if (!decoder)
> return -ENOMEM;
> @@ -1090,8 +1139,11 @@ tvp514x_probe(struct i2c_client *client, const struct
> i2c_device_id *id) memcpy(decoder->tvp514x_regs, tvp514x_reg_list_default,
> sizeof(tvp514x_reg_list_default));
>
> - /* Copy board specific information here */
> - decoder->pdata = client->dev.platform_data;
> + tvp514x_get_pdata(client, decoder);
Getter functions usually return a value. The code would (at least in my
opinion) be clearer if you modifieid tvp514x_get_pdata() to return a pointer
to the pdata instead of assigning it directly to decoder->pdata inside the
function.
> + if (!decoder->pdata) {
> + v4l2_err(client, "No platform data!!\n");
> + return -EPROBE_DEFER;
Why EPROBE_DEFER ? If there's no pdata now you won't magically get it later
:-)
> + }
>
> /**
> * Fetch platform specific data, and configure the
> @@ -1242,6 +1294,7 @@ MODULE_DEVICE_TABLE(i2c, tvp514x_id);
>
> static struct i2c_driver tvp514x_driver = {
> .driver = {
> + .of_match_table = tvp514x_of_match,
Please use of_match_ptr() instead of defining tvp514x_of_match as NULL above.
> .owner = THIS_MODULE,
> .name = TVP514X_MODULE_NAME,
> },
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] media: i2c: tvp514x: add OF support
2013-04-29 13:57 ` Laurent Pinchart
@ 2013-04-29 17:23 ` Prabhakar Lad
0 siblings, 0 replies; 3+ messages in thread
From: Prabhakar Lad @ 2013-04-29 17:23 UTC (permalink / raw)
To: Laurent Pinchart
Cc: LMML, Hans Verkuil, Mauro Carvalho Chehab, Guennadi Liakhovetski,
Sylwester Nawrocki, Sakari Ailus, Grant Likely, Rob Herring,
Rob Landley, devicetree-discuss, linux-doc, linux-kernel,
davinci-linux-open-source
Hi Laurent,
Thanks for the review.
On Mon, Apr 29, 2013 at 7:27 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Prabhakar,
>
> Thank you for the patch. Please see below for a couple of comments. Most of
> them apply to your adv7343 patch as well.
>
alright I'll fix for that patch as well.
> On Friday 26 April 2013 16:53:50 Prabhakar Lad wrote:
>> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>>
>> add OF support for the tvp514x 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
>> ---
>> RFC v1: https://patchwork.kernel.org/patch/2030061/
>> RFC v2: https://patchwork.kernel.org/patch/2061811/
>>
>> Changes for current version from RFC v2:
>> 1: Fixed review comments pointed by Sylwester.
>>
>> .../devicetree/bindings/media/i2c/tvp514x.txt | 38 +++++++++++
>> drivers/media/i2c/tvp514x.c | 67 +++++++++++++++--
>> 2 files changed, 98 insertions(+), 7 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..618640a
>> --- /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"
>
> According to the code below, it must be one of
>
> "ti,tvp5146-decoder"
> "ti,tvp5146m2-decoder"
> "ti,tvp5147-decoder"
> "ti,tvp5147m1-decoder"
>
> Couldn't you remove "-decoder" ?
>
yeah I missed to mention other compatible options, 'll remove
"-decoder" as well.
> You should add a reference to the V4L2 DT bindings documentation to explain
> what the port and endpoint nodes are for.
>
OK
>> +- hsync-active: HSYNC Polarity configuration for current interface.
>> +- vsync-active: VSYNC Polarity configuration for current interface.
>> +- pclk-sample: Clock polarity of the current interface.
>
> s/current interface/endpoint/ ?
>
yeah endpoint.
>> +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) */
>> + vsync-active = <1>;
>> + /* Active low (Defaults to 0) */
>> + pclk-sample = <0>;
>> + };
>> + };
>> + };
>> + ...
>> +};
>> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
>> index 887bd93..d37b85e 100644
>> --- a/drivers/media/i2c/tvp514x.c
>> +++ b/drivers/media/i2c/tvp514x.c
>> @@ -35,7 +35,9 @@
>> #include <linux/videodev2.h>
>> #include <linux/module.h>
>> #include <linux/v4l2-mediabus.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>
>> @@ -1056,6 +1058,58 @@ static struct tvp514x_decoder tvp514x_dev = {
>>
>> };
>>
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id tvp514x_of_match[] = {
>> + {.compatible = "ti,tvp5146-decoder", },
>> + {.compatible = "ti,tvp5146m2-decoder", },
>> + {.compatible = "ti,tvp5147-decoder", },
>> + {.compatible = "ti,tvp5147m1-decoder", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, tvp514x_of_match);
>> +
>> +static void tvp514x_get_pdata(struct i2c_client *client,
>> + struct tvp514x_decoder *decoder)
>> +{
>> + if (!client->dev.platform_data && client->dev.of_node) {
>> + struct device_node *endpoint;
>> +
>> + endpoint = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
>> + if (endpoint) {
>> + struct tvp514x_platform_data *pdata;
>> + struct v4l2_of_endpoint bus_cfg;
>> + unsigned int flags;
>> +
>> + pdata =
>> + devm_kzalloc(&client->dev,
>> + sizeof(struct tvp514x_platform_data),
>
> sizeof(*pdata) ? That's the preferred style, as it makes sure that you will
> still allocate the right amount of memory if the type of the pdata variable
> changes (that's pretty unlikely here of course, but the general principle
> holds true).
>
OK will fix it.
>> + GFP_KERNEL);
>> + if (!pdata)
>> + return;
>> +
>> + 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;
>> + decoder->pdata = pdata;
>> + }
>> + }
>
> As pointed out by Sascha in his review of your mt9p031 patch, you need to use
> client->dev.platform_data if client->dev.of_node is NULL.
>
OK
>> +}
>> +#else
>> +#define tvp514x_of_match NULL
>> +
>> +static void tvp514x_get_pdata(struct i2c_client *client,
>> + struct tvp514x_decoder *decoder)
>> +{
>> + decoder->pdata = client->dev.platform_data;
>> +}
>> +#endif
>> +
>> /**
>> * tvp514x_probe() - decoder driver i2c probe handler
>> * @client: i2c driver client device structure
>> @@ -1075,11 +1129,6 @@ 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;
>>
>> - if (!client->dev.platform_data) {
>> - v4l2_err(client, "No platform data!!\n");
>> - return -ENODEV;
>> - }
>> -
>> decoder = devm_kzalloc(&client->dev, sizeof(*decoder), GFP_KERNEL);
>> if (!decoder)
>> return -ENOMEM;
>> @@ -1090,8 +1139,11 @@ tvp514x_probe(struct i2c_client *client, const struct
>> i2c_device_id *id) memcpy(decoder->tvp514x_regs, tvp514x_reg_list_default,
>> sizeof(tvp514x_reg_list_default));
>>
>> - /* Copy board specific information here */
>> - decoder->pdata = client->dev.platform_data;
>> + tvp514x_get_pdata(client, decoder);
>
> Getter functions usually return a value. The code would (at least in my
> opinion) be clearer if you modifieid tvp514x_get_pdata() to return a pointer
> to the pdata instead of assigning it directly to decoder->pdata inside the
> function.
>
hmmm thats what my earlier RFC version did, Sylwester suggested this way.
I'll make it as per your suggestion.
>> + if (!decoder->pdata) {
>> + v4l2_err(client, "No platform data!!\n");
>> + return -EPROBE_DEFER;
>
> Why EPROBE_DEFER ? If there's no pdata now you won't magically get it later
> :-)
>
:-)
>> + }
>>
>> /**
>> * Fetch platform specific data, and configure the
>> @@ -1242,6 +1294,7 @@ MODULE_DEVICE_TABLE(i2c, tvp514x_id);
>>
>> static struct i2c_driver tvp514x_driver = {
>> .driver = {
>> + .of_match_table = tvp514x_of_match,
>
> Please use of_match_ptr() instead of defining tvp514x_of_match as NULL above.
>
OK.
Regards,
--Prabhakar Lad
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-04-29 17:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-26 11:23 [PATCH] media: i2c: tvp514x: add OF support Prabhakar Lad
2013-04-29 13:57 ` Laurent Pinchart
2013-04-29 17:23 ` Prabhakar Lad
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).