diff mbox

Linux-3.14-rc2: Order of serial node compatibles in DTS files.

Message ID 20140212052816.GA15434@pek-khao-d1.corp.ad.wrs.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Kevin Hao Feb. 12, 2014, 5:28 a.m. UTC 
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?


Thanks,
Kevin
> 
> Stephen Chivers,
> CSC Australia Pty. Ltd.
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Comments

Sebastian Hesselbarth Feb. 12, 2014, 8:30 a.m. UTC | #1 
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;
>   }
>
Kevin Hao Feb. 12, 2014, 10:31 a.m. UTC | #2 
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
Sebastian Hesselbarth Feb. 12, 2014, 11:26 a.m. UTC | #3 
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++;
	}
Kevin Hao Feb. 12, 2014, 11:32 a.m. UTC | #4 
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 mbox

Patch

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;
 }