Hi Laurent, Thanks for the review. On Mon, Apr 29, 2013 at 7:27 PM, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxx> >> >> add OF support for the tvp514x driver. >> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> >> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx> >> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> >> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> >> Cc: Guennadi Liakhovetski <g.liakhovetski@xxxxxx> >> Cc: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx> >> Cc: Sakari Ailus <sakari.ailus@xxxxxx> >> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx> >> Cc: Rob Herring <rob.herring@xxxxxxxxxxx> >> Cc: Rob Landley <rob@xxxxxxxxxxx> >> Cc: devicetree-discuss@xxxxxxxxxxxxxxxx >> Cc: linux-doc@xxxxxxxxxxxxxxx >> Cc: linux-kernel@xxxxxxxxxxxxxxx >> Cc: davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
