[PATCH] media: i2c: tvp514x: add OF support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Apr 29 23:57:34 EST 2013
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 at gmail.com>
>
> add OF support for the tvp514x driver.
>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg at gmail.com>
> Cc: Hans Verkuil <hans.verkuil at cisco.com>
> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab at redhat.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski at gmx.de>
> Cc: Sylwester Nawrocki <s.nawrocki at samsung.com>
> Cc: Sakari Ailus <sakari.ailus at iki.fi>
> Cc: Grant Likely <grant.likely at secretlab.ca>
> Cc: Rob Herring <rob.herring at calxeda.com>
> Cc: Rob Landley <rob at landley.net>
> Cc: devicetree-discuss at lists.ozlabs.org
> Cc: linux-doc at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: davinci-linux-open-source at 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 at 1c22000 {
> + ...
> + ...
> +
> + tvp514x at 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
More information about the devicetree-discuss
mailing list