Re: [PATCH] of: require a match on all fields of of_device_id

2012-07-23 Thread Scott Wood 
On 07/22/2012 08:56 PM, Rob Herring wrote:
 On 07/18/2012 11:04 AM, Scott Wood wrote:
 On 07/17/2012 09:38 PM, Rob Herring wrote:
 On 07/17/2012 08:11 PM, Scott Wood wrote:
 Commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6 (of: match by compatible
 property first) breaks the gianfar ethernet driver found on various
 Freescale PPC chips.

 You do know this is reverted, right?

 No, I didn't.  I got it via Kumar's next branch, and saw that it was
 still in your fixes-for-grant branch, and didn't see any revert-related
 e-mail activity on the devicetree-discuss list about it.  I now see that
 it was reverted directly in Linus's tree (I didn't see either the
 original or the revert in Linus's tree when I checked, but apparently I
 hadn't fetched that as recently as I thought).

 Here's my fix (untested) which is a bit simpler. I'm assuming if we care
 about which compatible string we are matching to, then we require name
 and type are blank and we only care about compatible strings.

 Any particular reason for making that assumption?  We should be avoiding
 the need for .name or .type matching in new bindings, but this seems
 like unnecessarily inconsistent behavior.
 
 Only to ensure we don't change existing behavior and I think trying to
 cover all possibilities will be nearly impossible. For example, what if
 we match on both a specific compatible prop alone and a less specific
 compatible prop plus name and/or type. Which one do we pick as the
 better match?

Whichever one has a compatible string that is listed earlier.  Having an
additional type/name field doesn't necessarily make it a better match --
it's just there to resolve ambiguity (or sometimes for no good reason at
all).

You're going to change existing behavior in some case no matter what,
short of a match table flag saying it's OK to not keep depending on
link order, or just not making the change.  Might as well change it to
something sane.

-Scott


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] of: require a match on all fields of of_device_id

2012-07-22 Thread Rob Herring 
On 07/18/2012 11:04 AM, Scott Wood wrote:
 On 07/17/2012 09:38 PM, Rob Herring wrote:
 On 07/17/2012 08:11 PM, Scott Wood wrote:
 Commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6 (of: match by compatible
 property first) breaks the gianfar ethernet driver found on various
 Freescale PPC chips.

 You do know this is reverted, right?
 
 No, I didn't.  I got it via Kumar's next branch, and saw that it was
 still in your fixes-for-grant branch, and didn't see any revert-related
 e-mail activity on the devicetree-discuss list about it.  I now see that
 it was reverted directly in Linus's tree (I didn't see either the
 original or the revert in Linus's tree when I checked, but apparently I
 hadn't fetched that as recently as I thought).
 
 Here's my fix (untested) which is a bit simpler. I'm assuming if we care
 about which compatible string we are matching to, then we require name
 and type are blank and we only care about compatible strings.
 
 Any particular reason for making that assumption?  We should be avoiding
 the need for .name or .type matching in new bindings, but this seems
 like unnecessarily inconsistent behavior.

Only to ensure we don't change existing behavior and I think trying to
cover all possibilities will be nearly impossible. For example, what if
we match on both a specific compatible prop alone and a less specific
compatible prop plus name and/or type. Which one do we pick as the
better match?

Rob
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] of: require a match on all fields of of_device_id

2012-07-18 Thread Scott Wood 
On 07/17/2012 09:38 PM, Rob Herring wrote:
 On 07/17/2012 08:11 PM, Scott Wood wrote:
 Commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6 (of: match by compatible
 property first) breaks the gianfar ethernet driver found on various
 Freescale PPC chips.
 
 You do know this is reverted, right?

No, I didn't.  I got it via Kumar's next branch, and saw that it was
still in your fixes-for-grant branch, and didn't see any revert-related
e-mail activity on the devicetree-discuss list about it.  I now see that
it was reverted directly in Linus's tree (I didn't see either the
original or the revert in Linus's tree when I checked, but apparently I
hadn't fetched that as recently as I thought).

 Here's my fix (untested) which is a bit simpler. I'm assuming if we care
 about which compatible string we are matching to, then we require name
 and type are blank and we only care about compatible strings.

Any particular reason for making that assumption?  We should be avoiding
the need for .name or .type matching in new bindings, but this seems
like unnecessarily inconsistent behavior.

-Scott


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH] of: require a match on all fields of of_device_id

2012-07-17 Thread Scott Wood 
Commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6 (of: match by compatible
property first) breaks the gianfar ethernet driver found on various
Freescale PPC chips.

There are, for unfortunate historical reasons, two nodes with a
compatible of gianfar.  One has a device_type of network and the
other has device_type of mdio.  The match entries look like this:

 {
 .type = mdio,
 .compatible = gianfar,
 },

and

 {
 .type = network,
 .compatible = gianfar,
 },

With the above patch, both nodes get probed by the first driver, because
nothing else in the match struct is looked at if there's a compatible
match.

Signed-off-by: Scott Wood scottw...@freescale.com
---
 drivers/of/base.c |   44 
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index bc86ea2..4e707cc 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -511,14 +511,37 @@ out:
 }
 EXPORT_SYMBOL(of_find_node_with_property);
 
-static const struct of_device_id *of_match_compat(const struct of_device_id 
*matches,
- const char *compat)
+/*
+ * Tell if an device_node matches the non-compatible fields of
+ * a specific of_match element.
+ */
+static bool of_match_one_noncompat(const struct of_device_id *match,
+  const struct device_node *node)
+{
+   bool is_match = true;
+
+   if (match-name[0])
+   is_match = node-name  !strcmp(match-name, node-name);
+   if (match-type[0])
+   is_match = node-type  !strcmp(match-type, node-type);
+
+   return is_match;
+}
+
+/*
+ * Find an OF match using the supplied compatible string, rather than
+ * the node's entire string list.
+ */
+static const struct of_device_id *of_match_compat(
+   const struct of_device_id *matches, const char *compat,
+   const struct device_node *node)
 {
while (matches-name[0] || matches-type[0] || matches-compatible[0]) {
const char *cp = matches-compatible;
int len = strlen(cp);
 
-   if (len  0  of_compat_cmp(compat, cp, len) == 0)
+   if (len  0  of_compat_cmp(compat, cp, len) == 0 
+   of_match_one_noncompat(matches, node))
return matches;
 
matches++;
@@ -544,23 +567,20 @@ const struct of_device_id *of_match_node(const struct 
of_device_id *matches,
return NULL;
 
of_property_for_each_string(node, compatible, prop, cp) {
-   const struct of_device_id *match = of_match_compat(matches, cp);
+   const struct of_device_id *match =
+   of_match_compat(matches, cp, node);
if (match)
return match;
}
 
while (matches-name[0] || matches-type[0] || matches-compatible[0]) {
-   int match = 1;
-   if (matches-name[0])
-   match = node-name
-!strcmp(matches-name, node-name);
-   if (matches-type[0])
-   match = node-type
-!strcmp(matches-type, node-type);
-   if (match  !matches-compatible[0])
+   if (of_match_one_noncompat(matches, node) 
+   !matches-compatible[0])
return matches;
+
matches++;
}
+
return NULL;
 }
 EXPORT_SYMBOL(of_match_node);
-- 
1.7.9.5

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] of: require a match on all fields of of_device_id

2012-07-17 Thread Tabi Timur-B04825 
On Tue, Jul 17, 2012 at 8:11 PM, Scott Wood scottw...@freescale.com wrote:
 Commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6 (of: match by compatible
 property first) breaks the gianfar ethernet driver found on various
 Freescale PPC chips.

 There are, for unfortunate historical reasons, two nodes with a
 compatible of gianfar.

Would it be worth updating the binding for the two nodes to make the
compatible property different?  We could do something like this:

ethernet@24000 {
reg = 0x24000 0x1000;
compatible = fsl,gianfar-eth, gianfar;
...
};

(and something similar for MDIO nodes)

and update all the drivers to look for both strings.  After a few
years, we can delete gianfar.

-- 
Timur Tabi
Linux kernel developer at Freescale
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] of: require a match on all fields of of_device_id

2012-07-17 Thread Rob Herring 
On 07/17/2012 08:11 PM, Scott Wood wrote:
 Commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6 (of: match by compatible
 property first) breaks the gianfar ethernet driver found on various
 Freescale PPC chips.

You do know this is reverted, right?

 
 There are, for unfortunate historical reasons, two nodes with a
 compatible of gianfar.  One has a device_type of network and the
 other has device_type of mdio.  The match entries look like this:
 
 {
 .type = mdio,
 .compatible = gianfar,
 },
 
 and
 
 {
 .type = network,
 .compatible = gianfar,
 },
 
 With the above patch, both nodes get probed by the first driver, because
 nothing else in the match struct is looked at if there's a compatible
 match.
 
 Signed-off-by: Scott Wood scottw...@freescale.com

Here's my fix (untested) which is a bit simpler. I'm assuming if we care
about which compatible string we are matching to, then we require name
and type are blank and we only care about compatible strings.

diff --git a/drivers/of/base.c b/drivers/of/base.c
index eada3f4..6e10004 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -518,6 +518,9 @@ static const struct of_device_id
*of_match_compat(const struct of_device_id *mat
const char *cp = matches-compatible;
int len = strlen(cp);

+   if (matches-name[0] || matches-type[0])
+   continue;
+
if (len  0  of_compat_cmp(compat, cp, len) == 0)
return matches;

@@ -557,7 +560,10 @@ 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 (match  !matches-compatible[0])
+   if (matches-compatible[0])
+   match = of_device_is_compatible(node,
+   matches-compatible);
+   if (match)
return matches;
matches++;
}

 ---
  drivers/of/base.c |   44 
  1 file changed, 32 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/of/base.c b/drivers/of/base.c
 index bc86ea2..4e707cc 100644
 --- a/drivers/of/base.c
 +++ b/drivers/of/base.c
 @@ -511,14 +511,37 @@ out:
  }
  EXPORT_SYMBOL(of_find_node_with_property);
  
 -static const struct of_device_id *of_match_compat(const struct of_device_id 
 *matches,
 -   const char *compat)
 +/*
 + * Tell if an device_node matches the non-compatible fields of
 + * a specific of_match element.
 + */
 +static bool of_match_one_noncompat(const struct of_device_id *match,
 +const struct device_node *node)
 +{
 + bool is_match = true;
 +
 + if (match-name[0])
 + is_match = node-name  !strcmp(match-name, node-name);
 + if (match-type[0])
 + is_match = node-type  !strcmp(match-type, node-type);
 +
 + return is_match;
 +}
 +
 +/*
 + * Find an OF match using the supplied compatible string, rather than
 + * the node's entire string list.
 + */
 +static const struct of_device_id *of_match_compat(
 + const struct of_device_id *matches, const char *compat,
 + const struct device_node *node)
  {
   while (matches-name[0] || matches-type[0] || matches-compatible[0]) {
   const char *cp = matches-compatible;
   int len = strlen(cp);
  
 - if (len  0  of_compat_cmp(compat, cp, len) == 0)
 + if (len  0  of_compat_cmp(compat, cp, len) == 0 
 + of_match_one_noncompat(matches, node))
   return matches;
  
   matches++;
 @@ -544,23 +567,20 @@ const struct of_device_id *of_match_node(const struct 
 of_device_id *matches,
   return NULL;
  
   of_property_for_each_string(node, compatible, prop, cp) {
 - const struct of_device_id *match = of_match_compat(matches, cp);
 + const struct of_device_id *match =
 + of_match_compat(matches, cp, node);
   if (match)
   return match;
   }
  
   while (matches-name[0] || matches-type[0] || matches-compatible[0]) {
 - int match = 1;
 - if (matches-name[0])
 - match = node-name
 -  !strcmp(matches-name, node-name);
 - if (matches-type[0])
 - match = node-type
 -  !strcmp(matches-type, node-type);
 - if (match  !matches-compatible[0])
 + if (of_match_one_noncompat(matches, node) 
 + !matches-compatible[0])
   return matches;
 +
   matches++;
   }
 +
   return NULL;
  }