From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ee0-x22f.google.com (mail-ee0-x22f.google.com [IPv6:2a00:1450:4013:c00::22f]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 5BD162C00B5 for ; Wed, 12 Feb 2014 22:26:25 +1100 (EST) Received: by mail-ee0-f47.google.com with SMTP id d49so4230514eek.6 for ; Wed, 12 Feb 2014 03:26:19 -0800 (PST) Message-ID: <52FB5A56.2050402@gmail.com> Date: Wed, 12 Feb 2014 12:26:14 +0100 From: Sebastian Hesselbarth To: Kevin Hao Subject: Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files. References: <20140211072606.GA26514@visitor2.iram.es> <63AEBD99-AA87-4FD7-BBDA-0CE419959F14@kernel.crashing.org> <52FAA97F.4060600@gmail.com> <1392162080.6733.404.camel@snotra.buserror.net> <52FAB65C.4090201@gmail.com> <20140212052816.GA15434@pek-khao-d1.corp.ad.wrs.com> <52FB3108.3000202@gmail.com> <20140212103153.GC15434@pek-khao-d1.corp.ad.wrs.com> In-Reply-To: <20140212103153.GC15434@pek-khao-d1.corp.ad.wrs.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Cc: Chris Proctor , Arnd Bergmann , devicetree , Stephen N Chivers , Scott Wood , linuxppc-dev@lists.ozlabs.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 02/12/14 11:31, Kevin Hao wrote: > On Wed, Feb 12, 2014 at 09:30:00AM +0100, Sebastian Hesselbarth wrote: >> On 02/12/2014 06:28 AM, Kevin Hao wrote: >>> On Wed, Feb 12, 2014 at 10:21:58AM +1000, Stephen N Chivers wrote: >>>> But, the Interrupt Controller (MPIC) >>>> goes AWOL and it is down hill from there. >>>> >>>> The MPIC is specified in the DTS as: >>>> >>>> mpic: pic@40000 { >>>> interrupt-controller; >>>> #address-cells = <0>; >>>> #interrupt-cells = <2>; >>>> reg = <0x40000 0x40000>; >>>> compatible = "chrp,open-pic"; >>>> device_type = "open-pic"; >>>> big-endian; >>>> }; >>>> >>>> The board support file has the standard mechanism for allocating >>>> the PIC: >>>> >>>> struct mpic *mpic; >>>> >>>> mpic = mpic_alloc(NULL, 0, 0, 0, 256, " OpenPIC "); >>>> BUG_ON(mpic == NULL); >>>> >>>> mpic_init(mpic); >>>> >>>> I checked for damage in applying the patch and it has applied >>>> correctly. >>> >>> How about the following fix? >>> >>> diff --git a/drivers/of/base.c b/drivers/of/base.c >>> index ff85450d5683..ca91984d3c4b 100644 >>> --- a/drivers/of/base.c >>> +++ b/drivers/of/base.c >>> @@ -730,32 +730,40 @@ out: >>> } >>> EXPORT_SYMBOL(of_find_node_with_property); >>> >>> +static int of_match_type_name(const struct device_node *node, >>> + const struct of_device_id *m) >> >> I am fine with having a sub-function here, but it should rather be >> named of_match_type_or_name. > > OK. > >> >>> +{ >>> + int match = 1; >>> + >>> + if (m->name[0]) >>> + match &= node->name && !strcmp(m->name, node->name); >>> + >>> + if (m->type[0]) >>> + match &= node->type && !strcmp(m->type, node->type); >>> + >>> + return match; >>> +} >> [...] >>> + /* Check against matches without compatible string */ >>> + m = matches; >>> + while (!m->compatible[0] && (m->name[0] || m->type[0])) { >> >> We shouldn't check for anything else than the sentinel here. >> Although I guess yours will not quit early as mine did but that >> way we don't have to worry about it. > > Yes, this is still buggy. I will change something like this: > > m = matches; > /* Check against matches without compatible string */ > while (m->name[0] || m->type[0] || m->compatible[0]) { > if (m->compatible[0]) { > m++; > continue; > } > > match = of_match_type_name(node, m); > if (match) > return m; > m++; > } You can cook it down to: m = matches; /* Check against matches without compatible string */ while (m->name[0] || m->type[0] || m->compatible[0]) { if (!m->compatible[0] && of_match_type_or_name(node, m) return m; m++; } From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sebastian Hesselbarth Subject: Re: Linux-3.14-rc2: Order of serial node compatibles in DTS files. Date: Wed, 12 Feb 2014 12:26:14 +0100 Message-ID: <52FB5A56.2050402@gmail.com> References: <20140211072606.GA26514@visitor2.iram.es> <63AEBD99-AA87-4FD7-BBDA-0CE419959F14@kernel.crashing.org> <52FAA97F.4060600@gmail.com> <1392162080.6733.404.camel@snotra.buserror.net> <52FAB65C.4090201@gmail.com> <20140212052816.GA15434@pek-khao-d1.corp.ad.wrs.com> <52FB3108.3000202@gmail.com> <20140212103153.GC15434@pek-khao-d1.corp.ad.wrs.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140212103153.GC15434-2Y/eJ/Q4tBKYoDLmhA0N63T7OC0tnCxdQQ4Iyu8u01E@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Kevin Hao Cc: Stephen N Chivers , Chris Proctor , Arnd Bergmann , devicetree , Scott Wood , linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org List-Id: devicetree@vger.kernel.org On 02/12/14 11:31, Kevin Hao wrote: > On Wed, Feb 12, 2014 at 09:30:00AM +0100, Sebastian Hesselbarth wrote: >> On 02/12/2014 06:28 AM, Kevin Hao wrote: >>> On Wed, Feb 12, 2014 at 10:21:58AM +1000, Stephen N Chivers wrote: >>>> But, the Interrupt Controller (MPIC) >>>> goes AWOL and it is down hill from there. >>>> >>>> The MPIC is specified in the DTS as: >>>> >>>> mpic: pic@40000 { >>>> interrupt-controller; >>>> #address-cells = <0>; >>>> #interrupt-cells = <2>; >>>> reg = <0x40000 0x40000>; >>>> compatible = "chrp,open-pic"; >>>> device_type = "open-pic"; >>>> big-endian; >>>> }; >>>> >>>> The board support file has the standard mechanism for allocating >>>> the PIC: >>>> >>>> struct mpic *mpic; >>>> >>>> mpic = mpic_alloc(NULL, 0, 0, 0, 256, " OpenPIC "); >>>> BUG_ON(mpic == NULL); >>>> >>>> mpic_init(mpic); >>>> >>>> I checked for damage in applying the patch and it has applied >>>> correctly. >>> >>> How about the following fix? >>> >>> diff --git a/drivers/of/base.c b/drivers/of/base.c >>> index ff85450d5683..ca91984d3c4b 100644 >>> --- a/drivers/of/base.c >>> +++ b/drivers/of/base.c >>> @@ -730,32 +730,40 @@ out: >>> } >>> EXPORT_SYMBOL(of_find_node_with_property); >>> >>> +static int of_match_type_name(const struct device_node *node, >>> + const struct of_device_id *m) >> >> I am fine with having a sub-function here, but it should rather be >> named of_match_type_or_name. > > OK. > >> >>> +{ >>> + int match = 1; >>> + >>> + if (m->name[0]) >>> + match &= node->name && !strcmp(m->name, node->name); >>> + >>> + if (m->type[0]) >>> + match &= node->type && !strcmp(m->type, node->type); >>> + >>> + return match; >>> +} >> [...] >>> + /* Check against matches without compatible string */ >>> + m = matches; >>> + while (!m->compatible[0] && (m->name[0] || m->type[0])) { >> >> We shouldn't check for anything else than the sentinel here. >> Although I guess yours will not quit early as mine did but that >> way we don't have to worry about it. > > Yes, this is still buggy. I will change something like this: > > m = matches; > /* Check against matches without compatible string */ > while (m->name[0] || m->type[0] || m->compatible[0]) { > if (m->compatible[0]) { > m++; > continue; > } > > match = of_match_type_name(node, m); > if (match) > return m; > m++; > } You can cook it down to: m = matches; /* Check against matches without compatible string */ while (m->name[0] || m->type[0] || m->compatible[0]) { if (!m->compatible[0] && of_match_type_or_name(node, m) return m; m++; } -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html