Message ID | 510E65B3.10901@gmail.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1U1zbn-00049b-HA; Sun, 03 Feb 2013 14:27:47 +0100 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.75/mailfrontend-4) with esmtp id 1U1zbm-0002us-A6; Sun, 03 Feb 2013 14:27:47 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752343Ab3BCN11 (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Sun, 3 Feb 2013 08:27:27 -0500 Received: from mail-ea0-f174.google.com ([209.85.215.174]:53881 "EHLO mail-ea0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752060Ab3BCN10 (ORCPT <rfc822; linux-media@vger.kernel.org>); Sun, 3 Feb 2013 08:27:26 -0500 Received: by mail-ea0-f174.google.com with SMTP id 1so2438749eaa.33 for <multiple recipients>; Sun, 03 Feb 2013 05:27:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=sd12vXAQsXrE6+xHSMP63G9zygQlZdgkFJJl6CLyC8k=; b=A7rmEp21McMs0fi6aqVjVp4Ndi2pwTgKRWCgPnTw4x3H97bbh/yQHTqXV0+6D+H9MQ y4Rv6F0jsxPauYn8Qbr3LC6fHCGrRrG5EUr4zhbnIalsIRr37H0Pdao01mv7ZnY+PsaP rHRQRXjd6nAmMJVMhF+H8xH95H6DgYnhP9qhi5kXB0g9gCvMDw5HZfLzrxBtuCJBgIZ2 bwBpMFm+CnFjh9VFe86Gwx4u0YawkdfCacPboSm54+nJE0YR6YDADvxSdVelTmOGNF3K QL2wbS7V4iuqsR/niCQiPgCRONdkrLtyCNxp/f+8qBVcb/t0BHdRDsXwfkniL+uWM6P1 OFEQ== X-Received: by 10.14.202.197 with SMTP id d45mr14638577eeo.1.1359898044676; Sun, 03 Feb 2013 05:27:24 -0800 (PST) Received: from [192.168.43.71] (user-94-254-193-124.play-internet.pl. [94.254.193.124]) by mx.google.com with ESMTPS id t44sm22307371eeo.2.2013.02.03.05.27.18 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sun, 03 Feb 2013 05:27:23 -0800 (PST) Message-ID: <510E65B3.10901@gmail.com> Date: Sun, 03 Feb 2013 14:27:15 +0100 From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20120412 Thunderbird/11.0.1 MIME-Version: 1.0 To: Prabhakar Lad <prabhakar.csengg@gmail.com> CC: LMML <linux-media@vger.kernel.org>, DLOS <davinci-linux-open-source@linux.davincidsp.com>, "Lad, Prabhakar" <prabhakar.lad@ti.com>, Hans Verkuil <hans.verkuil@cisco.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Mauro Carvalho Chehab <mchehab@redhat.com>, Guennadi Liakhovetski <g.liakhovetski@gmx.de>, Sylwester Nawrocki <s.nawrocki@samsung.com>, Sakari Ailus <sakari.ailus@iki.fi>, Grant Likely <grant.likely@secretlab.ca>, Rob Herring <rob.herring@calxeda.com>, Rob Landley <rob@landley.net>, devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RFC v2] media: tvp514x: add OF support References: <1359464832-8875-1-git-send-email-prabhakar.lad@ti.com> <510C43A0.7090906@gmail.com> <CA+V-a8u6VADw_HfbBN4ESGUXTSMKfVyKZaEf1bhVGACof6qZ8A@mail.gmail.com> In-Reply-To: <CA+V-a8u6VADw_HfbBN4ESGUXTSMKfVyKZaEf1bhVGACof6qZ8A@mail.gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 5.6.1.2065439, Antispam-Engine: 2.7.2.376379, Antispam-Data: 2013.2.3.131818 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' FORGED_FROM_GMAIL 0.1, MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODY_SIZE_4000_4999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, DKIM_SIGNATURE 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CP_NOT_1 0, __CP_URI_IN_BODY 0, __CT 0, __CTE 0, __CT_TEXT_PLAIN 0, __FRAUD_WEBMAIL 0, __FRAUD_WEBMAIL_FROM 0, __FROM_GMAIL 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MOZILLA_MSGID 0, __MOZILLA_USER_AGENT 0, __MULTIPLE_RCPTS_CC_X2 0, __PHISH_SPEAR_STRUCTURE_1 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __URI_NO_WWW 0, __URI_NS , __USER_AGENT 0, __YOUTUBE_RCVD 0' |
Commit Message
Sylwester Nawrocki
Feb. 3, 2013, 1:27 p.m. UTC
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: 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Comments
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. */