Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757802Ab3D2RXh (ORCPT ); Mon, 29 Apr 2013 13:23:37 -0400 Received: from mail-we0-f170.google.com ([74.125.82.170]:52139 "EHLO mail-we0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757302Ab3D2RXf (ORCPT ); Mon, 29 Apr 2013 13:23:35 -0400 MIME-Version: 1.0 In-Reply-To: <7684541.nmKBKd3EoN@avalon> References: <1366975430-31806-1-git-send-email-prabhakar.csengg@gmail.com> <7684541.nmKBKd3EoN@avalon> From: Prabhakar Lad Date: Mon, 29 Apr 2013 22:53:13 +0530 Message-ID: Subject: Re: [PATCH] media: i2c: tvp514x: add OF support To: Laurent Pinchart Cc: LMML , Hans Verkuil , Mauro Carvalho Chehab , Guennadi Liakhovetski , Sylwester Nawrocki , Sakari Ailus , Grant Likely , Rob Herring , Rob Landley , devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, davinci-linux-open-source@linux.davincidsp.com Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9056 Lines: 256 Hi Laurent, Thanks for the review. On Mon, Apr 29, 2013 at 7:27 PM, Laurent Pinchart 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 >> >> add OF support for the tvp514x driver. >> >> Signed-off-by: Lad, Prabhakar >> Cc: Hans Verkuil >> Cc: Laurent Pinchart >> Cc: Mauro Carvalho Chehab >> Cc: Guennadi Liakhovetski >> Cc: Sylwester Nawrocki >> Cc: Sakari Ailus >> Cc: Grant Likely >> Cc: Rob Herring >> Cc: Rob Landley >> 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 >> #include >> #include >> +#include >> >> +#include >> #include >> #include >> #include >> @@ -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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/