[PATCH] of: match the compatible in the order set by the dts file

[PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

If we set the uart compatible in the dts file like this:
   ------------------------------------------------------
    compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
   ------------------------------------------------------

and we set the uart compatible in the uart driver like this:
   ------------------------------------------------------
	{ .compatible = "fsl,imx1-uart", ... },
	{ .compatible = "fsl,imx21-uart", ... },
	{ .compatible = "fsl,imx6q-uart", ... },
	{ /* sentinel */ }
   ------------------------------------------------------

the current code will match the "fsl,imx21-uart" in the end.

Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".

This patch rewrites the match code, and make it to check the compatible
in the order set by the DTS file.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/of/base.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0d61fc5..b13846b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -622,10 +622,14 @@ static
 const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 					   const struct device_node *node)
 {
+	struct of_device_id *old = (struct of_device_id *)matches;
+	const char *cp;
+	int cplen, l;
+
 	if (!matches)
 		return NULL;
 
-	while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
+	while (matches->name[0] || matches->type[0]) {
 		int match = 1;
 		if (matches->name[0])
 			match &= node->name
@@ -633,13 +637,30 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 		if (matches->type[0])
 			match &= node->type
 				&& !strcmp(matches->type, node->type);
-		if (matches->compatible[0])
-			match &= __of_device_is_compatible(node,
-							   matches->compatible);
 		if (match)
 			return matches;
 		matches++;
 	}
+
+	/* Check the compatible in the order set by the DTS file. */
+	cp = __of_get_property(node, "compatible", &cplen);
+	if (!cp)
+		return NULL;
+
+	while (cplen > 0) {
+		matches = old;
+
+		while (matches->compatible[0]) {
+			if (!of_compat_cmp(cp, matches->compatible,
+						strlen(matches->compatible)))
+				return matches;
+			matches++;
+		}
+
+		l = strlen(cp) + 1;
+		cp += l;
+		cplen -= l;
+	}
 	return NULL;
 }
 
-- 
1.7.1

[PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

If we set the uart compatible in the dts file like this:
   ------------------------------------------------------
    compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
   ------------------------------------------------------

and we set the uart compatible in the uart driver like this:
   ------------------------------------------------------
	{ .compatible = "fsl,imx1-uart", ... },
	{ .compatible = "fsl,imx21-uart", ... },
	{ .compatible = "fsl,imx6q-uart", ... },
	{ /* sentinel */ }
   ------------------------------------------------------

the current code will match the "fsl,imx21-uart" in the end.

Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".

This patch rewrites the match code, and make it to check the compatible
in the order set by the DTS file.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/of/base.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0d61fc5..b13846b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -622,10 +622,14 @@ static
 const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 					   const struct device_node *node)
 {
+	struct of_device_id *old = (struct of_device_id *)matches;
+	const char *cp;
+	int cplen, l;
+
 	if (!matches)
 		return NULL;
 
-	while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
+	while (matches->name[0] || matches->type[0]) {
 		int match = 1;
 		if (matches->name[0])
 			match &= node->name
@@ -633,13 +637,30 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 		if (matches->type[0])
 			match &= node->type
 				&& !strcmp(matches->type, node->type);
-		if (matches->compatible[0])
-			match &= __of_device_is_compatible(node,
-							   matches->compatible);
 		if (match)
 			return matches;
 		matches++;
 	}
+
+	/* Check the compatible in the order set by the DTS file. */
+	cp = __of_get_property(node, "compatible", &cplen);
+	if (!cp)
+		return NULL;
+
+	while (cplen > 0) {
+		matches = old;
+
+		while (matches->compatible[0]) {
+			if (!of_compat_cmp(cp, matches->compatible,
+						strlen(matches->compatible)))
+				return matches;
+			matches++;
+		}
+
+		l = strlen(cp) + 1;
+		cp += l;
+		cplen -= l;
+	}
 	return NULL;
 }
 
-- 
1.7.1

[PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

If we set the uart compatible in the dts file like this:
   ------------------------------------------------------
    compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
   ------------------------------------------------------

and we set the uart compatible in the uart driver like this:
   ------------------------------------------------------
	{ .compatible = "fsl,imx1-uart", ... },
	{ .compatible = "fsl,imx21-uart", ... },
	{ .compatible = "fsl,imx6q-uart", ... },
	{ /* sentinel */ }
   ------------------------------------------------------

the current code will match the "fsl,imx21-uart" in the end.

Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".

This patch rewrites the match code, and make it to check the compatible
in the order set by the DTS file.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/of/base.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0d61fc5..b13846b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -622,10 +622,14 @@ static
 const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 					   const struct device_node *node)
 {
+	struct of_device_id *old = (struct of_device_id *)matches;
+	const char *cp;
+	int cplen, l;
+
 	if (!matches)
 		return NULL;
 
-	while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
+	while (matches->name[0] || matches->type[0]) {
 		int match = 1;
 		if (matches->name[0])
 			match &= node->name
@@ -633,13 +637,30 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
 		if (matches->type[0])
 			match &= node->type
 				&& !strcmp(matches->type, node->type);
-		if (matches->compatible[0])
-			match &= __of_device_is_compatible(node,
-							   matches->compatible);
 		if (match)
 			return matches;
 		matches++;
 	}
+
+	/* Check the compatible in the order set by the DTS file. */
+	cp = __of_get_property(node, "compatible", &cplen);
+	if (!cp)
+		return NULL;
+
+	while (cplen > 0) {
+		matches = old;
+
+		while (matches->compatible[0]) {
+			if (!of_compat_cmp(cp, matches->compatible,
+						strlen(matches->compatible)))
+				return matches;
+			matches++;
+		}
+
+		l = strlen(cp) + 1;
+		cp += l;
+		cplen -= l;
+	}
 	return NULL;
 }
 
-- 
1.7.1

Re: [PATCH] of: match the compatible in the order set by the dts file

[Sascha Hauer]

On Fri, Jul 05, 2013 at 04:43:38PM +0800, Huang Shijie wrote:
> If we set the uart compatible in the dts file like this:
>    ------------------------------------------------------
>     compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>    ------------------------------------------------------
> 
> and we set the uart compatible in the uart driver like this:
>    ------------------------------------------------------
> 	{ .compatible = "fsl,imx1-uart", ... },
> 	{ .compatible = "fsl,imx21-uart", ... },
> 	{ .compatible = "fsl,imx6q-uart", ... },
> 	{ /* sentinel */ }
>    ------------------------------------------------------
> 
> the current code will match the "fsl,imx21-uart" in the end.
> 
> Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".
> 
> This patch rewrites the match code, and make it to check the compatible
> in the order set by the DTS file.

Why don't you set the matching order in the driver the way you want it
to be, i.e.:

	{ .compatible = "fsl,imx6q-uart", ... },
	{ .compatible = "fsl,imx21-uart", ... },
	{ .compatible = "fsl,imx1-uart", ... },

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] of: match the compatible in the order set by the dts file

[Sascha Hauer]

On Fri, Jul 05, 2013 at 04:43:38PM +0800, Huang Shijie wrote:
> If we set the uart compatible in the dts file like this:
>    ------------------------------------------------------
>     compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>    ------------------------------------------------------
> 
> and we set the uart compatible in the uart driver like this:
>    ------------------------------------------------------
> 	{ .compatible = "fsl,imx1-uart", ... },
> 	{ .compatible = "fsl,imx21-uart", ... },
> 	{ .compatible = "fsl,imx6q-uart", ... },
> 	{ /* sentinel */ }
>    ------------------------------------------------------
> 
> the current code will match the "fsl,imx21-uart" in the end.
> 
> Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".
> 
> This patch rewrites the match code, and make it to check the compatible
> in the order set by the DTS file.

Why don't you set the matching order in the driver the way you want it
to be, i.e.:

	{ .compatible = "fsl,imx6q-uart", ... },
	{ .compatible = "fsl,imx21-uart", ... },
	{ .compatible = "fsl,imx1-uart", ... },

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

于 2013年07月09日 15:05, Sascha Hauer 写道:
> Why don't you set the matching order in the driver the way you want it
> to be, i.e.:
>
> 	{ .compatible = "fsl,imx6q-uart", ... },
> 	{ .compatible = "fsl,imx21-uart", ... },
> 	{ .compatible = "fsl,imx1-uart", ... },
>
yes. i can set it like this.

but this method looks like a ugly workaround.

thanks
Huang Shijie

Re: [PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

于 2013年07月09日 15:05, Sascha Hauer 写道:
> Why don't you set the matching order in the driver the way you want it
> to be, i.e.:
>
> 	{ .compatible = "fsl,imx6q-uart", ... },
> 	{ .compatible = "fsl,imx21-uart", ... },
> 	{ .compatible = "fsl,imx1-uart", ... },
>
yes. i can set it like this.

but this method looks like a ugly workaround.

thanks
Huang Shijie

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

? 2013?07?09? 15:05, Sascha Hauer ??:
> Why don't you set the matching order in the driver the way you want it
> to be, i.e.:
>
> 	{ .compatible = "fsl,imx6q-uart", ... },
> 	{ .compatible = "fsl,imx21-uart", ... },
> 	{ .compatible = "fsl,imx1-uart", ... },
>
yes. i can set it like this.

but this method looks like a ugly workaround.

thanks
Huang Shijie

Re: [PATCH] of: match the compatible in the order set by the dts file

[Sascha Hauer]

On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> 于 2013年07月09日 15:05, Sascha Hauer 写道:
> >Why don't you set the matching order in the driver the way you want it
> >to be, i.e.:
> >
> >	{ .compatible = "fsl,imx6q-uart", ... },
> >	{ .compatible = "fsl,imx21-uart", ... },
> >	{ .compatible = "fsl,imx1-uart", ... },
> >
> yes. i can set it like this.
> 
> but this method looks like a ugly workaround.

If a driver has different ways of supporting a single device, then
putting the preferred or most feature rich on top doesn't look very ugly
to me.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] of: match the compatible in the order set by the dts file

[Sascha Hauer]

On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> ? 2013?07?09? 15:05, Sascha Hauer ??:
> >Why don't you set the matching order in the driver the way you want it
> >to be, i.e.:
> >
> >	{ .compatible = "fsl,imx6q-uart", ... },
> >	{ .compatible = "fsl,imx21-uart", ... },
> >	{ .compatible = "fsl,imx1-uart", ... },
> >
> yes. i can set it like this.
> 
> but this method looks like a ugly workaround.

If a driver has different ways of supporting a single device, then
putting the preferred or most feature rich on top doesn't look very ugly
to me.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

于 2013年07月09日 15:51, Sascha Hauer 写道:
> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>>> Why don't you set the matching order in the driver the way you want it
>>> to be, i.e.:
>>>
>>> 	{ .compatible = "fsl,imx6q-uart", ... },
>>> 	{ .compatible = "fsl,imx21-uart", ... },
>>> 	{ .compatible = "fsl,imx1-uart", ... },
>>>
>> yes. i can set it like this.
>>
>> but this method looks like a ugly workaround.
> If a driver has different ways of supporting a single device, then
> putting the preferred or most feature rich on top doesn't look very ugly
> to me.
this method makes it much _coupled_ between the driver and the dts file.

IMHO, it's an unnecessary _burden_ to the driver programmer:
    he should puts the most feature compatible on the top.

it's much graceful if we let the driver programmer be transparent about 
this.

thanks
Huang Shijie

Re: [PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

于 2013年07月09日 15:51, Sascha Hauer 写道:
> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>>> Why don't you set the matching order in the driver the way you want it
>>> to be, i.e.:
>>>
>>> 	{ .compatible = "fsl,imx6q-uart", ... },
>>> 	{ .compatible = "fsl,imx21-uart", ... },
>>> 	{ .compatible = "fsl,imx1-uart", ... },
>>>
>> yes. i can set it like this.
>>
>> but this method looks like a ugly workaround.
> If a driver has different ways of supporting a single device, then
> putting the preferred or most feature rich on top doesn't look very ugly
> to me.
this method makes it much _coupled_ between the driver and the dts file.

IMHO, it's an unnecessary _burden_ to the driver programmer:
    he should puts the most feature compatible on the top.

it's much graceful if we let the driver programmer be transparent about 
this.

thanks
Huang Shijie

[PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

? 2013?07?09? 15:51, Sascha Hauer ??:
> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>> ? 2013?07?09? 15:05, Sascha Hauer ??:
>>> Why don't you set the matching order in the driver the way you want it
>>> to be, i.e.:
>>>
>>> 	{ .compatible = "fsl,imx6q-uart", ... },
>>> 	{ .compatible = "fsl,imx21-uart", ... },
>>> 	{ .compatible = "fsl,imx1-uart", ... },
>>>
>> yes. i can set it like this.
>>
>> but this method looks like a ugly workaround.
> If a driver has different ways of supporting a single device, then
> putting the preferred or most feature rich on top doesn't look very ugly
> to me.
this method makes it much _coupled_ between the driver and the dts file.

IMHO, it's an unnecessary _burden_ to the driver programmer:
    he should puts the most feature compatible on the top.

it's much graceful if we let the driver programmer be transparent about 
this.

thanks
Huang Shijie

Re: [PATCH] of: match the compatible in the order set by the dts file

[Rob Herring]

On Tue, Jul 9, 2013 at 3:10 AM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年07月09日 15:51, Sascha Hauer 写道:
>
>> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>>>
>>> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>>>>
>>>> Why don't you set the matching order in the driver the way you want it
>>>> to be, i.e.:
>>>>
>>>>         { .compatible = "fsl,imx6q-uart", ... },
>>>>         { .compatible = "fsl,imx21-uart", ... },
>>>>         { .compatible = "fsl,imx1-uart", ... },
>>>>
>>> yes. i can set it like this.
>>>
>>> but this method looks like a ugly workaround.
>>
>> If a driver has different ways of supporting a single device, then
>> putting the preferred or most feature rich on top doesn't look very ugly
>> to me.
>
> this method makes it much _coupled_ between the driver and the dts file.
>
> IMHO, it's an unnecessary _burden_ to the driver programmer:
>    he should puts the most feature compatible on the top.
>
> it's much graceful if we let the driver programmer be transparent about
> this.

The dts requires compatible strings to be most specific to least
specific. There is no reason that driver match tables should not be
the same and that is the assumption. Matching is not just based on
compatible properties and your patch does not handle the other cases.

Rob



>
>
> thanks
> Huang Shijie
>
>
>
>
>
>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] of: match the compatible in the order set by the dts file

[Rob Herring]

On Tue, Jul 9, 2013 at 3:10 AM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年07月09日 15:51, Sascha Hauer 写道:
>
>> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>>>
>>> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>>>>
>>>> Why don't you set the matching order in the driver the way you want it
>>>> to be, i.e.:
>>>>
>>>>         { .compatible = "fsl,imx6q-uart", ... },
>>>>         { .compatible = "fsl,imx21-uart", ... },
>>>>         { .compatible = "fsl,imx1-uart", ... },
>>>>
>>> yes. i can set it like this.
>>>
>>> but this method looks like a ugly workaround.
>>
>> If a driver has different ways of supporting a single device, then
>> putting the preferred or most feature rich on top doesn't look very ugly
>> to me.
>
> this method makes it much _coupled_ between the driver and the dts file.
>
> IMHO, it's an unnecessary _burden_ to the driver programmer:
>    he should puts the most feature compatible on the top.
>
> it's much graceful if we let the driver programmer be transparent about
> this.

The dts requires compatible strings to be most specific to least
specific. There is no reason that driver match tables should not be
the same and that is the assumption. Matching is not just based on
compatible properties and your patch does not handle the other cases.

Rob



>
>
> thanks
> Huang Shijie
>
>
>
>
>
>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH] of: match the compatible in the order set by the dts file

[Rob Herring]

On Tue, Jul 9, 2013 at 3:10 AM, Huang Shijie <b32955@freescale.com> wrote:
> ? 2013?07?09? 15:51, Sascha Hauer ??:
>
>> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>>>
>>> ? 2013?07?09? 15:05, Sascha Hauer ??:
>>>>
>>>> Why don't you set the matching order in the driver the way you want it
>>>> to be, i.e.:
>>>>
>>>>         { .compatible = "fsl,imx6q-uart", ... },
>>>>         { .compatible = "fsl,imx21-uart", ... },
>>>>         { .compatible = "fsl,imx1-uart", ... },
>>>>
>>> yes. i can set it like this.
>>>
>>> but this method looks like a ugly workaround.
>>
>> If a driver has different ways of supporting a single device, then
>> putting the preferred or most feature rich on top doesn't look very ugly
>> to me.
>
> this method makes it much _coupled_ between the driver and the dts file.
>
> IMHO, it's an unnecessary _burden_ to the driver programmer:
>    he should puts the most feature compatible on the top.
>
> it's much graceful if we let the driver programmer be transparent about
> this.

The dts requires compatible strings to be most specific to least
specific. There is no reason that driver match tables should not be
the same and that is the assumption. Matching is not just based on
compatible properties and your patch does not handle the other cases.

Rob



>
>
> thanks
> Huang Shijie
>
>
>
>
>
>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] of: match the compatible in the order set by the dts file

[Stephen Warren]

On 07/09/2013 01:05 AM, Sascha Hauer wrote:
> On Fri, Jul 05, 2013 at 04:43:38PM +0800, Huang Shijie wrote:
>> If we set the uart compatible in the dts file like this:
>>    ------------------------------------------------------
>>     compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>>    ------------------------------------------------------
>>
>> and we set the uart compatible in the uart driver like this:
>>    ------------------------------------------------------
>> 	{ .compatible = "fsl,imx1-uart", ... },
>> 	{ .compatible = "fsl,imx21-uart", ... },
>> 	{ .compatible = "fsl,imx6q-uart", ... },
>> 	{ /* sentinel */ }
>>    ------------------------------------------------------
>>
>> the current code will match the "fsl,imx21-uart" in the end.
>>
>> Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".
>>
>> This patch rewrites the match code, and make it to check the compatible
>> in the order set by the DTS file.
> 
> Why don't you set the matching order in the driver the way you want it
> to be, i.e.:
> 
> 	{ .compatible = "fsl,imx6q-uart", ... },
> 	{ .compatible = "fsl,imx21-uart", ... },
> 	{ .compatible = "fsl,imx1-uart", ... },

DT semantics are that earlier entries in the compatible property are
supposed to be matched first, irrespective of how the driver is constructed.

[PATCH] of: match the compatible in the order set by the dts file

[Stephen Warren]

On 07/09/2013 01:05 AM, Sascha Hauer wrote:
> On Fri, Jul 05, 2013 at 04:43:38PM +0800, Huang Shijie wrote:
>> If we set the uart compatible in the dts file like this:
>>    ------------------------------------------------------
>>     compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>>    ------------------------------------------------------
>>
>> and we set the uart compatible in the uart driver like this:
>>    ------------------------------------------------------
>> 	{ .compatible = "fsl,imx1-uart", ... },
>> 	{ .compatible = "fsl,imx21-uart", ... },
>> 	{ .compatible = "fsl,imx6q-uart", ... },
>> 	{ /* sentinel */ }
>>    ------------------------------------------------------
>>
>> the current code will match the "fsl,imx21-uart" in the end.
>>
>> Of course, this is not what we want. We want it to match the "fsl,imx6q-uart".
>>
>> This patch rewrites the match code, and make it to check the compatible
>> in the order set by the DTS file.
> 
> Why don't you set the matching order in the driver the way you want it
> to be, i.e.:
> 
> 	{ .compatible = "fsl,imx6q-uart", ... },
> 	{ .compatible = "fsl,imx21-uart", ... },
> 	{ .compatible = "fsl,imx1-uart", ... },

DT semantics are that earlier entries in the compatible property are
supposed to be matched first, irrespective of how the driver is constructed.

Re: [PATCH] of: match the compatible in the order set by the dts file

[Stephen Warren]

On 07/09/2013 06:03 AM, Rob Herring wrote:
> On Tue, Jul 9, 2013 at 3:10 AM, Huang Shijie <b32955@freescale.com> wrote:
>> 于 2013年07月09日 15:51, Sascha Hauer 写道:
>>
>>> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>>>>
>>>> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>>>>>
>>>>> Why don't you set the matching order in the driver the way you want it
>>>>> to be, i.e.:
>>>>>
>>>>>         { .compatible = "fsl,imx6q-uart", ... },
>>>>>         { .compatible = "fsl,imx21-uart", ... },
>>>>>         { .compatible = "fsl,imx1-uart", ... },
>>>>>
>>>> yes. i can set it like this.
>>>>
>>>> but this method looks like a ugly workaround.
>>>
>>> If a driver has different ways of supporting a single device, then
>>> putting the preferred or most feature rich on top doesn't look very ugly
>>> to me.
>>
>> this method makes it much _coupled_ between the driver and the dts file.
>>
>> IMHO, it's an unnecessary _burden_ to the driver programmer:
>>    he should puts the most feature compatible on the top.
>>
>> it's much graceful if we let the driver programmer be transparent about
>> this.
> 
> The dts requires compatible strings to be most specific to least
> specific. There is no reason that driver match tables should not be
> the same and that is the assumption. Matching is not just based on
> compatible properties and your patch does not handle the other cases.

Well, that may be true, but the only way to guarantee that the DT
compatible property is matched correctly is to match it in the order
it's written. Forcing driver writers to write the of_match table in a
particular order is quite a hack, and doesn't guarantee the correct
match order in all cases, only typical cases.

Re: [PATCH] of: match the compatible in the order set by the dts file

[Stephen Warren]

On 07/09/2013 06:03 AM, Rob Herring wrote:
> On Tue, Jul 9, 2013 at 3:10 AM, Huang Shijie <b32955@freescale.com> wrote:
>> 于 2013年07月09日 15:51, Sascha Hauer 写道:
>>
>>> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>>>>
>>>> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>>>>>
>>>>> Why don't you set the matching order in the driver the way you want it
>>>>> to be, i.e.:
>>>>>
>>>>>         { .compatible = "fsl,imx6q-uart", ... },
>>>>>         { .compatible = "fsl,imx21-uart", ... },
>>>>>         { .compatible = "fsl,imx1-uart", ... },
>>>>>
>>>> yes. i can set it like this.
>>>>
>>>> but this method looks like a ugly workaround.
>>>
>>> If a driver has different ways of supporting a single device, then
>>> putting the preferred or most feature rich on top doesn't look very ugly
>>> to me.
>>
>> this method makes it much _coupled_ between the driver and the dts file.
>>
>> IMHO, it's an unnecessary _burden_ to the driver programmer:
>>    he should puts the most feature compatible on the top.
>>
>> it's much graceful if we let the driver programmer be transparent about
>> this.
> 
> The dts requires compatible strings to be most specific to least
> specific. There is no reason that driver match tables should not be
> the same and that is the assumption. Matching is not just based on
> compatible properties and your patch does not handle the other cases.

Well, that may be true, but the only way to guarantee that the DT
compatible property is matched correctly is to match it in the order
it's written. Forcing driver writers to write the of_match table in a
particular order is quite a hack, and doesn't guarantee the correct
match order in all cases, only typical cases.

[PATCH] of: match the compatible in the order set by the dts file

[Stephen Warren]

On 07/09/2013 06:03 AM, Rob Herring wrote:
> On Tue, Jul 9, 2013 at 3:10 AM, Huang Shijie <b32955@freescale.com> wrote:
>> ? 2013?07?09? 15:51, Sascha Hauer ??:
>>
>>> On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>>>>
>>>> ? 2013?07?09? 15:05, Sascha Hauer ??:
>>>>>
>>>>> Why don't you set the matching order in the driver the way you want it
>>>>> to be, i.e.:
>>>>>
>>>>>         { .compatible = "fsl,imx6q-uart", ... },
>>>>>         { .compatible = "fsl,imx21-uart", ... },
>>>>>         { .compatible = "fsl,imx1-uart", ... },
>>>>>
>>>> yes. i can set it like this.
>>>>
>>>> but this method looks like a ugly workaround.
>>>
>>> If a driver has different ways of supporting a single device, then
>>> putting the preferred or most feature rich on top doesn't look very ugly
>>> to me.
>>
>> this method makes it much _coupled_ between the driver and the dts file.
>>
>> IMHO, it's an unnecessary _burden_ to the driver programmer:
>>    he should puts the most feature compatible on the top.
>>
>> it's much graceful if we let the driver programmer be transparent about
>> this.
> 
> The dts requires compatible strings to be most specific to least
> specific. There is no reason that driver match tables should not be
> the same and that is the assumption. Matching is not just based on
> compatible properties and your patch does not handle the other cases.

Well, that may be true, but the only way to guarantee that the DT
compatible property is matched correctly is to match it in the order
it's written. Forcing driver writers to write the of_match table in a
particular order is quite a hack, and doesn't guarantee the correct
match order in all cases, only typical cases.

Re: [PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

于 2013年07月09日 20:03, Rob Herring 写道:
> the same and that is the assumption. Matching is not just based on
> compatible properties and your patch does not handle the other cases.
Could you show a example of "the other cases"?

After this patch,

[1] the matching will first check the @match->type and @match->name,
if we can match the @match->type or @match->name, return the match
immediately.

[2] If [1] fails, we will check the compatible properties.



thanks
Huang Shijie

Re: [PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

于 2013年07月09日 20:03, Rob Herring 写道:
> the same and that is the assumption. Matching is not just based on
> compatible properties and your patch does not handle the other cases.
Could you show a example of "the other cases"?

After this patch,

[1] the matching will first check the @match->type and @match->name,
if we can match the @match->type or @match->name, return the match
immediately.

[2] If [1] fails, we will check the compatible properties.



thanks
Huang Shijie

[PATCH] of: match the compatible in the order set by the dts file

[Huang Shijie]

? 2013?07?09? 20:03, Rob Herring ??:
> the same and that is the assumption. Matching is not just based on
> compatible properties and your patch does not handle the other cases.
Could you show a example of "the other cases"?

After this patch,

[1] the matching will first check the @match->type and @match->name,
if we can match the @match->type or @match->name, return the match
immediately.

[2] If [1] fails, we will check the compatible properties.



thanks
Huang Shijie

Re: [PATCH] of: match the compatible in the order set by the dts file

[Grant Likely]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年07月09日 15:51, Sascha Hauer 写道:
> > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> >> 于 2013年07月09日 15:05, Sascha Hauer 写道:
> >>> Why don't you set the matching order in the driver the way you want it
> >>> to be, i.e.:
> >>>
> >>> 	{ .compatible = "fsl,imx6q-uart", ... },
> >>> 	{ .compatible = "fsl,imx21-uart", ... },
> >>> 	{ .compatible = "fsl,imx1-uart", ... },
> >>>
> >> yes. i can set it like this.
> >>
> >> but this method looks like a ugly workaround.
> > If a driver has different ways of supporting a single device, then
> > putting the preferred or most feature rich on top doesn't look very ugly
> > to me.
> this method makes it much _coupled_ between the driver and the dts file.
> 
> IMHO, it's an unnecessary _burden_ to the driver programmer:
>     he should puts the most feature compatible on the top.
> 
> it's much graceful if we let the driver programmer be transparent about 
> this.

Absolutely true. Applied, thanks.

g.

Re: [PATCH] of: match the compatible in the order set by the dts file

[Grant Likely]

On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年07月09日 15:51, Sascha Hauer 写道:
> > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> >> 于 2013年07月09日 15:05, Sascha Hauer 写道:
> >>> Why don't you set the matching order in the driver the way you want it
> >>> to be, i.e.:
> >>>
> >>> 	{ .compatible = "fsl,imx6q-uart", ... },
> >>> 	{ .compatible = "fsl,imx21-uart", ... },
> >>> 	{ .compatible = "fsl,imx1-uart", ... },
> >>>
> >> yes. i can set it like this.
> >>
> >> but this method looks like a ugly workaround.
> > If a driver has different ways of supporting a single device, then
> > putting the preferred or most feature rich on top doesn't look very ugly
> > to me.
> this method makes it much _coupled_ between the driver and the dts file.
> 
> IMHO, it's an unnecessary _burden_ to the driver programmer:
>     he should puts the most feature compatible on the top.
> 
> it's much graceful if we let the driver programmer be transparent about 
> this.

Absolutely true. Applied, thanks.

g.

[PATCH] of: match the compatible in the order set by the dts file

[Grant Likely]

On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955@freescale.com> wrote:
> ??? 2013???07???09??? 15:51, Sascha Hauer ??????:
> > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> >> ??? 2013???07???09??? 15:05, Sascha Hauer ??????:
> >>> Why don't you set the matching order in the driver the way you want it
> >>> to be, i.e.:
> >>>
> >>> 	{ .compatible = "fsl,imx6q-uart", ... },
> >>> 	{ .compatible = "fsl,imx21-uart", ... },
> >>> 	{ .compatible = "fsl,imx1-uart", ... },
> >>>
> >> yes. i can set it like this.
> >>
> >> but this method looks like a ugly workaround.
> > If a driver has different ways of supporting a single device, then
> > putting the preferred or most feature rich on top doesn't look very ugly
> > to me.
> this method makes it much _coupled_ between the driver and the dts file.
> 
> IMHO, it's an unnecessary _burden_ to the driver programmer:
>     he should puts the most feature compatible on the top.
> 
> it's much graceful if we let the driver programmer be transparent about 
> this.

Absolutely true. Applied, thanks.

g.

Re: [PATCH] of: match the compatible in the order set by the dts file

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2147 bytes --]

On Sat, Jul 20, 2013 at 06:44:39AM +0100, Grant Likely wrote:
> On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955@freescale.com> wrote:
> > 于 2013年07月09日 15:51, Sascha Hauer 写道:
> > > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> > >> 于 2013年07月09日 15:05, Sascha Hauer 写道:
> > >>> Why don't you set the matching order in the driver the way you want it
> > >>> to be, i.e.:
> > >>>
> > >>> 	{ .compatible = "fsl,imx6q-uart", ... },
> > >>> 	{ .compatible = "fsl,imx21-uart", ... },
> > >>> 	{ .compatible = "fsl,imx1-uart", ... },
> > >>>
> > >> yes. i can set it like this.
> > >>
> > >> but this method looks like a ugly workaround.
> > > If a driver has different ways of supporting a single device, then
> > > putting the preferred or most feature rich on top doesn't look very ugly
> > > to me.
> > this method makes it much _coupled_ between the driver and the dts file.
> > 
> > IMHO, it's an unnecessary _burden_ to the driver programmer:
> >     he should puts the most feature compatible on the top.
> > 
> > it's much graceful if we let the driver programmer be transparent about 
> > this.
> 
> Absolutely true. Applied, thanks.

I don't think this will work. The patch looks very similar to one that I
posted about a year ago (commit 107a84e "of: match by compatible
property first"). The problem it was trying to address was exactly the
same. However the patch caused regressions on SPARC and had to be
reverted in commit bc51b0c "Revert "of: match by compatible property
first"".

The revert commit quotes Rob having a fix for this, and I remember that
I pinged him about it occasionally, but I suspect that those must have
fallen through the cracks.

Grant, before applying this patch we should make sure that it doesn't
cause a regression again, so perhaps asking Meelis Roos (mentioned in
the revert commit) to test would be good. I have a strong feeling that
this patch will break in the same way than mine did a year ago, because
it looks too similar and doesn't handle the compatible & name
combination that Rob mentioned.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] of: match the compatible in the order set by the dts file

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2173 bytes --]

On Sat, Jul 20, 2013 at 06:44:39AM +0100, Grant Likely wrote:
> On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 于 2013年07月09日 15:51, Sascha Hauer 写道:
> > > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> > >> 于 2013年07月09日 15:05, Sascha Hauer 写道:
> > >>> Why don't you set the matching order in the driver the way you want it
> > >>> to be, i.e.:
> > >>>
> > >>> 	{ .compatible = "fsl,imx6q-uart", ... },
> > >>> 	{ .compatible = "fsl,imx21-uart", ... },
> > >>> 	{ .compatible = "fsl,imx1-uart", ... },
> > >>>
> > >> yes. i can set it like this.
> > >>
> > >> but this method looks like a ugly workaround.
> > > If a driver has different ways of supporting a single device, then
> > > putting the preferred or most feature rich on top doesn't look very ugly
> > > to me.
> > this method makes it much _coupled_ between the driver and the dts file.
> > 
> > IMHO, it's an unnecessary _burden_ to the driver programmer:
> >     he should puts the most feature compatible on the top.
> > 
> > it's much graceful if we let the driver programmer be transparent about 
> > this.
> 
> Absolutely true. Applied, thanks.

I don't think this will work. The patch looks very similar to one that I
posted about a year ago (commit 107a84e "of: match by compatible
property first"). The problem it was trying to address was exactly the
same. However the patch caused regressions on SPARC and had to be
reverted in commit bc51b0c "Revert "of: match by compatible property
first"".

The revert commit quotes Rob having a fix for this, and I remember that
I pinged him about it occasionally, but I suspect that those must have
fallen through the cracks.

Grant, before applying this patch we should make sure that it doesn't
cause a regression again, so perhaps asking Meelis Roos (mentioned in
the revert commit) to test would be good. I have a strong feeling that
this patch will break in the same way than mine did a year ago, because
it looks too similar and doesn't handle the compatible & name
combination that Rob mentioned.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH] of: match the compatible in the order set by the dts file

[Thierry Reding]

On Sat, Jul 20, 2013 at 06:44:39AM +0100, Grant Likely wrote:
> On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955@freescale.com> wrote:
> > ? 2013?07?09? 15:51, Sascha Hauer ??:
> > > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
> > >> ? 2013?07?09? 15:05, Sascha Hauer ??:
> > >>> Why don't you set the matching order in the driver the way you want it
> > >>> to be, i.e.:
> > >>>
> > >>> 	{ .compatible = "fsl,imx6q-uart", ... },
> > >>> 	{ .compatible = "fsl,imx21-uart", ... },
> > >>> 	{ .compatible = "fsl,imx1-uart", ... },
> > >>>
> > >> yes. i can set it like this.
> > >>
> > >> but this method looks like a ugly workaround.
> > > If a driver has different ways of supporting a single device, then
> > > putting the preferred or most feature rich on top doesn't look very ugly
> > > to me.
> > this method makes it much _coupled_ between the driver and the dts file.
> > 
> > IMHO, it's an unnecessary _burden_ to the driver programmer:
> >     he should puts the most feature compatible on the top.
> > 
> > it's much graceful if we let the driver programmer be transparent about 
> > this.
> 
> Absolutely true. Applied, thanks.

I don't think this will work. The patch looks very similar to one that I
posted about a year ago (commit 107a84e "of: match by compatible
property first"). The problem it was trying to address was exactly the
same. However the patch caused regressions on SPARC and had to be
reverted in commit bc51b0c "Revert "of: match by compatible property
first"".

The revert commit quotes Rob having a fix for this, and I remember that
I pinged him about it occasionally, but I suspect that those must have
fallen through the cracks.

Grant, before applying this patch we should make sure that it doesn't
cause a regression again, so perhaps asking Meelis Roos (mentioned in
the revert commit) to test would be good. I have a strong feeling that
this patch will break in the same way than mine did a year ago, because
it looks too similar and doesn't handle the compatible & name
combination that Rob mentioned.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130721/282471db/attachment.sig>

Re: [PATCH] of: match the compatible in the order set by the dts file

[Rob Herring]

On Sat, Jul 20, 2013 at 12:44 AM, Grant Likely <grant.likely@linaro.org>
wrote:
> On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955@freescale.com> wrote:
>> 于 2013年07月09日 15:51, Sascha Hauer 写道:
>> > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>> >> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>> >>> Why don't you set the matching order in the driver the way you want it
>> >>> to be, i.e.:
>> >>>
>> >>>   { .compatible = "fsl,imx6q-uart", ... },
>> >>>   { .compatible = "fsl,imx21-uart", ... },
>> >>>   { .compatible = "fsl,imx1-uart", ... },
>> >>>
>> >> yes. i can set it like this.
>> >>
>> >> but this method looks like a ugly workaround.
>> > If a driver has different ways of supporting a single device, then
>> > putting the preferred or most feature rich on top doesn't look very ugly
>> > to me.
>> this method makes it much _coupled_ between the driver and the dts file.
>>
>> IMHO, it's an unnecessary _burden_ to the driver programmer:
>>     he should puts the most feature compatible on the top.
>>
>> it's much graceful if we let the driver programmer be transparent about
>> this.
>
> Absolutely true. Applied, thanks.

We can debate whether the driver order matters or not, but either way
I'm not sure this patch does the right thing. It doesn't really look
correct to me, but I haven't dug into it.

We've already tried to fix matching and reverted the fix once before
(commit below). So this patch needs careful review and thought about
cases where the name and/or type is used to match.

commit bc51b0c22cebf5c311a6f1895fcca9f78efd0478
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue Jul 10 12:49:32 2012 -0700

    Revert "of: match by compatible property first"

    This reverts commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6.

    Meelis Roos reports a regression since 3.5-rc5 that stops Sun Fire V100
    and Sun Netra X1 sparc64 machines from booting, hanging after enabling
    serial console.  He bisected it to commit 107a84e61cdd.

    Rob Herring explains:
     "The problem is match combinations of compatible plus name and/or type
      fail to match correctly.  I have a fix for this, but given how late it
      is for 3.5 I think it is best to revert this for now.  There could be
      other cases that rely on the current although wrong behavior.  I will
      post an updated version for 3.6."

    Bisected-and-reported-by: Meelis Roos <mroos@linux.ee>
    Requested-by: Rob Herring <rob.herring@calxeda.com>
    Cc: Thierry Reding <thierry.reding@avionic-design.de>
    Cc: Grant Likely <grant.likely@secretlab.ca>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>


Rob

Re: [PATCH] of: match the compatible in the order set by the dts file

[Rob Herring]

On Sat, Jul 20, 2013 at 12:44 AM, Grant Likely <grant.likely@linaro.org>
wrote:
> On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955@freescale.com> wrote:
>> 于 2013年07月09日 15:51, Sascha Hauer 写道:
>> > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>> >> 于 2013年07月09日 15:05, Sascha Hauer 写道:
>> >>> Why don't you set the matching order in the driver the way you want it
>> >>> to be, i.e.:
>> >>>
>> >>>   { .compatible = "fsl,imx6q-uart", ... },
>> >>>   { .compatible = "fsl,imx21-uart", ... },
>> >>>   { .compatible = "fsl,imx1-uart", ... },
>> >>>
>> >> yes. i can set it like this.
>> >>
>> >> but this method looks like a ugly workaround.
>> > If a driver has different ways of supporting a single device, then
>> > putting the preferred or most feature rich on top doesn't look very ugly
>> > to me.
>> this method makes it much _coupled_ between the driver and the dts file.
>>
>> IMHO, it's an unnecessary _burden_ to the driver programmer:
>>     he should puts the most feature compatible on the top.
>>
>> it's much graceful if we let the driver programmer be transparent about
>> this.
>
> Absolutely true. Applied, thanks.

We can debate whether the driver order matters or not, but either way
I'm not sure this patch does the right thing. It doesn't really look
correct to me, but I haven't dug into it.

We've already tried to fix matching and reverted the fix once before
(commit below). So this patch needs careful review and thought about
cases where the name and/or type is used to match.

commit bc51b0c22cebf5c311a6f1895fcca9f78efd0478
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue Jul 10 12:49:32 2012 -0700

    Revert "of: match by compatible property first"

    This reverts commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6.

    Meelis Roos reports a regression since 3.5-rc5 that stops Sun Fire V100
    and Sun Netra X1 sparc64 machines from booting, hanging after enabling
    serial console.  He bisected it to commit 107a84e61cdd.

    Rob Herring explains:
     "The problem is match combinations of compatible plus name and/or type
      fail to match correctly.  I have a fix for this, but given how late it
      is for 3.5 I think it is best to revert this for now.  There could be
      other cases that rely on the current although wrong behavior.  I will
      post an updated version for 3.6."

    Bisected-and-reported-by: Meelis Roos <mroos@linux.ee>
    Requested-by: Rob Herring <rob.herring@calxeda.com>
    Cc: Thierry Reding <thierry.reding@avionic-design.de>
    Cc: Grant Likely <grant.likely@secretlab.ca>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>


Rob
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH] of: match the compatible in the order set by the dts file

[Rob Herring]

On Sat, Jul 20, 2013 at 12:44 AM, Grant Likely <grant.likely@linaro.org>
wrote:
> On Tue, 9 Jul 2013 16:10:53 +0800, Huang Shijie <b32955@freescale.com> wrote:
>> ? 2013?07?09? 15:51, Sascha Hauer ??:
>> > On Tue, Jul 09, 2013 at 03:46:34PM +0800, Huang Shijie wrote:
>> >> ? 2013?07?09? 15:05, Sascha Hauer ??:
>> >>> Why don't you set the matching order in the driver the way you want it
>> >>> to be, i.e.:
>> >>>
>> >>>   { .compatible = "fsl,imx6q-uart", ... },
>> >>>   { .compatible = "fsl,imx21-uart", ... },
>> >>>   { .compatible = "fsl,imx1-uart", ... },
>> >>>
>> >> yes. i can set it like this.
>> >>
>> >> but this method looks like a ugly workaround.
>> > If a driver has different ways of supporting a single device, then
>> > putting the preferred or most feature rich on top doesn't look very ugly
>> > to me.
>> this method makes it much _coupled_ between the driver and the dts file.
>>
>> IMHO, it's an unnecessary _burden_ to the driver programmer:
>>     he should puts the most feature compatible on the top.
>>
>> it's much graceful if we let the driver programmer be transparent about
>> this.
>
> Absolutely true. Applied, thanks.

We can debate whether the driver order matters or not, but either way
I'm not sure this patch does the right thing. It doesn't really look
correct to me, but I haven't dug into it.

We've already tried to fix matching and reverted the fix once before
(commit below). So this patch needs careful review and thought about
cases where the name and/or type is used to match.

commit bc51b0c22cebf5c311a6f1895fcca9f78efd0478
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue Jul 10 12:49:32 2012 -0700

    Revert "of: match by compatible property first"

    This reverts commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6.

    Meelis Roos reports a regression since 3.5-rc5 that stops Sun Fire V100
    and Sun Netra X1 sparc64 machines from booting, hanging after enabling
    serial console.  He bisected it to commit 107a84e61cdd.

    Rob Herring explains:
     "The problem is match combinations of compatible plus name and/or type
      fail to match correctly.  I have a fix for this, but given how late it
      is for 3.5 I think it is best to revert this for now.  There could be
      other cases that rely on the current although wrong behavior.  I will
      post an updated version for 3.6."

    Bisected-and-reported-by: Meelis Roos <mroos@linux.ee>
    Requested-by: Rob Herring <rob.herring@calxeda.com>
    Cc: Thierry Reding <thierry.reding@avionic-design.de>
    Cc: Grant Likely <grant.likely@secretlab.ca>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>


Rob

Re: [PATCH] of: match the compatible in the order set by the dts file

[Grant Likely]

On Sun, Jul 21, 2013 at 9:45 PM, Rob Herring <robherring2@gmail.com> wrote:
> We can debate whether the driver order matters or not, but either way
> I'm not sure this patch does the right thing. It doesn't really look
> correct to me, but I haven't dug into it.
>
> We've already tried to fix matching and reverted the fix once before
> (commit below). So this patch needs careful review and thought about
> cases where the name and/or type is used to match.

The rules have always been well established. This patch /shouldn't/
cause any regression that cannot be narrowed down to a fixable driver
bug.

A harder problem to solve however is dealing with the case when
multiple drivers will potentially bind against the same device. Making
that work requires getting all of the match tables into the kernel
early so that the kernel can select the correct driver of many. Can't
be that big a problem though since we've never actually tried to solve
it.  :-)

[...]
>     Rob Herring explains:
>      "The problem is match combinations of compatible plus name and/or type
>       fail to match correctly.  I have a fix for this, but given how late it
>       is for 3.5 I think it is best to revert this for now.  There could be
>       other cases that rely on the current although wrong behavior.  I will
>       post an updated version for 3.6."

I don't believe the fix ever got posted. Do you still have it? Or can
you describe what needs to be done?

g.

Re: [PATCH] of: match the compatible in the order set by the dts file

[Grant Likely]

On Sun, Jul 21, 2013 at 9:45 PM, Rob Herring <robherring2@gmail.com> wrote:
> We can debate whether the driver order matters or not, but either way
> I'm not sure this patch does the right thing. It doesn't really look
> correct to me, but I haven't dug into it.
>
> We've already tried to fix matching and reverted the fix once before
> (commit below). So this patch needs careful review and thought about
> cases where the name and/or type is used to match.

The rules have always been well established. This patch /shouldn't/
cause any regression that cannot be narrowed down to a fixable driver
bug.

A harder problem to solve however is dealing with the case when
multiple drivers will potentially bind against the same device. Making
that work requires getting all of the match tables into the kernel
early so that the kernel can select the correct driver of many. Can't
be that big a problem though since we've never actually tried to solve
it.  :-)

[...]
>     Rob Herring explains:
>      "The problem is match combinations of compatible plus name and/or type
>       fail to match correctly.  I have a fix for this, but given how late it
>       is for 3.5 I think it is best to revert this for now.  There could be
>       other cases that rely on the current although wrong behavior.  I will
>       post an updated version for 3.6."

I don't believe the fix ever got posted. Do you still have it? Or can
you describe what needs to be done?

g.

[PATCH] of: match the compatible in the order set by the dts file

[Grant Likely]

On Sun, Jul 21, 2013 at 9:45 PM, Rob Herring <robherring2@gmail.com> wrote:
> We can debate whether the driver order matters or not, but either way
> I'm not sure this patch does the right thing. It doesn't really look
> correct to me, but I haven't dug into it.
>
> We've already tried to fix matching and reverted the fix once before
> (commit below). So this patch needs careful review and thought about
> cases where the name and/or type is used to match.

The rules have always been well established. This patch /shouldn't/
cause any regression that cannot be narrowed down to a fixable driver
bug.

A harder problem to solve however is dealing with the case when
multiple drivers will potentially bind against the same device. Making
that work requires getting all of the match tables into the kernel
early so that the kernel can select the correct driver of many. Can't
be that big a problem though since we've never actually tried to solve
it.  :-)

[...]
>     Rob Herring explains:
>      "The problem is match combinations of compatible plus name and/or type
>       fail to match correctly.  I have a fix for this, but given how late it
>       is for 3.5 I think it is best to revert this for now.  There could be
>       other cases that rely on the current although wrong behavior.  I will
>       post an updated version for 3.6."

I don't believe the fix ever got posted. Do you still have it? Or can
you describe what needs to be done?

g.