Message ID | 20140212052816.GA15434@pek-khao-d1.corp.ad.wrs.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
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. > +{ > + 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. Sebastian > + match = of_match_type_name(node, m); > + if (match) > + return m; > + m++; > + } > + > return NULL; > } >
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++; } Thanks, Kevin
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++; }
On Wed, Feb 12, 2014 at 12:26:14PM +0100, Sebastian Hesselbarth wrote: > 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++; > } Will do. Thanks, Kevin
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) +{ + 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; +} + static const struct of_device_id *__of_match_node(const struct of_device_id *matches, const struct device_node *node) { const char *cp; int cplen, l; + const struct of_device_id *m; + int match; if (!matches) return NULL; cp = __of_get_property(node, "compatible", &cplen); do { - const struct of_device_id *m = matches; + m = matches; /* Check against matches with current compatible string */ - while (m->name[0] || m->type[0] || m->compatible[0]) { - 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); - if (m->compatible[0]) - match &= cp - && !of_compat_cmp(m->compatible, cp, + while (m->compatible[0]) { + match = of_match_type_name(node, m); + match &= cp && !of_compat_cmp(m->compatible, cp, strlen(m->compatible)); if (match) return m; @@ -770,6 +778,15 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches, } } while (cp && (cplen > 0)); + /* Check against matches without compatible string */ + m = matches; + while (!m->compatible[0] && (m->name[0] || m->type[0])) { + match = of_match_type_name(node, m); + if (match) + return m; + m++; + } + return NULL; }