* [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-19 3:06 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-19 3:06 UTC (permalink / raw)
To: Daniel Drake
Cc: David Woodhouse, cbou, linux-kernel, x86, tglx, mingo, hpa,
dmitry.torokhov, Grant Likely, devicetree-discuss,
David S. Miller, sparclinux
On Fri, 18 Feb 2011 23:42:57 +0000
Daniel Drake <dsd@laptop.org> wrote:
> On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> wrote:
> > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> >>
> >> +static int __init add_common_platform_devices(void)
> >> +{
> >> + struct platform_device *pdev;
> >> +
> >> + pdev = platform_device_register_simple("olpc-battery", -1,
> >> NULL, 0);
> >> + if (IS_ERR(pdev))
> >> + return PTR_ERR(pdev);
> >> +
> >> + return 0;
> >> +}
> >> +
> >
> > Still kind of sucks that you have to do this, and can't bind to
> > something in the device-tree.
>
> OK, feel free to put this patch on hold for now. I started looking at
> the device tree approach today. It looks doable but first we have to
> fix a DT bug/inconsistency that is preventing us from correctly
> binding to the tree's devices.
>
> Daniel
Mea culpa. The patch below fixes a bug I introduced earlier.
Cc'ing the sparc folks, as this probably affects them
(although I would think that it fixes broken behavior for them..?)
From: Andres Salomon <dilinger@queued.net>
Commit e2f2a93b changed dp->name from using the 'name' property to
using package-to-path. This fixed /proc/device-tree creation by
eliminating conflicts between names (the 'name' property provides
names like 'battery', whereas package-to-path provides names like
'/foo/bar/battery@0', which we stripped to 'battery@0'). However,
it also breaks of_device_id table matching.
The fix that we _really_ wanted was to keep dp->name based upon
the name property ('battery'), but based dp->full_name upon
package-to-path ('battery@0'). This patch does just that.
Signed-off-by: Andres Salomon <dilinger@queued.net>
Reported-by: Daniel Drake <dsd@laptop.org>
---
drivers/of/pdt.c | 42 +++++++++++++++++++-----------------------
1 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..b39d584 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -48,7 +48,7 @@ static inline void irq_trans_init(struct device_node *dp) { }
static inline const char *of_pdt_node_name(struct device_node *dp)
{
- return dp->name;
+ return NULL;
}
#endif /* !CONFIG_SPARC */
@@ -156,23 +156,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
return res+1;
}
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM callback,
- * or some other random failure), fall back to just looking at the node's
- * 'name' property.
- */
-static char * __init of_pdt_build_name(phandle node)
-{
- char *buf;
-
- buf = of_pdt_try_pkg2path(node);
- if (!buf)
- buf = of_pdt_get_one_property(node, "name");
-
- return buf;
-}
-
static struct device_node * __init of_pdt_create_node(phandle node,
struct device_node *parent)
{
@@ -187,7 +170,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
kref_init(&dp->kref);
- dp->name = of_pdt_build_name(node);
+ dp->name = of_pdt_get_one_property(node, "name");
dp->type = of_pdt_get_one_property(node, "device_type");
dp->phandle = node;
@@ -198,13 +181,26 @@ static struct device_node * __init of_pdt_create_node(phandle node,
return dp;
}
-static char * __init of_pdt_build_full_name(struct device_node *dp)
+static char * __init of_pdt_build_full_name(struct device_node *dp,
+ phandle node)
{
int len, ourlen, plen;
+ const char *name;
char *n;
+ /*
+ * When fetching the full name, rather than what we see with the
+ * name property (ie, 'battery'), we want the name we see with
+ * package-to-path (ie, 'battery@0').
+ */
+ name = of_pdt_node_name(dp);
+ if (!name)
+ name = of_pdt_try_pkg2path(node);
+ if (!name)
+ name = dp->name;
+
plen = strlen(dp->parent->full_name);
- ourlen = strlen(of_pdt_node_name(dp));
+ ourlen = strlen(name);
len = ourlen + plen + 2;
n = prom_early_alloc(len);
@@ -213,7 +209,7 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
strcpy(n + plen, "/");
plen++;
}
- strcpy(n + plen, of_pdt_node_name(dp));
+ strcpy(n + plen, name);
return n;
}
@@ -243,7 +239,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
#if defined(CONFIG_SPARC)
dp->path_component_name = build_path_component(dp);
#endif
- dp->full_name = of_pdt_build_full_name(dp);
+ dp->full_name = of_pdt_build_full_name(dp, node);
dp->child = of_pdt_build_tree(dp,
of_pdt_prom_ops->getchild(node), nextp);
--
1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-19 3:06 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-19 3:06 UTC (permalink / raw)
To: Daniel Drake
Cc: David Woodhouse, cbou, linux-kernel, x86, tglx, mingo, hpa,
dmitry.torokhov, Grant Likely, devicetree-discuss,
David S. Miller, sparclinux
On Fri, 18 Feb 2011 23:42:57 +0000
Daniel Drake <dsd@laptop.org> wrote:
> On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> wrote:
> > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> >>
> >> +static int __init add_common_platform_devices(void)
> >> +{
> >> + struct platform_device *pdev;
> >> +
> >> + pdev = platform_device_register_simple("olpc-battery", -1,
> >> NULL, 0);
> >> + if (IS_ERR(pdev))
> >> + return PTR_ERR(pdev);
> >> +
> >> + return 0;
> >> +}
> >> +
> >
> > Still kind of sucks that you have to do this, and can't bind to
> > something in the device-tree.
>
> OK, feel free to put this patch on hold for now. I started looking at
> the device tree approach today. It looks doable but first we have to
> fix a DT bug/inconsistency that is preventing us from correctly
> binding to the tree's devices.
>
> Daniel
Mea culpa. The patch below fixes a bug I introduced earlier.
Cc'ing the sparc folks, as this probably affects them
(although I would think that it fixes broken behavior for them..?)
From: Andres Salomon <dilinger@queued.net>
Commit e2f2a93b changed dp->name from using the 'name' property to
using package-to-path. This fixed /proc/device-tree creation by
eliminating conflicts between names (the 'name' property provides
names like 'battery', whereas package-to-path provides names like
'/foo/bar/battery@0', which we stripped to 'battery@0'). However,
it also breaks of_device_id table matching.
The fix that we _really_ wanted was to keep dp->name based upon
the name property ('battery'), but based dp->full_name upon
package-to-path ('battery@0'). This patch does just that.
Signed-off-by: Andres Salomon <dilinger@queued.net>
Reported-by: Daniel Drake <dsd@laptop.org>
---
drivers/of/pdt.c | 42 +++++++++++++++++++-----------------------
1 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..b39d584 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -48,7 +48,7 @@ static inline void irq_trans_init(struct device_node *dp) { }
static inline const char *of_pdt_node_name(struct device_node *dp)
{
- return dp->name;
+ return NULL;
}
#endif /* !CONFIG_SPARC */
@@ -156,23 +156,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
return res+1;
}
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM callback,
- * or some other random failure), fall back to just looking at the node's
- * 'name' property.
- */
-static char * __init of_pdt_build_name(phandle node)
-{
- char *buf;
-
- buf = of_pdt_try_pkg2path(node);
- if (!buf)
- buf = of_pdt_get_one_property(node, "name");
-
- return buf;
-}
-
static struct device_node * __init of_pdt_create_node(phandle node,
struct device_node *parent)
{
@@ -187,7 +170,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
kref_init(&dp->kref);
- dp->name = of_pdt_build_name(node);
+ dp->name = of_pdt_get_one_property(node, "name");
dp->type = of_pdt_get_one_property(node, "device_type");
dp->phandle = node;
@@ -198,13 +181,26 @@ static struct device_node * __init of_pdt_create_node(phandle node,
return dp;
}
-static char * __init of_pdt_build_full_name(struct device_node *dp)
+static char * __init of_pdt_build_full_name(struct device_node *dp,
+ phandle node)
{
int len, ourlen, plen;
+ const char *name;
char *n;
+ /*
+ * When fetching the full name, rather than what we see with the
+ * name property (ie, 'battery'), we want the name we see with
+ * package-to-path (ie, 'battery@0').
+ */
+ name = of_pdt_node_name(dp);
+ if (!name)
+ name = of_pdt_try_pkg2path(node);
+ if (!name)
+ name = dp->name;
+
plen = strlen(dp->parent->full_name);
- ourlen = strlen(of_pdt_node_name(dp));
+ ourlen = strlen(name);
len = ourlen + plen + 2;
n = prom_early_alloc(len);
@@ -213,7 +209,7 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
strcpy(n + plen, "/");
plen++;
}
- strcpy(n + plen, of_pdt_node_name(dp));
+ strcpy(n + plen, name);
return n;
}
@@ -243,7 +239,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
#if defined(CONFIG_SPARC)
dp->path_component_name = build_path_component(dp);
#endif
- dp->full_name = of_pdt_build_full_name(dp);
+ dp->full_name = of_pdt_build_full_name(dp, node);
dp->child = of_pdt_build_tree(dp,
of_pdt_prom_ops->getchild(node), nextp);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH] of/pdt: don't bother parsing pkg2path results, return as-is
2011-02-19 3:06 ` Andres Salomon
(?)
@ 2011-02-19 3:12 ` Andres Salomon
-1 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-19 3:12 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, Grant Likely, devicetree-discuss,
Mitch Bradley, David S. Miller, sparclinux
On Fri, 18 Feb 2011 19:06:59 -0800
Andres Salomon <dilinger@queued.net> wrote:
> On Fri, 18 Feb 2011 23:42:57 +0000
> Daniel Drake <dsd@laptop.org> wrote:
>
> > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > wrote:
> > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > >>
> > >> +static int __init add_common_platform_devices(void)
> > >> +{
> > >> + struct platform_device *pdev;
> > >> +
> > >> + pdev = platform_device_register_simple("olpc-battery",
> > >> -1, NULL, 0);
> > >> + if (IS_ERR(pdev))
> > >> + return PTR_ERR(pdev);
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >
> > > Still kind of sucks that you have to do this, and can't bind to
> > > something in the device-tree.
> >
> > OK, feel free to put this patch on hold for now. I started looking
> > at the device tree approach today. It looks doable but first we
> > have to fix a DT bug/inconsistency that is preventing us from
> > correctly binding to the tree's devices.
> >
> > Daniel
>
>
> Mea culpa. The patch below fixes a bug I introduced earlier.
> Cc'ing the sparc folks, as this probably affects them
> (although I would think that it fixes broken behavior for them..?)
>
>
>
And this is a followup to the prior patch; an optimization based
upon what we're doing with pkg2path stuff.
The sparc folks don't use pkg2path, so this shouldn't affect them at
all.
Cc'ing Mitch as well, in case there's some reason why I should be
wary of doing this. :)
From: Andres Salomon <dilinger@queued.net>
The of_pdt_try_pkg2path() function currently allocates memory for
and fetches the full path name from OFW (of the form '/foo/bar/baz@0').
It then returns only the name ('baz@0)', and the caller re-constructs
the full name (for dp->full_name) based upon dp->parent->full_name and
what was returned by of_pdt_try_pkg2path(). Oh, and it allocates more
memory for it.
OLPC is the only architecture which fills in the pkg2path hook, so
this shouldn't affect any other users (ie, sparc). Since in practice
(dp->parent->full_name + '/' + strrchr(pkg2path, '/')) and the result
from pkg2path end up being exactly the same, just short circuit the rest
of the of_pdt_build_full_name logic and set dp->full_name to the result
of pkg2path. This means we don't have to parse it nor allocate more
memory for it.
This saves time, code, and memory.
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index b39d584..605834b 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
static char * __init of_pdt_try_pkg2path(phandle node)
{
- char *res, *buf = NULL;
+ char *buf = NULL;
int len;
if (!of_pdt_prom_ops->pkg2path)
@@ -147,13 +147,7 @@ static char * __init of_pdt_try_pkg2path(phandle node)
pr_err("%s: package-to-path failed\n", __func__);
return NULL;
}
-
- res = strrchr(buf, '/');
- if (!res) {
- pr_err("%s: couldn't find / in %s\n", __func__, buf);
- return NULL;
- }
- return res+1;
+ return buf;
}
static struct device_node * __init of_pdt_create_node(phandle node,
@@ -193,10 +187,13 @@ static char * __init of_pdt_build_full_name(struct device_node *dp,
* name property (ie, 'battery'), we want the name we see with
* package-to-path (ie, 'battery@0').
*/
+ n = of_pdt_try_pkg2path(node);
+ if (n)
+ return n;
+
+ /* Older methods for determining full name */
name = of_pdt_node_name(dp);
if (!name)
- name = of_pdt_try_pkg2path(node);
- if (!name)
name = dp->name;
plen = strlen(dp->parent->full_name);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH] of/pdt: don't bother parsing pkg2path results, return as-is
@ 2011-02-19 3:12 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-19 3:12 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, Grant Likely, devicetree-discuss,
Mitch Bradley, David S. Miller, sparclinux
On Fri, 18 Feb 2011 19:06:59 -0800
Andres Salomon <dilinger@queued.net> wrote:
> On Fri, 18 Feb 2011 23:42:57 +0000
> Daniel Drake <dsd@laptop.org> wrote:
>
> > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > wrote:
> > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > >>
> > >> +static int __init add_common_platform_devices(void)
> > >> +{
> > >> + struct platform_device *pdev;
> > >> +
> > >> + pdev = platform_device_register_simple("olpc-battery",
> > >> -1, NULL, 0);
> > >> + if (IS_ERR(pdev))
> > >> + return PTR_ERR(pdev);
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >
> > > Still kind of sucks that you have to do this, and can't bind to
> > > something in the device-tree.
> >
> > OK, feel free to put this patch on hold for now. I started looking
> > at the device tree approach today. It looks doable but first we
> > have to fix a DT bug/inconsistency that is preventing us from
> > correctly binding to the tree's devices.
> >
> > Daniel
>
>
> Mea culpa. The patch below fixes a bug I introduced earlier.
> Cc'ing the sparc folks, as this probably affects them
> (although I would think that it fixes broken behavior for them..?)
>
>
>
And this is a followup to the prior patch; an optimization based
upon what we're doing with pkg2path stuff.
The sparc folks don't use pkg2path, so this shouldn't affect them at
all.
Cc'ing Mitch as well, in case there's some reason why I should be
wary of doing this. :)
From: Andres Salomon <dilinger@queued.net>
The of_pdt_try_pkg2path() function currently allocates memory for
and fetches the full path name from OFW (of the form '/foo/bar/baz@0').
It then returns only the name ('baz@0)', and the caller re-constructs
the full name (for dp->full_name) based upon dp->parent->full_name and
what was returned by of_pdt_try_pkg2path(). Oh, and it allocates more
memory for it.
OLPC is the only architecture which fills in the pkg2path hook, so
this shouldn't affect any other users (ie, sparc). Since in practice
(dp->parent->full_name + '/' + strrchr(pkg2path, '/')) and the result
from pkg2path end up being exactly the same, just short circuit the rest
of the of_pdt_build_full_name logic and set dp->full_name to the result
of pkg2path. This means we don't have to parse it nor allocate more
memory for it.
This saves time, code, and memory.
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index b39d584..605834b 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
static char * __init of_pdt_try_pkg2path(phandle node)
{
- char *res, *buf = NULL;
+ char *buf = NULL;
int len;
if (!of_pdt_prom_ops->pkg2path)
@@ -147,13 +147,7 @@ static char * __init of_pdt_try_pkg2path(phandle node)
pr_err("%s: package-to-path failed\n", __func__);
return NULL;
}
-
- res = strrchr(buf, '/');
- if (!res) {
- pr_err("%s: couldn't find / in %s\n", __func__, buf);
- return NULL;
- }
- return res+1;
+ return buf;
}
static struct device_node * __init of_pdt_create_node(phandle node,
@@ -193,10 +187,13 @@ static char * __init of_pdt_build_full_name(struct device_node *dp,
* name property (ie, 'battery'), we want the name we see with
* package-to-path (ie, 'battery@0').
*/
+ n = of_pdt_try_pkg2path(node);
+ if (n)
+ return n;
+
+ /* Older methods for determining full name */
name = of_pdt_node_name(dp);
if (!name)
- name = of_pdt_try_pkg2path(node);
- if (!name)
name = dp->name;
plen = strlen(dp->parent->full_name);
--
1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH] of/pdt: don't bother parsing pkg2path results, return as-is
@ 2011-02-19 3:12 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-19 3:12 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, Grant Likely, devicetree-discuss,
Mitch Bradley, David S. Miller, sparclinux
On Fri, 18 Feb 2011 19:06:59 -0800
Andres Salomon <dilinger@queued.net> wrote:
> On Fri, 18 Feb 2011 23:42:57 +0000
> Daniel Drake <dsd@laptop.org> wrote:
>
> > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > wrote:
> > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > >>
> > >> +static int __init add_common_platform_devices(void)
> > >> +{
> > >> + struct platform_device *pdev;
> > >> +
> > >> + pdev = platform_device_register_simple("olpc-battery",
> > >> -1, NULL, 0);
> > >> + if (IS_ERR(pdev))
> > >> + return PTR_ERR(pdev);
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >
> > > Still kind of sucks that you have to do this, and can't bind to
> > > something in the device-tree.
> >
> > OK, feel free to put this patch on hold for now. I started looking
> > at the device tree approach today. It looks doable but first we
> > have to fix a DT bug/inconsistency that is preventing us from
> > correctly binding to the tree's devices.
> >
> > Daniel
>
>
> Mea culpa. The patch below fixes a bug I introduced earlier.
> Cc'ing the sparc folks, as this probably affects them
> (although I would think that it fixes broken behavior for them..?)
>
>
>
And this is a followup to the prior patch; an optimization based
upon what we're doing with pkg2path stuff.
The sparc folks don't use pkg2path, so this shouldn't affect them at
all.
Cc'ing Mitch as well, in case there's some reason why I should be
wary of doing this. :)
From: Andres Salomon <dilinger@queued.net>
The of_pdt_try_pkg2path() function currently allocates memory for
and fetches the full path name from OFW (of the form '/foo/bar/baz@0').
It then returns only the name ('baz@0)', and the caller re-constructs
the full name (for dp->full_name) based upon dp->parent->full_name and
what was returned by of_pdt_try_pkg2path(). Oh, and it allocates more
memory for it.
OLPC is the only architecture which fills in the pkg2path hook, so
this shouldn't affect any other users (ie, sparc). Since in practice
(dp->parent->full_name + '/' + strrchr(pkg2path, '/')) and the result
from pkg2path end up being exactly the same, just short circuit the rest
of the of_pdt_build_full_name logic and set dp->full_name to the result
of pkg2path. This means we don't have to parse it nor allocate more
memory for it.
This saves time, code, and memory.
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index b39d584..605834b 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
static char * __init of_pdt_try_pkg2path(phandle node)
{
- char *res, *buf = NULL;
+ char *buf = NULL;
int len;
if (!of_pdt_prom_ops->pkg2path)
@@ -147,13 +147,7 @@ static char * __init of_pdt_try_pkg2path(phandle node)
pr_err("%s: package-to-path failed\n", __func__);
return NULL;
}
-
- res = strrchr(buf, '/');
- if (!res) {
- pr_err("%s: couldn't find / in %s\n", __func__, buf);
- return NULL;
- }
- return res+1;
+ return buf;
}
static struct device_node * __init of_pdt_create_node(phandle node,
@@ -193,10 +187,13 @@ static char * __init of_pdt_build_full_name(struct device_node *dp,
* name property (ie, 'battery'), we want the name we see with
* package-to-path (ie, 'battery@0').
*/
+ n = of_pdt_try_pkg2path(node);
+ if (n)
+ return n;
+
+ /* Older methods for determining full name */
name = of_pdt_node_name(dp);
if (!name)
- name = of_pdt_try_pkg2path(node);
- if (!name)
name = dp->name;
plen = strlen(dp->parent->full_name);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: don't bother parsing pkg2path results, return
2011-02-19 3:12 ` Andres Salomon
(?)
@ 2011-02-23 19:45 ` Grant Likely
-1 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 19:45 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, Mitch Bradley,
David S. Miller, sparclinux
On Fri, Feb 18, 2011 at 07:12:25PM -0800, Andres Salomon wrote:
> On Fri, 18 Feb 2011 19:06:59 -0800
> Andres Salomon <dilinger@queued.net> wrote:
>
> > On Fri, 18 Feb 2011 23:42:57 +0000
> > Daniel Drake <dsd@laptop.org> wrote:
> >
> > > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > > wrote:
> > > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > > >>
> > > >> +static int __init add_common_platform_devices(void)
> > > >> +{
> > > >> + struct platform_device *pdev;
> > > >> +
> > > >> + pdev = platform_device_register_simple("olpc-battery",
> > > >> -1, NULL, 0);
> > > >> + if (IS_ERR(pdev))
> > > >> + return PTR_ERR(pdev);
> > > >> +
> > > >> + return 0;
> > > >> +}
> > > >> +
> > > >
> > > > Still kind of sucks that you have to do this, and can't bind to
> > > > something in the device-tree.
> > >
> > > OK, feel free to put this patch on hold for now. I started looking
> > > at the device tree approach today. It looks doable but first we
> > > have to fix a DT bug/inconsistency that is preventing us from
> > > correctly binding to the tree's devices.
> > >
> > > Daniel
> >
> >
> > Mea culpa. The patch below fixes a bug I introduced earlier.
> > Cc'ing the sparc folks, as this probably affects them
> > (although I would think that it fixes broken behavior for them..?)
> >
> >
> >
>
> And this is a followup to the prior patch; an optimization based
> upon what we're doing with pkg2path stuff.
>
> The sparc folks don't use pkg2path, so this shouldn't affect them at
> all.
>
> Cc'ing Mitch as well, in case there's some reason why I should be
> wary of doing this. :)
>
>
>
>
> From: Andres Salomon <dilinger@queued.net>
>
> The of_pdt_try_pkg2path() function currently allocates memory for
> and fetches the full path name from OFW (of the form '/foo/bar/baz@0').
> It then returns only the name ('baz@0)', and the caller re-constructs
> the full name (for dp->full_name) based upon dp->parent->full_name and
> what was returned by of_pdt_try_pkg2path(). Oh, and it allocates more
> memory for it.
>
> OLPC is the only architecture which fills in the pkg2path hook, so
> this shouldn't affect any other users (ie, sparc). Since in practice
> (dp->parent->full_name + '/' + strrchr(pkg2path, '/')) and the result
> from pkg2path end up being exactly the same, just short circuit the rest
> of the of_pdt_build_full_name logic and set dp->full_name to the result
> of pkg2path. This means we don't have to parse it nor allocate more
> memory for it.
>
> This saves time, code, and memory.
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> ---
> drivers/of/pdt.c | 17 +++++++----------
> 1 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index b39d584..605834b 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
>
> static char * __init of_pdt_try_pkg2path(phandle node)
> {
> - char *res, *buf = NULL;
> + char *buf = NULL;
> int len;
>
> if (!of_pdt_prom_ops->pkg2path)
> @@ -147,13 +147,7 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> pr_err("%s: package-to-path failed\n", __func__);
> return NULL;
> }
> -
> - res = strrchr(buf, '/');
> - if (!res) {
> - pr_err("%s: couldn't find / in %s\n", __func__, buf);
> - return NULL;
> - }
> - return res+1;
> + return buf;
> }
>
> static struct device_node * __init of_pdt_create_node(phandle node,
> @@ -193,10 +187,13 @@ static char * __init of_pdt_build_full_name(struct device_node *dp,
> * name property (ie, 'battery'), we want the name we see with
> * package-to-path (ie, 'battery@0').
> */
> + n = of_pdt_try_pkg2path(node);
> + if (n)
> + return n;
> +
> + /* Older methods for determining full name */
> name = of_pdt_node_name(dp);
> if (!name)
> - name = of_pdt_try_pkg2path(node);
> - if (!name)
Ah, this addresses one of the comments I just made on the first patch.
I'd squash the two patches together before you repost.
g.
> name = dp->name;
>
> plen = strlen(dp->parent->full_name);
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: don't bother parsing pkg2path results, return as-is
@ 2011-02-23 19:45 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 19:45 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, Mitch Bradley,
David S. Miller, sparclinux
On Fri, Feb 18, 2011 at 07:12:25PM -0800, Andres Salomon wrote:
> On Fri, 18 Feb 2011 19:06:59 -0800
> Andres Salomon <dilinger@queued.net> wrote:
>
> > On Fri, 18 Feb 2011 23:42:57 +0000
> > Daniel Drake <dsd@laptop.org> wrote:
> >
> > > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > > wrote:
> > > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > > >>
> > > >> +static int __init add_common_platform_devices(void)
> > > >> +{
> > > >> + struct platform_device *pdev;
> > > >> +
> > > >> + pdev = platform_device_register_simple("olpc-battery",
> > > >> -1, NULL, 0);
> > > >> + if (IS_ERR(pdev))
> > > >> + return PTR_ERR(pdev);
> > > >> +
> > > >> + return 0;
> > > >> +}
> > > >> +
> > > >
> > > > Still kind of sucks that you have to do this, and can't bind to
> > > > something in the device-tree.
> > >
> > > OK, feel free to put this patch on hold for now. I started looking
> > > at the device tree approach today. It looks doable but first we
> > > have to fix a DT bug/inconsistency that is preventing us from
> > > correctly binding to the tree's devices.
> > >
> > > Daniel
> >
> >
> > Mea culpa. The patch below fixes a bug I introduced earlier.
> > Cc'ing the sparc folks, as this probably affects them
> > (although I would think that it fixes broken behavior for them..?)
> >
> >
> >
>
> And this is a followup to the prior patch; an optimization based
> upon what we're doing with pkg2path stuff.
>
> The sparc folks don't use pkg2path, so this shouldn't affect them at
> all.
>
> Cc'ing Mitch as well, in case there's some reason why I should be
> wary of doing this. :)
>
>
>
>
> From: Andres Salomon <dilinger@queued.net>
>
> The of_pdt_try_pkg2path() function currently allocates memory for
> and fetches the full path name from OFW (of the form '/foo/bar/baz@0').
> It then returns only the name ('baz@0)', and the caller re-constructs
> the full name (for dp->full_name) based upon dp->parent->full_name and
> what was returned by of_pdt_try_pkg2path(). Oh, and it allocates more
> memory for it.
>
> OLPC is the only architecture which fills in the pkg2path hook, so
> this shouldn't affect any other users (ie, sparc). Since in practice
> (dp->parent->full_name + '/' + strrchr(pkg2path, '/')) and the result
> from pkg2path end up being exactly the same, just short circuit the rest
> of the of_pdt_build_full_name logic and set dp->full_name to the result
> of pkg2path. This means we don't have to parse it nor allocate more
> memory for it.
>
> This saves time, code, and memory.
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> ---
> drivers/of/pdt.c | 17 +++++++----------
> 1 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index b39d584..605834b 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
>
> static char * __init of_pdt_try_pkg2path(phandle node)
> {
> - char *res, *buf = NULL;
> + char *buf = NULL;
> int len;
>
> if (!of_pdt_prom_ops->pkg2path)
> @@ -147,13 +147,7 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> pr_err("%s: package-to-path failed\n", __func__);
> return NULL;
> }
> -
> - res = strrchr(buf, '/');
> - if (!res) {
> - pr_err("%s: couldn't find / in %s\n", __func__, buf);
> - return NULL;
> - }
> - return res+1;
> + return buf;
> }
>
> static struct device_node * __init of_pdt_create_node(phandle node,
> @@ -193,10 +187,13 @@ static char * __init of_pdt_build_full_name(struct device_node *dp,
> * name property (ie, 'battery'), we want the name we see with
> * package-to-path (ie, 'battery@0').
> */
> + n = of_pdt_try_pkg2path(node);
> + if (n)
> + return n;
> +
> + /* Older methods for determining full name */
> name = of_pdt_node_name(dp);
> if (!name)
> - name = of_pdt_try_pkg2path(node);
> - if (!name)
Ah, this addresses one of the comments I just made on the first patch.
I'd squash the two patches together before you repost.
g.
> name = dp->name;
>
> plen = strlen(dp->parent->full_name);
> --
> 1.7.2.3
>
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: don't bother parsing pkg2path results, return as-is
@ 2011-02-23 19:45 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 19:45 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, Mitch Bradley,
David S. Miller, sparclinux
On Fri, Feb 18, 2011 at 07:12:25PM -0800, Andres Salomon wrote:
> On Fri, 18 Feb 2011 19:06:59 -0800
> Andres Salomon <dilinger@queued.net> wrote:
>
> > On Fri, 18 Feb 2011 23:42:57 +0000
> > Daniel Drake <dsd@laptop.org> wrote:
> >
> > > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > > wrote:
> > > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > > >>
> > > >> +static int __init add_common_platform_devices(void)
> > > >> +{
> > > >> + struct platform_device *pdev;
> > > >> +
> > > >> + pdev = platform_device_register_simple("olpc-battery",
> > > >> -1, NULL, 0);
> > > >> + if (IS_ERR(pdev))
> > > >> + return PTR_ERR(pdev);
> > > >> +
> > > >> + return 0;
> > > >> +}
> > > >> +
> > > >
> > > > Still kind of sucks that you have to do this, and can't bind to
> > > > something in the device-tree.
> > >
> > > OK, feel free to put this patch on hold for now. I started looking
> > > at the device tree approach today. It looks doable but first we
> > > have to fix a DT bug/inconsistency that is preventing us from
> > > correctly binding to the tree's devices.
> > >
> > > Daniel
> >
> >
> > Mea culpa. The patch below fixes a bug I introduced earlier.
> > Cc'ing the sparc folks, as this probably affects them
> > (although I would think that it fixes broken behavior for them..?)
> >
> >
> >
>
> And this is a followup to the prior patch; an optimization based
> upon what we're doing with pkg2path stuff.
>
> The sparc folks don't use pkg2path, so this shouldn't affect them at
> all.
>
> Cc'ing Mitch as well, in case there's some reason why I should be
> wary of doing this. :)
>
>
>
>
> From: Andres Salomon <dilinger@queued.net>
>
> The of_pdt_try_pkg2path() function currently allocates memory for
> and fetches the full path name from OFW (of the form '/foo/bar/baz@0').
> It then returns only the name ('baz@0)', and the caller re-constructs
> the full name (for dp->full_name) based upon dp->parent->full_name and
> what was returned by of_pdt_try_pkg2path(). Oh, and it allocates more
> memory for it.
>
> OLPC is the only architecture which fills in the pkg2path hook, so
> this shouldn't affect any other users (ie, sparc). Since in practice
> (dp->parent->full_name + '/' + strrchr(pkg2path, '/')) and the result
> from pkg2path end up being exactly the same, just short circuit the rest
> of the of_pdt_build_full_name logic and set dp->full_name to the result
> of pkg2path. This means we don't have to parse it nor allocate more
> memory for it.
>
> This saves time, code, and memory.
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> ---
> drivers/of/pdt.c | 17 +++++++----------
> 1 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index b39d584..605834b 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
>
> static char * __init of_pdt_try_pkg2path(phandle node)
> {
> - char *res, *buf = NULL;
> + char *buf = NULL;
> int len;
>
> if (!of_pdt_prom_ops->pkg2path)
> @@ -147,13 +147,7 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> pr_err("%s: package-to-path failed\n", __func__);
> return NULL;
> }
> -
> - res = strrchr(buf, '/');
> - if (!res) {
> - pr_err("%s: couldn't find / in %s\n", __func__, buf);
> - return NULL;
> - }
> - return res+1;
> + return buf;
> }
>
> static struct device_node * __init of_pdt_create_node(phandle node,
> @@ -193,10 +187,13 @@ static char * __init of_pdt_build_full_name(struct device_node *dp,
> * name property (ie, 'battery'), we want the name we see with
> * package-to-path (ie, 'battery@0').
> */
> + n = of_pdt_try_pkg2path(node);
> + if (n)
> + return n;
> +
> + /* Older methods for determining full name */
> name = of_pdt_node_name(dp);
> if (!name)
> - name = of_pdt_try_pkg2path(node);
> - if (!name)
Ah, this addresses one of the comments I just made on the first patch.
I'd squash the two patches together before you repost.
g.
> name = dp->name;
>
> plen = strlen(dp->parent->full_name);
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-19 3:06 ` Andres Salomon
(?)
@ 2011-02-23 19:43 ` Grant Likely
-1 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 19:43 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> On Fri, 18 Feb 2011 23:42:57 +0000
> Daniel Drake <dsd@laptop.org> wrote:
>
> > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > wrote:
> > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > >>
> > >> +static int __init add_common_platform_devices(void)
> > >> +{
> > >> + struct platform_device *pdev;
> > >> +
> > >> + pdev = platform_device_register_simple("olpc-battery", -1,
> > >> NULL, 0);
> > >> + if (IS_ERR(pdev))
> > >> + return PTR_ERR(pdev);
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >
> > > Still kind of sucks that you have to do this, and can't bind to
> > > something in the device-tree.
> >
> > OK, feel free to put this patch on hold for now. I started looking at
> > the device tree approach today. It looks doable but first we have to
> > fix a DT bug/inconsistency that is preventing us from correctly
> > binding to the tree's devices.
> >
> > Daniel
>
>
> Mea culpa. The patch below fixes a bug I introduced earlier.
> Cc'ing the sparc folks, as this probably affects them
> (although I would think that it fixes broken behavior for them..?)
Wait; why are you binding to a device based on name? Binding by name
and/or device_type is strongly discouraged for new code. Use compatible
instead.
As for this patch, comments below...
> From: Andres Salomon <dilinger@queued.net>
>
> Commit e2f2a93b changed dp->name from using the 'name' property to
> using package-to-path. This fixed /proc/device-tree creation by
> eliminating conflicts between names (the 'name' property provides
> names like 'battery', whereas package-to-path provides names like
> '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> it also breaks of_device_id table matching.
>
> The fix that we _really_ wanted was to keep dp->name based upon
> the name property ('battery'), but based dp->full_name upon
> package-to-path ('battery@0'). This patch does just that.
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> Reported-by: Daniel Drake <dsd@laptop.org>
From what I can tell, this only affects OLPC, correct? It looks like
SPARC's implementation of of_pdt_node_name() will sidestep most of this
name retrieval code.
> ---
> drivers/of/pdt.c | 42 +++++++++++++++++++-----------------------
> 1 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..b39d584 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -48,7 +48,7 @@ static inline void irq_trans_init(struct device_node *dp) { }
>
> static inline const char *of_pdt_node_name(struct device_node *dp)
> {
> - return dp->name;
> + return NULL;
> }
Rather than using this hook; perhaps the sparc .path_component_name
should be enabled for all architectures. It is a useful data item to
keep a pointer to, and it would simplify the of_pdt code.
>
> #endif /* !CONFIG_SPARC */
> @@ -156,23 +156,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> return res+1;
> }
>
> -/*
> - * When fetching the node's name, first try using package-to-path; if
> - * that fails (either because the arch hasn't supplied a PROM callback,
> - * or some other random failure), fall back to just looking at the node's
> - * 'name' property.
> - */
> -static char * __init of_pdt_build_name(phandle node)
> -{
> - char *buf;
> -
> - buf = of_pdt_try_pkg2path(node);
> - if (!buf)
> - buf = of_pdt_get_one_property(node, "name");
> -
> - return buf;
> -}
> -
> static struct device_node * __init of_pdt_create_node(phandle node,
> struct device_node *parent)
> {
> @@ -187,7 +170,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
>
> kref_init(&dp->kref);
>
> - dp->name = of_pdt_build_name(node);
> + dp->name = of_pdt_get_one_property(node, "name");
> dp->type = of_pdt_get_one_property(node, "device_type");
> dp->phandle = node;
>
> @@ -198,13 +181,26 @@ static struct device_node * __init of_pdt_create_node(phandle node,
> return dp;
> }
>
> -static char * __init of_pdt_build_full_name(struct device_node *dp)
> +static char * __init of_pdt_build_full_name(struct device_node *dp,
> + phandle node)
> {
> int len, ourlen, plen;
> + const char *name;
> char *n;
>
> + /*
> + * When fetching the full name, rather than what we see with the
> + * name property (ie, 'battery'), we want the name we see with
> + * package-to-path (ie, 'battery@0').
> + */
> + name = of_pdt_node_name(dp);
> + if (!name)
> + name = of_pdt_try_pkg2path(node);
> + if (!name)
> + name = dp->name;
> +
Rather than this thrashing about looking for a way to get the path
component, it should be fairly certain how to obtain the node's name
before getting into of_pdt_build_full_name(). I'd follow SPARC's
lead and create an implementation of build_path_component();
However, considering that this is creating the full name; wouldn't the
raw output of pkg2path() provide exactly the full name string you need
here instead of breaking it down into components and adding it back up
again?
> plen = strlen(dp->parent->full_name);
> - ourlen = strlen(of_pdt_node_name(dp));
> + ourlen = strlen(name);
> len = ourlen + plen + 2;
>
> n = prom_early_alloc(len);
> @@ -213,7 +209,7 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
> strcpy(n + plen, "/");
> plen++;
> }
> - strcpy(n + plen, of_pdt_node_name(dp));
> + strcpy(n + plen, name);
>
> return n;
> }
> @@ -243,7 +239,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
> #if defined(CONFIG_SPARC)
> dp->path_component_name = build_path_component(dp);
> #endif
> - dp->full_name = of_pdt_build_full_name(dp);
> + dp->full_name = of_pdt_build_full_name(dp, node);
>
> dp->child = of_pdt_build_tree(dp,
> of_pdt_prom_ops->getchild(node), nextp);
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-23 19:43 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 19:43 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> On Fri, 18 Feb 2011 23:42:57 +0000
> Daniel Drake <dsd@laptop.org> wrote:
>
> > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > wrote:
> > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > >>
> > >> +static int __init add_common_platform_devices(void)
> > >> +{
> > >> + struct platform_device *pdev;
> > >> +
> > >> + pdev = platform_device_register_simple("olpc-battery", -1,
> > >> NULL, 0);
> > >> + if (IS_ERR(pdev))
> > >> + return PTR_ERR(pdev);
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >
> > > Still kind of sucks that you have to do this, and can't bind to
> > > something in the device-tree.
> >
> > OK, feel free to put this patch on hold for now. I started looking at
> > the device tree approach today. It looks doable but first we have to
> > fix a DT bug/inconsistency that is preventing us from correctly
> > binding to the tree's devices.
> >
> > Daniel
>
>
> Mea culpa. The patch below fixes a bug I introduced earlier.
> Cc'ing the sparc folks, as this probably affects them
> (although I would think that it fixes broken behavior for them..?)
Wait; why are you binding to a device based on name? Binding by name
and/or device_type is strongly discouraged for new code. Use compatible
instead.
As for this patch, comments below...
> From: Andres Salomon <dilinger@queued.net>
>
> Commit e2f2a93b changed dp->name from using the 'name' property to
> using package-to-path. This fixed /proc/device-tree creation by
> eliminating conflicts between names (the 'name' property provides
> names like 'battery', whereas package-to-path provides names like
> '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> it also breaks of_device_id table matching.
>
> The fix that we _really_ wanted was to keep dp->name based upon
> the name property ('battery'), but based dp->full_name upon
> package-to-path ('battery@0'). This patch does just that.
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> Reported-by: Daniel Drake <dsd@laptop.org>
From what I can tell, this only affects OLPC, correct? It looks like
SPARC's implementation of of_pdt_node_name() will sidestep most of this
name retrieval code.
> ---
> drivers/of/pdt.c | 42 +++++++++++++++++++-----------------------
> 1 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..b39d584 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -48,7 +48,7 @@ static inline void irq_trans_init(struct device_node *dp) { }
>
> static inline const char *of_pdt_node_name(struct device_node *dp)
> {
> - return dp->name;
> + return NULL;
> }
Rather than using this hook; perhaps the sparc .path_component_name
should be enabled for all architectures. It is a useful data item to
keep a pointer to, and it would simplify the of_pdt code.
>
> #endif /* !CONFIG_SPARC */
> @@ -156,23 +156,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> return res+1;
> }
>
> -/*
> - * When fetching the node's name, first try using package-to-path; if
> - * that fails (either because the arch hasn't supplied a PROM callback,
> - * or some other random failure), fall back to just looking at the node's
> - * 'name' property.
> - */
> -static char * __init of_pdt_build_name(phandle node)
> -{
> - char *buf;
> -
> - buf = of_pdt_try_pkg2path(node);
> - if (!buf)
> - buf = of_pdt_get_one_property(node, "name");
> -
> - return buf;
> -}
> -
> static struct device_node * __init of_pdt_create_node(phandle node,
> struct device_node *parent)
> {
> @@ -187,7 +170,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
>
> kref_init(&dp->kref);
>
> - dp->name = of_pdt_build_name(node);
> + dp->name = of_pdt_get_one_property(node, "name");
> dp->type = of_pdt_get_one_property(node, "device_type");
> dp->phandle = node;
>
> @@ -198,13 +181,26 @@ static struct device_node * __init of_pdt_create_node(phandle node,
> return dp;
> }
>
> -static char * __init of_pdt_build_full_name(struct device_node *dp)
> +static char * __init of_pdt_build_full_name(struct device_node *dp,
> + phandle node)
> {
> int len, ourlen, plen;
> + const char *name;
> char *n;
>
> + /*
> + * When fetching the full name, rather than what we see with the
> + * name property (ie, 'battery'), we want the name we see with
> + * package-to-path (ie, 'battery@0').
> + */
> + name = of_pdt_node_name(dp);
> + if (!name)
> + name = of_pdt_try_pkg2path(node);
> + if (!name)
> + name = dp->name;
> +
Rather than this thrashing about looking for a way to get the path
component, it should be fairly certain how to obtain the node's name
before getting into of_pdt_build_full_name(). I'd follow SPARC's
lead and create an implementation of build_path_component();
However, considering that this is creating the full name; wouldn't the
raw output of pkg2path() provide exactly the full name string you need
here instead of breaking it down into components and adding it back up
again?
> plen = strlen(dp->parent->full_name);
> - ourlen = strlen(of_pdt_node_name(dp));
> + ourlen = strlen(name);
> len = ourlen + plen + 2;
>
> n = prom_early_alloc(len);
> @@ -213,7 +209,7 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
> strcpy(n + plen, "/");
> plen++;
> }
> - strcpy(n + plen, of_pdt_node_name(dp));
> + strcpy(n + plen, name);
>
> return n;
> }
> @@ -243,7 +239,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
> #if defined(CONFIG_SPARC)
> dp->path_component_name = build_path_component(dp);
> #endif
> - dp->full_name = of_pdt_build_full_name(dp);
> + dp->full_name = of_pdt_build_full_name(dp, node);
>
> dp->child = of_pdt_build_tree(dp,
> of_pdt_prom_ops->getchild(node), nextp);
> --
> 1.7.2.3
>
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-23 19:43 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 19:43 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> On Fri, 18 Feb 2011 23:42:57 +0000
> Daniel Drake <dsd@laptop.org> wrote:
>
> > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > wrote:
> > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > >>
> > >> +static int __init add_common_platform_devices(void)
> > >> +{
> > >> + struct platform_device *pdev;
> > >> +
> > >> + pdev = platform_device_register_simple("olpc-battery", -1,
> > >> NULL, 0);
> > >> + if (IS_ERR(pdev))
> > >> + return PTR_ERR(pdev);
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >
> > > Still kind of sucks that you have to do this, and can't bind to
> > > something in the device-tree.
> >
> > OK, feel free to put this patch on hold for now. I started looking at
> > the device tree approach today. It looks doable but first we have to
> > fix a DT bug/inconsistency that is preventing us from correctly
> > binding to the tree's devices.
> >
> > Daniel
>
>
> Mea culpa. The patch below fixes a bug I introduced earlier.
> Cc'ing the sparc folks, as this probably affects them
> (although I would think that it fixes broken behavior for them..?)
Wait; why are you binding to a device based on name? Binding by name
and/or device_type is strongly discouraged for new code. Use compatible
instead.
As for this patch, comments below...
> From: Andres Salomon <dilinger@queued.net>
>
> Commit e2f2a93b changed dp->name from using the 'name' property to
> using package-to-path. This fixed /proc/device-tree creation by
> eliminating conflicts between names (the 'name' property provides
> names like 'battery', whereas package-to-path provides names like
> '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> it also breaks of_device_id table matching.
>
> The fix that we _really_ wanted was to keep dp->name based upon
> the name property ('battery'), but based dp->full_name upon
> package-to-path ('battery@0'). This patch does just that.
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> Reported-by: Daniel Drake <dsd@laptop.org>
>From what I can tell, this only affects OLPC, correct? It looks like
SPARC's implementation of of_pdt_node_name() will sidestep most of this
name retrieval code.
> ---
> drivers/of/pdt.c | 42 +++++++++++++++++++-----------------------
> 1 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..b39d584 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -48,7 +48,7 @@ static inline void irq_trans_init(struct device_node *dp) { }
>
> static inline const char *of_pdt_node_name(struct device_node *dp)
> {
> - return dp->name;
> + return NULL;
> }
Rather than using this hook; perhaps the sparc .path_component_name
should be enabled for all architectures. It is a useful data item to
keep a pointer to, and it would simplify the of_pdt code.
>
> #endif /* !CONFIG_SPARC */
> @@ -156,23 +156,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> return res+1;
> }
>
> -/*
> - * When fetching the node's name, first try using package-to-path; if
> - * that fails (either because the arch hasn't supplied a PROM callback,
> - * or some other random failure), fall back to just looking at the node's
> - * 'name' property.
> - */
> -static char * __init of_pdt_build_name(phandle node)
> -{
> - char *buf;
> -
> - buf = of_pdt_try_pkg2path(node);
> - if (!buf)
> - buf = of_pdt_get_one_property(node, "name");
> -
> - return buf;
> -}
> -
> static struct device_node * __init of_pdt_create_node(phandle node,
> struct device_node *parent)
> {
> @@ -187,7 +170,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
>
> kref_init(&dp->kref);
>
> - dp->name = of_pdt_build_name(node);
> + dp->name = of_pdt_get_one_property(node, "name");
> dp->type = of_pdt_get_one_property(node, "device_type");
> dp->phandle = node;
>
> @@ -198,13 +181,26 @@ static struct device_node * __init of_pdt_create_node(phandle node,
> return dp;
> }
>
> -static char * __init of_pdt_build_full_name(struct device_node *dp)
> +static char * __init of_pdt_build_full_name(struct device_node *dp,
> + phandle node)
> {
> int len, ourlen, plen;
> + const char *name;
> char *n;
>
> + /*
> + * When fetching the full name, rather than what we see with the
> + * name property (ie, 'battery'), we want the name we see with
> + * package-to-path (ie, 'battery@0').
> + */
> + name = of_pdt_node_name(dp);
> + if (!name)
> + name = of_pdt_try_pkg2path(node);
> + if (!name)
> + name = dp->name;
> +
Rather than this thrashing about looking for a way to get the path
component, it should be fairly certain how to obtain the node's name
before getting into of_pdt_build_full_name(). I'd follow SPARC's
lead and create an implementation of build_path_component();
However, considering that this is creating the full name; wouldn't the
raw output of pkg2path() provide exactly the full name string you need
here instead of breaking it down into components and adding it back up
again?
> plen = strlen(dp->parent->full_name);
> - ourlen = strlen(of_pdt_node_name(dp));
> + ourlen = strlen(name);
> len = ourlen + plen + 2;
>
> n = prom_early_alloc(len);
> @@ -213,7 +209,7 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
> strcpy(n + plen, "/");
> plen++;
> }
> - strcpy(n + plen, of_pdt_node_name(dp));
> + strcpy(n + plen, name);
>
> return n;
> }
> @@ -243,7 +239,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
> #if defined(CONFIG_SPARC)
> dp->path_component_name = build_path_component(dp);
> #endif
> - dp->full_name = of_pdt_build_full_name(dp);
> + dp->full_name = of_pdt_build_full_name(dp, node);
>
> dp->child = of_pdt_build_tree(dp,
> of_pdt_prom_ops->getchild(node), nextp);
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-23 19:43 ` Grant Likely
(?)
@ 2011-02-23 19:54 ` Andres Salomon
-1 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-23 19:54 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On Wed, 23 Feb 2011 12:43:52 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> > On Fri, 18 Feb 2011 23:42:57 +0000
> > Daniel Drake <dsd@laptop.org> wrote:
> >
> > > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > > wrote:
> > > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > > >>
> > > >> +static int __init add_common_platform_devices(void)
> > > >> +{
> > > >> + struct platform_device *pdev;
> > > >> +
> > > >> + pdev = platform_device_register_simple("olpc-battery",
> > > >> -1, NULL, 0);
> > > >> + if (IS_ERR(pdev))
> > > >> + return PTR_ERR(pdev);
> > > >> +
> > > >> + return 0;
> > > >> +}
> > > >> +
> > > >
> > > > Still kind of sucks that you have to do this, and can't bind to
> > > > something in the device-tree.
> > >
> > > OK, feel free to put this patch on hold for now. I started
> > > looking at the device tree approach today. It looks doable but
> > > first we have to fix a DT bug/inconsistency that is preventing us
> > > from correctly binding to the tree's devices.
> > >
> > > Daniel
> >
> >
> > Mea culpa. The patch below fixes a bug I introduced earlier.
> > Cc'ing the sparc folks, as this probably affects them
> > (although I would think that it fixes broken behavior for them..?)
>
> Wait; why are you binding to a device based on name? Binding by name
> and/or device_type is strongly discouraged for new code. Use
> compatible instead.
>
Daniel posted a separate patch showing his code, would you mind
commenting on that? I noticed he didn't cc you though, here's the
patch:
https://patchwork.kernel.org/patch/574901/
> As for this patch, comments below...
>
> > From: Andres Salomon <dilinger@queued.net>
> >
> > Commit e2f2a93b changed dp->name from using the 'name' property to
> > using package-to-path. This fixed /proc/device-tree creation by
> > eliminating conflicts between names (the 'name' property provides
> > names like 'battery', whereas package-to-path provides names like
> > '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> > it also breaks of_device_id table matching.
> >
> > The fix that we _really_ wanted was to keep dp->name based upon
> > the name property ('battery'), but based dp->full_name upon
> > package-to-path ('battery@0'). This patch does just that.
> >
> > Signed-off-by: Andres Salomon <dilinger@queued.net>
> > Reported-by: Daniel Drake <dsd@laptop.org>
>
> From what I can tell, this only affects OLPC, correct? It looks like
> SPARC's implementation of of_pdt_node_name() will sidestep most of
> this name retrieval code.
>
This affects sparc as well; it changes behavior back for dp->name to
what it used to be. dp->full_name behavior is still the same for sparc.
> > ---
> > drivers/of/pdt.c | 42 +++++++++++++++++++-----------------------
> > 1 files changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > index 28295d0..b39d584 100644
> > --- a/drivers/of/pdt.c
> > +++ b/drivers/of/pdt.c
> > @@ -48,7 +48,7 @@ static inline void irq_trans_init(struct
> > device_node *dp) { }
> > static inline const char *of_pdt_node_name(struct device_node *dp)
> > {
> > - return dp->name;
> > + return NULL;
> > }
>
> Rather than using this hook; perhaps the sparc .path_component_name
> should be enabled for all architectures. It is a useful data item to
> keep a pointer to, and it would simplify the of_pdt code.
>
> >
Yeah, I considered that, but pkg2path works even better for our
purposes.
I'll combine patches and resend, thanks.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-23 19:54 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-23 19:54 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On Wed, 23 Feb 2011 12:43:52 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> > On Fri, 18 Feb 2011 23:42:57 +0000
> > Daniel Drake <dsd@laptop.org> wrote:
> >
> > > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > > wrote:
> > > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > > >>
> > > >> +static int __init add_common_platform_devices(void)
> > > >> +{
> > > >> + struct platform_device *pdev;
> > > >> +
> > > >> + pdev = platform_device_register_simple("olpc-battery",
> > > >> -1, NULL, 0);
> > > >> + if (IS_ERR(pdev))
> > > >> + return PTR_ERR(pdev);
> > > >> +
> > > >> + return 0;
> > > >> +}
> > > >> +
> > > >
> > > > Still kind of sucks that you have to do this, and can't bind to
> > > > something in the device-tree.
> > >
> > > OK, feel free to put this patch on hold for now. I started
> > > looking at the device tree approach today. It looks doable but
> > > first we have to fix a DT bug/inconsistency that is preventing us
> > > from correctly binding to the tree's devices.
> > >
> > > Daniel
> >
> >
> > Mea culpa. The patch below fixes a bug I introduced earlier.
> > Cc'ing the sparc folks, as this probably affects them
> > (although I would think that it fixes broken behavior for them..?)
>
> Wait; why are you binding to a device based on name? Binding by name
> and/or device_type is strongly discouraged for new code. Use
> compatible instead.
>
Daniel posted a separate patch showing his code, would you mind
commenting on that? I noticed he didn't cc you though, here's the
patch:
https://patchwork.kernel.org/patch/574901/
> As for this patch, comments below...
>
> > From: Andres Salomon <dilinger@queued.net>
> >
> > Commit e2f2a93b changed dp->name from using the 'name' property to
> > using package-to-path. This fixed /proc/device-tree creation by
> > eliminating conflicts between names (the 'name' property provides
> > names like 'battery', whereas package-to-path provides names like
> > '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> > it also breaks of_device_id table matching.
> >
> > The fix that we _really_ wanted was to keep dp->name based upon
> > the name property ('battery'), but based dp->full_name upon
> > package-to-path ('battery@0'). This patch does just that.
> >
> > Signed-off-by: Andres Salomon <dilinger@queued.net>
> > Reported-by: Daniel Drake <dsd@laptop.org>
>
> From what I can tell, this only affects OLPC, correct? It looks like
> SPARC's implementation of of_pdt_node_name() will sidestep most of
> this name retrieval code.
>
This affects sparc as well; it changes behavior back for dp->name to
what it used to be. dp->full_name behavior is still the same for sparc.
> > ---
> > drivers/of/pdt.c | 42 +++++++++++++++++++-----------------------
> > 1 files changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > index 28295d0..b39d584 100644
> > --- a/drivers/of/pdt.c
> > +++ b/drivers/of/pdt.c
> > @@ -48,7 +48,7 @@ static inline void irq_trans_init(struct
> > device_node *dp) { }
> > static inline const char *of_pdt_node_name(struct device_node *dp)
> > {
> > - return dp->name;
> > + return NULL;
> > }
>
> Rather than using this hook; perhaps the sparc .path_component_name
> should be enabled for all architectures. It is a useful data item to
> keep a pointer to, and it would simplify the of_pdt code.
>
> >
Yeah, I considered that, but pkg2path works even better for our
purposes.
I'll combine patches and resend, thanks.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-23 19:54 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-23 19:54 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On Wed, 23 Feb 2011 12:43:52 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> > On Fri, 18 Feb 2011 23:42:57 +0000
> > Daniel Drake <dsd@laptop.org> wrote:
> >
> > > On 16 February 2011 22:44, David Woodhouse <dwmw2@infradead.org>
> > > wrote:
> > > > On Wed, 2011-02-16 at 22:28 +0000, Daniel Drake wrote:
> > > >>
> > > >> +static int __init add_common_platform_devices(void)
> > > >> +{
> > > >> + struct platform_device *pdev;
> > > >> +
> > > >> + pdev = platform_device_register_simple("olpc-battery",
> > > >> -1, NULL, 0);
> > > >> + if (IS_ERR(pdev))
> > > >> + return PTR_ERR(pdev);
> > > >> +
> > > >> + return 0;
> > > >> +}
> > > >> +
> > > >
> > > > Still kind of sucks that you have to do this, and can't bind to
> > > > something in the device-tree.
> > >
> > > OK, feel free to put this patch on hold for now. I started
> > > looking at the device tree approach today. It looks doable but
> > > first we have to fix a DT bug/inconsistency that is preventing us
> > > from correctly binding to the tree's devices.
> > >
> > > Daniel
> >
> >
> > Mea culpa. The patch below fixes a bug I introduced earlier.
> > Cc'ing the sparc folks, as this probably affects them
> > (although I would think that it fixes broken behavior for them..?)
>
> Wait; why are you binding to a device based on name? Binding by name
> and/or device_type is strongly discouraged for new code. Use
> compatible instead.
>
Daniel posted a separate patch showing his code, would you mind
commenting on that? I noticed he didn't cc you though, here's the
patch:
https://patchwork.kernel.org/patch/574901/
> As for this patch, comments below...
>
> > From: Andres Salomon <dilinger@queued.net>
> >
> > Commit e2f2a93b changed dp->name from using the 'name' property to
> > using package-to-path. This fixed /proc/device-tree creation by
> > eliminating conflicts between names (the 'name' property provides
> > names like 'battery', whereas package-to-path provides names like
> > '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> > it also breaks of_device_id table matching.
> >
> > The fix that we _really_ wanted was to keep dp->name based upon
> > the name property ('battery'), but based dp->full_name upon
> > package-to-path ('battery@0'). This patch does just that.
> >
> > Signed-off-by: Andres Salomon <dilinger@queued.net>
> > Reported-by: Daniel Drake <dsd@laptop.org>
>
> From what I can tell, this only affects OLPC, correct? It looks like
> SPARC's implementation of of_pdt_node_name() will sidestep most of
> this name retrieval code.
>
This affects sparc as well; it changes behavior back for dp->name to
what it used to be. dp->full_name behavior is still the same for sparc.
> > ---
> > drivers/of/pdt.c | 42 +++++++++++++++++++-----------------------
> > 1 files changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > index 28295d0..b39d584 100644
> > --- a/drivers/of/pdt.c
> > +++ b/drivers/of/pdt.c
> > @@ -48,7 +48,7 @@ static inline void irq_trans_init(struct
> > device_node *dp) { }
> > static inline const char *of_pdt_node_name(struct device_node *dp)
> > {
> > - return dp->name;
> > + return NULL;
> > }
>
> Rather than using this hook; perhaps the sparc .path_component_name
> should be enabled for all architectures. It is a useful data item to
> keep a pointer to, and it would simplify the of_pdt code.
>
> >
Yeah, I considered that, but pkg2path works even better for our
purposes.
I'll combine patches and resend, thanks.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
2011-02-23 19:54 ` Andres Salomon
@ 2011-02-23 20:06 ` Daniel Drake
-1 siblings, 0 replies; 51+ messages in thread
From: Daniel Drake @ 2011-02-23 20:06 UTC (permalink / raw)
To: Andres Salomon
Cc: Grant Likely, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On 23 February 2011 19:54, Andres Salomon <dilinger@queued.net> wrote:
>> Wait; why are you binding to a device based on name? Binding by name
>> and/or device_type is strongly discouraged for new code. Use
>> compatible instead.
>>
>
> Daniel posted a separate patch showing his code, would you mind
> commenting on that? I noticed he didn't cc you though, here's the
> patch:
>
> https://patchwork.kernel.org/patch/574901/
It does bind to the name.
We don't currently have a compatible property for the battery device.
We could fix that with a firmware upgrade, but it would break
compatibility with all existing installs. I guess this is still the
recommended approach...
Daniel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-23 20:06 ` Daniel Drake
0 siblings, 0 replies; 51+ messages in thread
From: Daniel Drake @ 2011-02-23 20:06 UTC (permalink / raw)
To: Andres Salomon
Cc: Grant Likely, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On 23 February 2011 19:54, Andres Salomon <dilinger@queued.net> wrote:
>> Wait; why are you binding to a device based on name? Binding by name
>> and/or device_type is strongly discouraged for new code. Use
>> compatible instead.
>>
>
> Daniel posted a separate patch showing his code, would you mind
> commenting on that? I noticed he didn't cc you though, here's the
> patch:
>
> https://patchwork.kernel.org/patch/574901/
It does bind to the name.
We don't currently have a compatible property for the battery device.
We could fix that with a firmware upgrade, but it would break
compatibility with all existing installs. I guess this is still the
recommended approach...
Daniel
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 51+ messages in thread
[parent not found: <AANLkTi=D1eWGsN4JVWEGeHp3AXfpbOOKr9Fq7juGAXtT-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-23 20:06 ` Daniel Drake
(?)
@ 2011-02-23 20:37 ` Grant Likely
-1 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 20:37 UTC (permalink / raw)
To: Daniel Drake
Cc: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
hpa-YMNOUZJC4hwAvxtiuMwx3w,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
x86-DgEjT+Ai2ygdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cbou-JGs/UdohzUI, mingo-H+wXaHxf7aLQT0dZR+AlfA, Andres Salomon,
sparclinux-u79uwXL29TY76Z2rM5mHXA, tglx-hfZtesqFncYOwBW4kG4KsQ,
David Woodhouse, David S. Miller
On Wed, Feb 23, 2011 at 08:06:47PM +0000, Daniel Drake wrote:
> On 23 February 2011 19:54, Andres Salomon <dilinger@queued.net> wrote:
> >> Wait; why are you binding to a device based on name? Binding by name
> >> and/or device_type is strongly discouraged for new code. Use
> >> compatible instead.
> >>
> >
> > Daniel posted a separate patch showing his code, would you mind
> > commenting on that? I noticed he didn't cc you though, here's the
> > patch:
> >
> > https://patchwork.kernel.org/patch/574901/
>
> It does bind to the name.
> We don't currently have a compatible property for the battery device.
> We could fix that with a firmware upgrade, but it would break
> compatibility with all existing installs. I guess this is still the
> recommended approach...
Hi Daniel,
As mentioned in my reply to your patch, the solution to this is fix up
the missing compatible property in the board support code before it
pollutes the global matching namespace.
g.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-23 20:37 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 20:37 UTC (permalink / raw)
To: Daniel Drake
Cc: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
hpa-YMNOUZJC4hwAvxtiuMwx3w,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
x86-DgEjT+Ai2ygdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cbou-JGs/UdohzUI, mingo-H+wXaHxf7aLQT0dZR+AlfA, Andres Salomon,
sparclinux-u79uwXL29TY76Z2rM5mHXA, tglx-hfZtesqFncYOwBW4kG4KsQ,
David Woodhouse, David S. Miller
On Wed, Feb 23, 2011 at 08:06:47PM +0000, Daniel Drake wrote:
> On 23 February 2011 19:54, Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org> wrote:
> >> Wait; why are you binding to a device based on name? Binding by name
> >> and/or device_type is strongly discouraged for new code. Use
> >> compatible instead.
> >>
> >
> > Daniel posted a separate patch showing his code, would you mind
> > commenting on that? I noticed he didn't cc you though, here's the
> > patch:
> >
> > https://patchwork.kernel.org/patch/574901/
>
> It does bind to the name.
> We don't currently have a compatible property for the battery device.
> We could fix that with a firmware upgrade, but it would break
> compatibility with all existing installs. I guess this is still the
> recommended approach...
Hi Daniel,
As mentioned in my reply to your patch, the solution to this is fix up
the missing compatible property in the board support code before it
pollutes the global matching namespace.
g.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-23 20:37 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 20:37 UTC (permalink / raw)
To: Daniel Drake
Cc: Andres Salomon, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 08:06:47PM +0000, Daniel Drake wrote:
> On 23 February 2011 19:54, Andres Salomon <dilinger@queued.net> wrote:
> >> Wait; why are you binding to a device based on name? Binding by name
> >> and/or device_type is strongly discouraged for new code. Use
> >> compatible instead.
> >>
> >
> > Daniel posted a separate patch showing his code, would you mind
> > commenting on that? I noticed he didn't cc you though, here's the
> > patch:
> >
> > https://patchwork.kernel.org/patch/574901/
>
> It does bind to the name.
> We don't currently have a compatible property for the battery device.
> We could fix that with a firmware upgrade, but it would break
> compatibility with all existing installs. I guess this is still the
> recommended approach...
Hi Daniel,
As mentioned in my reply to your patch, the solution to this is fix up
the missing compatible property in the board support code before it
pollutes the global matching namespace.
g.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-23 19:54 ` Andres Salomon
@ 2011-02-23 20:35 ` Grant Likely
-1 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 20:35 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 11:54:14AM -0800, Andres Salomon wrote:
> On Wed, 23 Feb 2011 12:43:52 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> > Wait; why are you binding to a device based on name? Binding by name
> > and/or device_type is strongly discouraged for new code. Use
> > compatible instead.
> >
>
> Daniel posted a separate patch showing his code, would you mind
> commenting on that? I noticed he didn't cc you though, here's the
> patch:
>
> https://patchwork.kernel.org/patch/574901/
Done.
g.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-23 20:35 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 20:35 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, David Woodhouse, cbou, linux-kernel, x86, tglx,
mingo, hpa, dmitry.torokhov, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 11:54:14AM -0800, Andres Salomon wrote:
> On Wed, 23 Feb 2011 12:43:52 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
> > Wait; why are you binding to a device based on name? Binding by name
> > and/or device_type is strongly discouraged for new code. Use
> > compatible instead.
> >
>
> Daniel posted a separate patch showing his code, would you mind
> commenting on that? I noticed he didn't cc you though, here's the
> patch:
>
> https://patchwork.kernel.org/patch/574901/
Done.
g.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH] of/pdt: allow DT device matching by fixing 'name'
@ 2011-02-23 23:03 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-23 23:03 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
Commit e2f2a93b changed dp->name from using the 'name' property to
using package-to-path. This fixed /proc/device-tree creation by
eliminating conflicts between names (the 'name' property provides
names like 'battery', whereas package-to-path provides names like
'/foo/bar/battery@0', which we stripped to 'battery@0'). However,
it also breaks of_device_id table matching.
The fix that we _really_ wanted was to keep dp->name based upon
the name property ('battery'), but based dp->full_name upon
package-to-path ('battery@0'). This patch does just that.
This also changes OLPC behavior to use the full result from
package-to-path for full_name, rather than stripping the directory
out. In practice, the strings end up being exactly the same; this
change saves time, code, and memory.
Note that this affects sparc by reverting dp->name back to what
sparc was originally doing (looking at the name property).
v2: combine two patches and revert of_pdt_node_name to original version.
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 42 +++++++++++++++---------------------------
1 files changed, 15 insertions(+), 27 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..a7aa85e 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
static char * __init of_pdt_try_pkg2path(phandle node)
{
- char *res, *buf = NULL;
+ char *buf = NULL;
int len;
if (!of_pdt_prom_ops->pkg2path)
@@ -147,29 +147,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
pr_err("%s: package-to-path failed\n", __func__);
return NULL;
}
-
- res = strrchr(buf, '/');
- if (!res) {
- pr_err("%s: couldn't find / in %s\n", __func__, buf);
- return NULL;
- }
- return res+1;
-}
-
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM callback,
- * or some other random failure), fall back to just looking at the node's
- * 'name' property.
- */
-static char * __init of_pdt_build_name(phandle node)
-{
- char *buf;
-
- buf = of_pdt_try_pkg2path(node);
- if (!buf)
- buf = of_pdt_get_one_property(node, "name");
-
return buf;
}
@@ -187,7 +164,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
kref_init(&dp->kref);
- dp->name = of_pdt_build_name(node);
+ dp->name = of_pdt_get_one_property(node, "name");
dp->type = of_pdt_get_one_property(node, "device_type");
dp->phandle = node;
@@ -198,11 +175,22 @@ static struct device_node * __init of_pdt_create_node(phandle node,
return dp;
}
-static char * __init of_pdt_build_full_name(struct device_node *dp)
+static char * __init of_pdt_build_full_name(struct device_node *dp,
+ phandle node)
{
int len, ourlen, plen;
char *n;
+ /*
+ * When fetching the full name we want the name we see with
+ * package-to-path (ie, '/foo/bar/battery@0') rather than what
+ * we see with the name property (ie, 'battery').
+ */
+ n = of_pdt_try_pkg2path(node);
+ if (n)
+ return n;
+
+ /* Older method for determining full name */
plen = strlen(dp->parent->full_name);
ourlen = strlen(of_pdt_node_name(dp));
len = ourlen + plen + 2;
@@ -243,7 +231,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
#if defined(CONFIG_SPARC)
dp->path_component_name = build_path_component(dp);
#endif
- dp->full_name = of_pdt_build_full_name(dp);
+ dp->full_name = of_pdt_build_full_name(dp, node);
dp->child = of_pdt_build_tree(dp,
of_pdt_prom_ops->getchild(node), nextp);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2)
@ 2011-02-23 23:03 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-23 23:03 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
Commit e2f2a93b changed dp->name from using the 'name' property to
using package-to-path. This fixed /proc/device-tree creation by
eliminating conflicts between names (the 'name' property provides
names like 'battery', whereas package-to-path provides names like
'/foo/bar/battery@0', which we stripped to 'battery@0'). However,
it also breaks of_device_id table matching.
The fix that we _really_ wanted was to keep dp->name based upon
the name property ('battery'), but based dp->full_name upon
package-to-path ('battery@0'). This patch does just that.
This also changes OLPC behavior to use the full result from
package-to-path for full_name, rather than stripping the directory
out. In practice, the strings end up being exactly the same; this
change saves time, code, and memory.
Note that this affects sparc by reverting dp->name back to what
sparc was originally doing (looking at the name property).
v2: combine two patches and revert of_pdt_node_name to original version.
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 42 +++++++++++++++---------------------------
1 files changed, 15 insertions(+), 27 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..a7aa85e 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
static char * __init of_pdt_try_pkg2path(phandle node)
{
- char *res, *buf = NULL;
+ char *buf = NULL;
int len;
if (!of_pdt_prom_ops->pkg2path)
@@ -147,29 +147,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
pr_err("%s: package-to-path failed\n", __func__);
return NULL;
}
-
- res = strrchr(buf, '/');
- if (!res) {
- pr_err("%s: couldn't find / in %s\n", __func__, buf);
- return NULL;
- }
- return res+1;
-}
-
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM callback,
- * or some other random failure), fall back to just looking at the node's
- * 'name' property.
- */
-static char * __init of_pdt_build_name(phandle node)
-{
- char *buf;
-
- buf = of_pdt_try_pkg2path(node);
- if (!buf)
- buf = of_pdt_get_one_property(node, "name");
-
return buf;
}
@@ -187,7 +164,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
kref_init(&dp->kref);
- dp->name = of_pdt_build_name(node);
+ dp->name = of_pdt_get_one_property(node, "name");
dp->type = of_pdt_get_one_property(node, "device_type");
dp->phandle = node;
@@ -198,11 +175,22 @@ static struct device_node * __init of_pdt_create_node(phandle node,
return dp;
}
-static char * __init of_pdt_build_full_name(struct device_node *dp)
+static char * __init of_pdt_build_full_name(struct device_node *dp,
+ phandle node)
{
int len, ourlen, plen;
char *n;
+ /*
+ * When fetching the full name we want the name we see with
+ * package-to-path (ie, '/foo/bar/battery@0') rather than what
+ * we see with the name property (ie, 'battery').
+ */
+ n = of_pdt_try_pkg2path(node);
+ if (n)
+ return n;
+
+ /* Older method for determining full name */
plen = strlen(dp->parent->full_name);
ourlen = strlen(of_pdt_node_name(dp));
len = ourlen + plen + 2;
@@ -243,7 +231,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
#if defined(CONFIG_SPARC)
dp->path_component_name = build_path_component(dp);
#endif
- dp->full_name = of_pdt_build_full_name(dp);
+ dp->full_name = of_pdt_build_full_name(dp, node);
dp->child = of_pdt_build_tree(dp,
of_pdt_prom_ops->getchild(node), nextp);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 51+ messages in thread[parent not found: <20110223150357.5a40793d-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>]
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-23 23:03 ` [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2) Andres Salomon
(?)
@ 2011-02-23 23:28 ` Grant Likely
-1 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 23:28 UTC (permalink / raw)
To: Andres Salomon
Cc: sparclinux-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David S. Miller,
Daniel Drake, linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
>
> Commit e2f2a93b changed dp->name from using the 'name' property to
> using package-to-path. This fixed /proc/device-tree creation by
> eliminating conflicts between names (the 'name' property provides
> names like 'battery', whereas package-to-path provides names like
> '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> it also breaks of_device_id table matching.
>
> The fix that we _really_ wanted was to keep dp->name based upon
> the name property ('battery'), but based dp->full_name upon
> package-to-path ('battery@0'). This patch does just that.
>
> This also changes OLPC behavior to use the full result from
> package-to-path for full_name, rather than stripping the directory
> out. In practice, the strings end up being exactly the same; this
> change saves time, code, and memory.
>
> Note that this affects sparc by reverting dp->name back to what
> sparc was originally doing (looking at the name property).
>
> v2: combine two patches and revert of_pdt_node_name to original version.
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
Hi Andres, comments below.
g.
> ---
> drivers/of/pdt.c | 42 +++++++++++++++---------------------------
> 1 files changed, 15 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..a7aa85e 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
>
> static char * __init of_pdt_try_pkg2path(phandle node)
> {
> - char *res, *buf = NULL;
> + char *buf = NULL;
> int len;
>
> if (!of_pdt_prom_ops->pkg2path)
> @@ -147,29 +147,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> pr_err("%s: package-to-path failed\n", __func__);
> return NULL;
> }
> -
> - res = strrchr(buf, '/');
> - if (!res) {
> - pr_err("%s: couldn't find / in %s\n", __func__, buf);
> - return NULL;
> - }
> - return res+1;
> -}
> -
> -/*
> - * When fetching the node's name, first try using package-to-path; if
> - * that fails (either because the arch hasn't supplied a PROM callback,
> - * or some other random failure), fall back to just looking at the node's
> - * 'name' property.
> - */
> -static char * __init of_pdt_build_name(phandle node)
> -{
> - char *buf;
> -
> - buf = of_pdt_try_pkg2path(node);
> - if (!buf)
> - buf = of_pdt_get_one_property(node, "name");
> -
> return buf;
> }
>
It seems to me that of_pdt_build_full_name will still be missing the
'@<addr>' component on non-sparc non-olpc builds because it uses the
broken of_pdt_node_name(). That needs to be fixed too, even if there
are no current users (or removed).
> @@ -187,7 +164,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
>
> kref_init(&dp->kref);
>
> - dp->name = of_pdt_build_name(node);
> + dp->name = of_pdt_get_one_property(node, "name");
> dp->type = of_pdt_get_one_property(node, "device_type");
> dp->phandle = node;
>
> @@ -198,11 +175,22 @@ static struct device_node * __init of_pdt_create_node(phandle node,
> return dp;
> }
>
> -static char * __init of_pdt_build_full_name(struct device_node *dp)
> +static char * __init of_pdt_build_full_name(struct device_node *dp,
> + phandle node)
Is dp->phandle not suitable here?
> {
> int len, ourlen, plen;
> char *n;
>
> + /*
> + * When fetching the full name we want the name we see with
> + * package-to-path (ie, '/foo/bar/battery@0') rather than what
> + * we see with the name property (ie, 'battery').
> + */
> + n = of_pdt_try_pkg2path(node);
> + if (n)
> + return n;
> +
> + /* Older method for determining full name */
> plen = strlen(dp->parent->full_name);
> ourlen = strlen(of_pdt_node_name(dp));
> len = ourlen + plen + 2;
> @@ -243,7 +231,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
> #if defined(CONFIG_SPARC)
> dp->path_component_name = build_path_component(dp);
> #endif
I still think it would be useful to remove the #if
defined(CONFIG_SPARC) from path_component_name, and it might be the
best way to solve my comment about broken of_pdt_node_name above.
> - dp->full_name = of_pdt_build_full_name(dp);
> + dp->full_name = of_pdt_build_full_name(dp, node);
>
> dp->child = of_pdt_build_tree(dp,
> of_pdt_prom_ops->getchild(node), nextp);
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2)
@ 2011-02-23 23:28 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 23:28 UTC (permalink / raw)
To: Andres Salomon
Cc: sparclinux-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David S. Miller,
Daniel Drake, linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
>
> Commit e2f2a93b changed dp->name from using the 'name' property to
> using package-to-path. This fixed /proc/device-tree creation by
> eliminating conflicts between names (the 'name' property provides
> names like 'battery', whereas package-to-path provides names like
> '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> it also breaks of_device_id table matching.
>
> The fix that we _really_ wanted was to keep dp->name based upon
> the name property ('battery'), but based dp->full_name upon
> package-to-path ('battery@0'). This patch does just that.
>
> This also changes OLPC behavior to use the full result from
> package-to-path for full_name, rather than stripping the directory
> out. In practice, the strings end up being exactly the same; this
> change saves time, code, and memory.
>
> Note that this affects sparc by reverting dp->name back to what
> sparc was originally doing (looking at the name property).
>
> v2: combine two patches and revert of_pdt_node_name to original version.
>
> Signed-off-by: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
Hi Andres, comments below.
g.
> ---
> drivers/of/pdt.c | 42 +++++++++++++++---------------------------
> 1 files changed, 15 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..a7aa85e 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
>
> static char * __init of_pdt_try_pkg2path(phandle node)
> {
> - char *res, *buf = NULL;
> + char *buf = NULL;
> int len;
>
> if (!of_pdt_prom_ops->pkg2path)
> @@ -147,29 +147,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> pr_err("%s: package-to-path failed\n", __func__);
> return NULL;
> }
> -
> - res = strrchr(buf, '/');
> - if (!res) {
> - pr_err("%s: couldn't find / in %s\n", __func__, buf);
> - return NULL;
> - }
> - return res+1;
> -}
> -
> -/*
> - * When fetching the node's name, first try using package-to-path; if
> - * that fails (either because the arch hasn't supplied a PROM callback,
> - * or some other random failure), fall back to just looking at the node's
> - * 'name' property.
> - */
> -static char * __init of_pdt_build_name(phandle node)
> -{
> - char *buf;
> -
> - buf = of_pdt_try_pkg2path(node);
> - if (!buf)
> - buf = of_pdt_get_one_property(node, "name");
> -
> return buf;
> }
>
It seems to me that of_pdt_build_full_name will still be missing the
'@<addr>' component on non-sparc non-olpc builds because it uses the
broken of_pdt_node_name(). That needs to be fixed too, even if there
are no current users (or removed).
> @@ -187,7 +164,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
>
> kref_init(&dp->kref);
>
> - dp->name = of_pdt_build_name(node);
> + dp->name = of_pdt_get_one_property(node, "name");
> dp->type = of_pdt_get_one_property(node, "device_type");
> dp->phandle = node;
>
> @@ -198,11 +175,22 @@ static struct device_node * __init of_pdt_create_node(phandle node,
> return dp;
> }
>
> -static char * __init of_pdt_build_full_name(struct device_node *dp)
> +static char * __init of_pdt_build_full_name(struct device_node *dp,
> + phandle node)
Is dp->phandle not suitable here?
> {
> int len, ourlen, plen;
> char *n;
>
> + /*
> + * When fetching the full name we want the name we see with
> + * package-to-path (ie, '/foo/bar/battery@0') rather than what
> + * we see with the name property (ie, 'battery').
> + */
> + n = of_pdt_try_pkg2path(node);
> + if (n)
> + return n;
> +
> + /* Older method for determining full name */
> plen = strlen(dp->parent->full_name);
> ourlen = strlen(of_pdt_node_name(dp));
> len = ourlen + plen + 2;
> @@ -243,7 +231,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
> #if defined(CONFIG_SPARC)
> dp->path_component_name = build_path_component(dp);
> #endif
I still think it would be useful to remove the #if
defined(CONFIG_SPARC) from path_component_name, and it might be the
best way to solve my comment about broken of_pdt_node_name above.
> - dp->full_name = of_pdt_build_full_name(dp);
> + dp->full_name = of_pdt_build_full_name(dp, node);
>
> dp->child = of_pdt_build_tree(dp,
> of_pdt_prom_ops->getchild(node), nextp);
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2)
@ 2011-02-23 23:28 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-23 23:28 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
>
> Commit e2f2a93b changed dp->name from using the 'name' property to
> using package-to-path. This fixed /proc/device-tree creation by
> eliminating conflicts between names (the 'name' property provides
> names like 'battery', whereas package-to-path provides names like
> '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> it also breaks of_device_id table matching.
>
> The fix that we _really_ wanted was to keep dp->name based upon
> the name property ('battery'), but based dp->full_name upon
> package-to-path ('battery@0'). This patch does just that.
>
> This also changes OLPC behavior to use the full result from
> package-to-path for full_name, rather than stripping the directory
> out. In practice, the strings end up being exactly the same; this
> change saves time, code, and memory.
>
> Note that this affects sparc by reverting dp->name back to what
> sparc was originally doing (looking at the name property).
>
> v2: combine two patches and revert of_pdt_node_name to original version.
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
Hi Andres, comments below.
g.
> ---
> drivers/of/pdt.c | 42 +++++++++++++++---------------------------
> 1 files changed, 15 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..a7aa85e 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
>
> static char * __init of_pdt_try_pkg2path(phandle node)
> {
> - char *res, *buf = NULL;
> + char *buf = NULL;
> int len;
>
> if (!of_pdt_prom_ops->pkg2path)
> @@ -147,29 +147,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
> pr_err("%s: package-to-path failed\n", __func__);
> return NULL;
> }
> -
> - res = strrchr(buf, '/');
> - if (!res) {
> - pr_err("%s: couldn't find / in %s\n", __func__, buf);
> - return NULL;
> - }
> - return res+1;
> -}
> -
> -/*
> - * When fetching the node's name, first try using package-to-path; if
> - * that fails (either because the arch hasn't supplied a PROM callback,
> - * or some other random failure), fall back to just looking at the node's
> - * 'name' property.
> - */
> -static char * __init of_pdt_build_name(phandle node)
> -{
> - char *buf;
> -
> - buf = of_pdt_try_pkg2path(node);
> - if (!buf)
> - buf = of_pdt_get_one_property(node, "name");
> -
> return buf;
> }
>
It seems to me that of_pdt_build_full_name will still be missing the
'@<addr>' component on non-sparc non-olpc builds because it uses the
broken of_pdt_node_name(). That needs to be fixed too, even if there
are no current users (or removed).
> @@ -187,7 +164,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
>
> kref_init(&dp->kref);
>
> - dp->name = of_pdt_build_name(node);
> + dp->name = of_pdt_get_one_property(node, "name");
> dp->type = of_pdt_get_one_property(node, "device_type");
> dp->phandle = node;
>
> @@ -198,11 +175,22 @@ static struct device_node * __init of_pdt_create_node(phandle node,
> return dp;
> }
>
> -static char * __init of_pdt_build_full_name(struct device_node *dp)
> +static char * __init of_pdt_build_full_name(struct device_node *dp,
> + phandle node)
Is dp->phandle not suitable here?
> {
> int len, ourlen, plen;
> char *n;
>
> + /*
> + * When fetching the full name we want the name we see with
> + * package-to-path (ie, '/foo/bar/battery@0') rather than what
> + * we see with the name property (ie, 'battery').
> + */
> + n = of_pdt_try_pkg2path(node);
> + if (n)
> + return n;
> +
> + /* Older method for determining full name */
> plen = strlen(dp->parent->full_name);
> ourlen = strlen(of_pdt_node_name(dp));
> len = ourlen + plen + 2;
> @@ -243,7 +231,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
> #if defined(CONFIG_SPARC)
> dp->path_component_name = build_path_component(dp);
> #endif
I still think it would be useful to remove the #if
defined(CONFIG_SPARC) from path_component_name, and it might be the
best way to solve my comment about broken of_pdt_node_name above.
> - dp->full_name = of_pdt_build_full_name(dp);
> + dp->full_name = of_pdt_build_full_name(dp, node);
>
> dp->child = of_pdt_build_tree(dp,
> of_pdt_prom_ops->getchild(node), nextp);
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-23 23:28 ` Grant Likely
@ 2011-02-24 0:16 ` Andres Salomon
-1 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 0:16 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, 23 Feb 2011 16:28:15 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
> >
> > Commit e2f2a93b changed dp->name from using the 'name' property to
> > using package-to-path. This fixed /proc/device-tree creation by
> > eliminating conflicts between names (the 'name' property provides
> > names like 'battery', whereas package-to-path provides names like
> > '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> > it also breaks of_device_id table matching.
> >
> > The fix that we _really_ wanted was to keep dp->name based upon
> > the name property ('battery'), but based dp->full_name upon
> > package-to-path ('battery@0'). This patch does just that.
> >
> > This also changes OLPC behavior to use the full result from
> > package-to-path for full_name, rather than stripping the directory
> > out. In practice, the strings end up being exactly the same; this
> > change saves time, code, and memory.
> >
> > Note that this affects sparc by reverting dp->name back to what
> > sparc was originally doing (looking at the name property).
> >
> > v2: combine two patches and revert of_pdt_node_name to original
> > version.
> >
> > Signed-off-by: Andres Salomon <dilinger@queued.net>
>
> Hi Andres, comments below.
>
> g.
>
> > ---
> > drivers/of/pdt.c | 42 +++++++++++++++---------------------------
> > 1 files changed, 15 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > index 28295d0..a7aa85e 100644
> > --- a/drivers/of/pdt.c
> > +++ b/drivers/of/pdt.c
> > @@ -134,7 +134,7 @@ static char * __init
> > of_pdt_get_one_property(phandle node, const char *name)
> > static char * __init of_pdt_try_pkg2path(phandle node)
> > {
> > - char *res, *buf = NULL;
> > + char *buf = NULL;
> > int len;
> >
> > if (!of_pdt_prom_ops->pkg2path)
> > @@ -147,29 +147,6 @@ static char * __init
> > of_pdt_try_pkg2path(phandle node) pr_err("%s: package-to-path
> > failed\n", __func__); return NULL;
> > }
> > -
> > - res = strrchr(buf, '/');
> > - if (!res) {
> > - pr_err("%s: couldn't find / in %s\n", __func__,
> > buf);
> > - return NULL;
> > - }
> > - return res+1;
> > -}
> > -
> > -/*
> > - * When fetching the node's name, first try using package-to-path;
> > if
> > - * that fails (either because the arch hasn't supplied a PROM
> > callback,
> > - * or some other random failure), fall back to just looking at the
> > node's
> > - * 'name' property.
> > - */
> > -static char * __init of_pdt_build_name(phandle node)
> > -{
> > - char *buf;
> > -
> > - buf = of_pdt_try_pkg2path(node);
> > - if (!buf)
> > - buf = of_pdt_get_one_property(node, "name");
> > -
> > return buf;
> > }
> >
>
> It seems to me that of_pdt_build_full_name will still be missing the
> '@<addr>' component on non-sparc non-olpc builds because it uses the
> broken of_pdt_node_name(). That needs to be fixed too, even if there
> are no current users (or removed).
The intent if for them to use the pkg2path hook. If
they can't use that, they'll need an architecture-specific way to
figure out the @<addr> component. It may even be possible for sparc
to use package-to-path; but either way, if an architecture doesn't
have package-to-path (OLPC and powerpc have it; I don't know about
sparc), and can't fake it, it'll need to do something special.
If the pkg2path hook isn't set and we're not sparc, we fall back to
dp->name. That sucks, but I don't know of a better way to do things.
>
> > @@ -187,7 +164,7 @@ static struct device_node * __init
> > of_pdt_create_node(phandle node,
> > kref_init(&dp->kref);
> >
> > - dp->name = of_pdt_build_name(node);
> > + dp->name = of_pdt_get_one_property(node, "name");
> > dp->type = of_pdt_get_one_property(node, "device_type");
> > dp->phandle = node;
> >
> > @@ -198,11 +175,22 @@ static struct device_node * __init
> > of_pdt_create_node(phandle node, return dp;
> > }
> >
> > -static char * __init of_pdt_build_full_name(struct device_node *dp)
> > +static char * __init of_pdt_build_full_name(struct device_node *dp,
> > + phandle node)
>
> Is dp->phandle not suitable here?
>
> > {
> > int len, ourlen, plen;
> > char *n;
> >
> > + /*
> > + * When fetching the full name we want the name we see with
> > + * package-to-path (ie, '/foo/bar/battery@0') rather than
> > what
> > + * we see with the name property (ie, 'battery').
> > + */
> > + n = of_pdt_try_pkg2path(node);
> > + if (n)
> > + return n;
> > +
> > + /* Older method for determining full name */
> > plen = strlen(dp->parent->full_name);
> > ourlen = strlen(of_pdt_node_name(dp));
> > len = ourlen + plen + 2;
> > @@ -243,7 +231,7 @@ static struct device_node * __init
> > of_pdt_build_tree(struct device_node *parent, #if
> > defined(CONFIG_SPARC) dp->path_component_name > > build_path_component(dp); #endif
>
> I still think it would be useful to remove the #if
> defined(CONFIG_SPARC) from path_component_name, and it might be the
> best way to solve my comment about broken of_pdt_node_name above.
>
path_component_name is filled in by build_path_component() in
sparc-land. It's very sparc-specific, and having path_component_name
around doesn't help us on other architectures if we don't know how to
fill it in.
> > - dp->full_name = of_pdt_build_full_name(dp);
> > + dp->full_name = of_pdt_build_full_name(dp, node);
> >
> > dp->child = of_pdt_build_tree(dp,
> > of_pdt_prom_ops->getchild(node),
> > nextp); --
> > 1.7.2.3
> >
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2)
@ 2011-02-24 0:16 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 0:16 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, 23 Feb 2011 16:28:15 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
> >
> > Commit e2f2a93b changed dp->name from using the 'name' property to
> > using package-to-path. This fixed /proc/device-tree creation by
> > eliminating conflicts between names (the 'name' property provides
> > names like 'battery', whereas package-to-path provides names like
> > '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> > it also breaks of_device_id table matching.
> >
> > The fix that we _really_ wanted was to keep dp->name based upon
> > the name property ('battery'), but based dp->full_name upon
> > package-to-path ('battery@0'). This patch does just that.
> >
> > This also changes OLPC behavior to use the full result from
> > package-to-path for full_name, rather than stripping the directory
> > out. In practice, the strings end up being exactly the same; this
> > change saves time, code, and memory.
> >
> > Note that this affects sparc by reverting dp->name back to what
> > sparc was originally doing (looking at the name property).
> >
> > v2: combine two patches and revert of_pdt_node_name to original
> > version.
> >
> > Signed-off-by: Andres Salomon <dilinger@queued.net>
>
> Hi Andres, comments below.
>
> g.
>
> > ---
> > drivers/of/pdt.c | 42 +++++++++++++++---------------------------
> > 1 files changed, 15 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > index 28295d0..a7aa85e 100644
> > --- a/drivers/of/pdt.c
> > +++ b/drivers/of/pdt.c
> > @@ -134,7 +134,7 @@ static char * __init
> > of_pdt_get_one_property(phandle node, const char *name)
> > static char * __init of_pdt_try_pkg2path(phandle node)
> > {
> > - char *res, *buf = NULL;
> > + char *buf = NULL;
> > int len;
> >
> > if (!of_pdt_prom_ops->pkg2path)
> > @@ -147,29 +147,6 @@ static char * __init
> > of_pdt_try_pkg2path(phandle node) pr_err("%s: package-to-path
> > failed\n", __func__); return NULL;
> > }
> > -
> > - res = strrchr(buf, '/');
> > - if (!res) {
> > - pr_err("%s: couldn't find / in %s\n", __func__,
> > buf);
> > - return NULL;
> > - }
> > - return res+1;
> > -}
> > -
> > -/*
> > - * When fetching the node's name, first try using package-to-path;
> > if
> > - * that fails (either because the arch hasn't supplied a PROM
> > callback,
> > - * or some other random failure), fall back to just looking at the
> > node's
> > - * 'name' property.
> > - */
> > -static char * __init of_pdt_build_name(phandle node)
> > -{
> > - char *buf;
> > -
> > - buf = of_pdt_try_pkg2path(node);
> > - if (!buf)
> > - buf = of_pdt_get_one_property(node, "name");
> > -
> > return buf;
> > }
> >
>
> It seems to me that of_pdt_build_full_name will still be missing the
> '@<addr>' component on non-sparc non-olpc builds because it uses the
> broken of_pdt_node_name(). That needs to be fixed too, even if there
> are no current users (or removed).
The intent if for them to use the pkg2path hook. If
they can't use that, they'll need an architecture-specific way to
figure out the @<addr> component. It may even be possible for sparc
to use package-to-path; but either way, if an architecture doesn't
have package-to-path (OLPC and powerpc have it; I don't know about
sparc), and can't fake it, it'll need to do something special.
If the pkg2path hook isn't set and we're not sparc, we fall back to
dp->name. That sucks, but I don't know of a better way to do things.
>
> > @@ -187,7 +164,7 @@ static struct device_node * __init
> > of_pdt_create_node(phandle node,
> > kref_init(&dp->kref);
> >
> > - dp->name = of_pdt_build_name(node);
> > + dp->name = of_pdt_get_one_property(node, "name");
> > dp->type = of_pdt_get_one_property(node, "device_type");
> > dp->phandle = node;
> >
> > @@ -198,11 +175,22 @@ static struct device_node * __init
> > of_pdt_create_node(phandle node, return dp;
> > }
> >
> > -static char * __init of_pdt_build_full_name(struct device_node *dp)
> > +static char * __init of_pdt_build_full_name(struct device_node *dp,
> > + phandle node)
>
> Is dp->phandle not suitable here?
>
> > {
> > int len, ourlen, plen;
> > char *n;
> >
> > + /*
> > + * When fetching the full name we want the name we see with
> > + * package-to-path (ie, '/foo/bar/battery@0') rather than
> > what
> > + * we see with the name property (ie, 'battery').
> > + */
> > + n = of_pdt_try_pkg2path(node);
> > + if (n)
> > + return n;
> > +
> > + /* Older method for determining full name */
> > plen = strlen(dp->parent->full_name);
> > ourlen = strlen(of_pdt_node_name(dp));
> > len = ourlen + plen + 2;
> > @@ -243,7 +231,7 @@ static struct device_node * __init
> > of_pdt_build_tree(struct device_node *parent, #if
> > defined(CONFIG_SPARC) dp->path_component_name =
> > build_path_component(dp); #endif
>
> I still think it would be useful to remove the #if
> defined(CONFIG_SPARC) from path_component_name, and it might be the
> best way to solve my comment about broken of_pdt_node_name above.
>
path_component_name is filled in by build_path_component() in
sparc-land. It's very sparc-specific, and having path_component_name
around doesn't help us on other architectures if we don't know how to
fill it in.
> > - dp->full_name = of_pdt_build_full_name(dp);
> > + dp->full_name = of_pdt_build_full_name(dp, node);
> >
> > dp->child = of_pdt_build_tree(dp,
> > of_pdt_prom_ops->getchild(node),
> > nextp); --
> > 1.7.2.3
> >
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-24 0:16 ` [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2) Andres Salomon
@ 2011-02-24 4:06 ` Grant Likely
-1 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-24 4:06 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 04:16:30PM -0800, Andres Salomon wrote:
> On Wed, 23 Feb 2011 16:28:15 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
> > >
> > > Commit e2f2a93b changed dp->name from using the 'name' property to
> > > using package-to-path. This fixed /proc/device-tree creation by
> > > eliminating conflicts between names (the 'name' property provides
> > > names like 'battery', whereas package-to-path provides names like
> > > '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> > > it also breaks of_device_id table matching.
> > >
> > > The fix that we _really_ wanted was to keep dp->name based upon
> > > the name property ('battery'), but based dp->full_name upon
> > > package-to-path ('battery@0'). This patch does just that.
> > >
> > > This also changes OLPC behavior to use the full result from
> > > package-to-path for full_name, rather than stripping the directory
> > > out. In practice, the strings end up being exactly the same; this
> > > change saves time, code, and memory.
> > >
> > > Note that this affects sparc by reverting dp->name back to what
> > > sparc was originally doing (looking at the name property).
> > >
> > > v2: combine two patches and revert of_pdt_node_name to original
> > > version.
> > >
> > > Signed-off-by: Andres Salomon <dilinger@queued.net>
> >
> > Hi Andres, comments below.
> >
> > g.
> >
> > > ---
> > > drivers/of/pdt.c | 42 +++++++++++++++---------------------------
> > > 1 files changed, 15 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > > index 28295d0..a7aa85e 100644
> > > --- a/drivers/of/pdt.c
> > > +++ b/drivers/of/pdt.c
> > > @@ -134,7 +134,7 @@ static char * __init
> > > of_pdt_get_one_property(phandle node, const char *name)
> > > static char * __init of_pdt_try_pkg2path(phandle node)
> > > {
> > > - char *res, *buf = NULL;
> > > + char *buf = NULL;
> > > int len;
> > >
> > > if (!of_pdt_prom_ops->pkg2path)
> > > @@ -147,29 +147,6 @@ static char * __init
> > > of_pdt_try_pkg2path(phandle node) pr_err("%s: package-to-path
> > > failed\n", __func__); return NULL;
> > > }
> > > -
> > > - res = strrchr(buf, '/');
> > > - if (!res) {
> > > - pr_err("%s: couldn't find / in %s\n", __func__,
> > > buf);
> > > - return NULL;
> > > - }
> > > - return res+1;
> > > -}
> > > -
> > > -/*
> > > - * When fetching the node's name, first try using package-to-path;
> > > if
> > > - * that fails (either because the arch hasn't supplied a PROM
> > > callback,
> > > - * or some other random failure), fall back to just looking at the
> > > node's
> > > - * 'name' property.
> > > - */
> > > -static char * __init of_pdt_build_name(phandle node)
> > > -{
> > > - char *buf;
> > > -
> > > - buf = of_pdt_try_pkg2path(node);
> > > - if (!buf)
> > > - buf = of_pdt_get_one_property(node, "name");
> > > -
> > > return buf;
> > > }
> > >
> >
> > It seems to me that of_pdt_build_full_name will still be missing the
> > '@<addr>' component on non-sparc non-olpc builds because it uses the
> > broken of_pdt_node_name(). That needs to be fixed too, even if there
> > are no current users (or removed).
>
>
> The intent if for them to use the pkg2path hook. If
> they can't use that, they'll need an architecture-specific way to
> figure out the @<addr> component. It may even be possible for sparc
> to use package-to-path; but either way, if an architecture doesn't
> have package-to-path (OLPC and powerpc have it; I don't know about
> sparc), and can't fake it, it'll need to do something special.
>
> If the pkg2path hook isn't set and we're not sparc, we fall back to
> dp->name. That sucks, but I don't know of a better way to do things.
More that sucks, it is just plain *wrong*. :-)
so, to sum up, of_pdt_build_tree goes through the following process:
1) dp = of_pdt_create_node
- dp->name = of_pdt_get_one_property(node, "name"); /* name w/o addr */
2) (SPARC) dp->path_component_name = build_path_component(dp);
- format <node name>@<address>
- uses dp->name value
- not on OLPC
3) dp->full_name = of_pdt_build_full_name(dp)
- (SPARC) use dp->path_component_name
- (OLPC) depend on value from of_pdt_try_pkg2path(node);
- (others) fake it with an incorrect value?
Am I correct?
Faking it is clearly not acceptable. The solution is to *force* all
platforms to implement a method for obtaining the full path. That
means BUG() or WARN() if pkg2path is not populated, or if it returns
NULL. It also means no mucking about with of_pdt_try_pkg2path().
Call ops->pkg2path directly and complain loudly when it does not work.
I see two choices:
1) implement ops->pkg2path for SPARC, but back it with
build_path_component() instead of a call to OF
2) #ifdef of_pdt_build_full_name() to use the current behaviour for
SPARC, but call pkg2path for everyone else.
Actually, I'd like to see build_path_component() refactored to return
the full_name instead. It is trivial to get the path component from
the full name at any time, but it is not trivial to go the other
direction.... but that is probably more surgery than should be done in
this immediate bug-fix. I'm assuming a fix is necessary before 2.6.38
is finalized?
g.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2)
@ 2011-02-24 4:06 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-24 4:06 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 04:16:30PM -0800, Andres Salomon wrote:
> On Wed, 23 Feb 2011 16:28:15 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
> > >
> > > Commit e2f2a93b changed dp->name from using the 'name' property to
> > > using package-to-path. This fixed /proc/device-tree creation by
> > > eliminating conflicts between names (the 'name' property provides
> > > names like 'battery', whereas package-to-path provides names like
> > > '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> > > it also breaks of_device_id table matching.
> > >
> > > The fix that we _really_ wanted was to keep dp->name based upon
> > > the name property ('battery'), but based dp->full_name upon
> > > package-to-path ('battery@0'). This patch does just that.
> > >
> > > This also changes OLPC behavior to use the full result from
> > > package-to-path for full_name, rather than stripping the directory
> > > out. In practice, the strings end up being exactly the same; this
> > > change saves time, code, and memory.
> > >
> > > Note that this affects sparc by reverting dp->name back to what
> > > sparc was originally doing (looking at the name property).
> > >
> > > v2: combine two patches and revert of_pdt_node_name to original
> > > version.
> > >
> > > Signed-off-by: Andres Salomon <dilinger@queued.net>
> >
> > Hi Andres, comments below.
> >
> > g.
> >
> > > ---
> > > drivers/of/pdt.c | 42 +++++++++++++++---------------------------
> > > 1 files changed, 15 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > > index 28295d0..a7aa85e 100644
> > > --- a/drivers/of/pdt.c
> > > +++ b/drivers/of/pdt.c
> > > @@ -134,7 +134,7 @@ static char * __init
> > > of_pdt_get_one_property(phandle node, const char *name)
> > > static char * __init of_pdt_try_pkg2path(phandle node)
> > > {
> > > - char *res, *buf = NULL;
> > > + char *buf = NULL;
> > > int len;
> > >
> > > if (!of_pdt_prom_ops->pkg2path)
> > > @@ -147,29 +147,6 @@ static char * __init
> > > of_pdt_try_pkg2path(phandle node) pr_err("%s: package-to-path
> > > failed\n", __func__); return NULL;
> > > }
> > > -
> > > - res = strrchr(buf, '/');
> > > - if (!res) {
> > > - pr_err("%s: couldn't find / in %s\n", __func__,
> > > buf);
> > > - return NULL;
> > > - }
> > > - return res+1;
> > > -}
> > > -
> > > -/*
> > > - * When fetching the node's name, first try using package-to-path;
> > > if
> > > - * that fails (either because the arch hasn't supplied a PROM
> > > callback,
> > > - * or some other random failure), fall back to just looking at the
> > > node's
> > > - * 'name' property.
> > > - */
> > > -static char * __init of_pdt_build_name(phandle node)
> > > -{
> > > - char *buf;
> > > -
> > > - buf = of_pdt_try_pkg2path(node);
> > > - if (!buf)
> > > - buf = of_pdt_get_one_property(node, "name");
> > > -
> > > return buf;
> > > }
> > >
> >
> > It seems to me that of_pdt_build_full_name will still be missing the
> > '@<addr>' component on non-sparc non-olpc builds because it uses the
> > broken of_pdt_node_name(). That needs to be fixed too, even if there
> > are no current users (or removed).
>
>
> The intent if for them to use the pkg2path hook. If
> they can't use that, they'll need an architecture-specific way to
> figure out the @<addr> component. It may even be possible for sparc
> to use package-to-path; but either way, if an architecture doesn't
> have package-to-path (OLPC and powerpc have it; I don't know about
> sparc), and can't fake it, it'll need to do something special.
>
> If the pkg2path hook isn't set and we're not sparc, we fall back to
> dp->name. That sucks, but I don't know of a better way to do things.
More that sucks, it is just plain *wrong*. :-)
so, to sum up, of_pdt_build_tree goes through the following process:
1) dp = of_pdt_create_node
- dp->name = of_pdt_get_one_property(node, "name"); /* name w/o addr */
2) (SPARC) dp->path_component_name = build_path_component(dp);
- format <node name>@<address>
- uses dp->name value
- not on OLPC
3) dp->full_name = of_pdt_build_full_name(dp)
- (SPARC) use dp->path_component_name
- (OLPC) depend on value from of_pdt_try_pkg2path(node);
- (others) fake it with an incorrect value?
Am I correct?
Faking it is clearly not acceptable. The solution is to *force* all
platforms to implement a method for obtaining the full path. That
means BUG() or WARN() if pkg2path is not populated, or if it returns
NULL. It also means no mucking about with of_pdt_try_pkg2path().
Call ops->pkg2path directly and complain loudly when it does not work.
I see two choices:
1) implement ops->pkg2path for SPARC, but back it with
build_path_component() instead of a call to OF
2) #ifdef of_pdt_build_full_name() to use the current behaviour for
SPARC, but call pkg2path for everyone else.
Actually, I'd like to see build_path_component() refactored to return
the full_name instead. It is trivial to get the path component from
the full name at any time, but it is not trivial to go the other
direction.... but that is probably more surgery than should be done in
this immediate bug-fix. I'm assuming a fix is necessary before 2.6.38
is finalized?
g.
^ permalink raw reply [flat|nested] 51+ messages in thread[parent not found: <20110224040638.GB22111-MrY2KI0G/OVr83L8+7iqerDks+cytr/Z@public.gmane.org>]
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-24 4:06 ` [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2) Grant Likely
(?)
@ 2011-02-24 4:36 ` Andres Salomon
-1 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 4:36 UTC (permalink / raw)
To: Grant Likely
Cc: sparclinux-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David S. Miller,
Daniel Drake, linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Wed, 23 Feb 2011 21:06:38 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 23, 2011 at 04:16:30PM -0800, Andres Salomon wrote:
> > On Wed, 23 Feb 2011 16:28:15 -0700
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >
> > > On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
> > > >
> > > > Commit e2f2a93b changed dp->name from using the 'name' property
> > > > to using package-to-path. This fixed /proc/device-tree
> > > > creation by eliminating conflicts between names (the 'name'
> > > > property provides names like 'battery', whereas package-to-path
> > > > provides names like '/foo/bar/battery@0', which we stripped to
> > > > 'battery@0'). However, it also breaks of_device_id table
> > > > matching.
> > > >
> > > > The fix that we _really_ wanted was to keep dp->name based upon
> > > > the name property ('battery'), but based dp->full_name upon
> > > > package-to-path ('battery@0'). This patch does just that.
> > > >
> > > > This also changes OLPC behavior to use the full result from
> > > > package-to-path for full_name, rather than stripping the
> > > > directory out. In practice, the strings end up being exactly
> > > > the same; this change saves time, code, and memory.
> > > >
> > > > Note that this affects sparc by reverting dp->name back to what
> > > > sparc was originally doing (looking at the name property).
> > > >
> > > > v2: combine two patches and revert of_pdt_node_name to original
> > > > version.
> > > >
> > > > Signed-off-by: Andres Salomon <dilinger@queued.net>
> > >
> > > Hi Andres, comments below.
> > >
> > > g.
> > >
> > > > ---
> > > > drivers/of/pdt.c | 42
> > > > +++++++++++++++--------------------------- 1 files changed, 15
> > > > insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > > > index 28295d0..a7aa85e 100644
> > > > --- a/drivers/of/pdt.c
> > > > +++ b/drivers/of/pdt.c
> > > > @@ -134,7 +134,7 @@ static char * __init
> > > > of_pdt_get_one_property(phandle node, const char *name)
> > > > static char * __init of_pdt_try_pkg2path(phandle node)
> > > > {
> > > > - char *res, *buf = NULL;
> > > > + char *buf = NULL;
> > > > int len;
> > > >
> > > > if (!of_pdt_prom_ops->pkg2path)
> > > > @@ -147,29 +147,6 @@ static char * __init
> > > > of_pdt_try_pkg2path(phandle node) pr_err("%s: package-to-path
> > > > failed\n", __func__); return NULL;
> > > > }
> > > > -
> > > > - res = strrchr(buf, '/');
> > > > - if (!res) {
> > > > - pr_err("%s: couldn't find / in %s\n", __func__,
> > > > buf);
> > > > - return NULL;
> > > > - }
> > > > - return res+1;
> > > > -}
> > > > -
> > > > -/*
> > > > - * When fetching the node's name, first try using
> > > > package-to-path; if
> > > > - * that fails (either because the arch hasn't supplied a PROM
> > > > callback,
> > > > - * or some other random failure), fall back to just looking at
> > > > the node's
> > > > - * 'name' property.
> > > > - */
> > > > -static char * __init of_pdt_build_name(phandle node)
> > > > -{
> > > > - char *buf;
> > > > -
> > > > - buf = of_pdt_try_pkg2path(node);
> > > > - if (!buf)
> > > > - buf = of_pdt_get_one_property(node, "name");
> > > > -
> > > > return buf;
> > > > }
> > > >
> > >
> > > It seems to me that of_pdt_build_full_name will still be missing
> > > the '@<addr>' component on non-sparc non-olpc builds because it
> > > uses the broken of_pdt_node_name(). That needs to be fixed too,
> > > even if there are no current users (or removed).
> >
> >
> > The intent if for them to use the pkg2path hook. If
> > they can't use that, they'll need an architecture-specific way to
> > figure out the @<addr> component. It may even be possible for sparc
> > to use package-to-path; but either way, if an architecture doesn't
> > have package-to-path (OLPC and powerpc have it; I don't know about
> > sparc), and can't fake it, it'll need to do something special.
> >
> > If the pkg2path hook isn't set and we're not sparc, we fall back to
> > dp->name. That sucks, but I don't know of a better way to do
> > things.
>
> More that sucks, it is just plain *wrong*. :-)
>
> so, to sum up, of_pdt_build_tree goes through the following process:
>
> 1) dp = of_pdt_create_node
> - dp->name = of_pdt_get_one_property(node, "name"); /* name w/o
> addr */
>
> 2) (SPARC) dp->path_component_name = build_path_component(dp);
> - format <node name>@<address>
> - uses dp->name value
> - not on OLPC
Also:
- returns only the node name, not the full path
- implemented differently depending upon bus type (see
pci_path_component/sbus_path_component/ebus_path_component/ambapp_path_component)
- sparc32 implemented differently versus sparc64
>
> 3) dp->full_name = of_pdt_build_full_name(dp)
> - (SPARC) use dp->path_component_name
> - (OLPC) depend on value from of_pdt_try_pkg2path(node);
> - (others) fake it with an incorrect value?
>
> Am I correct?
No.
(others) use of_pdt_try_pkg2path
The reason why we fall back to dp->name is because I don't know what
other architectures out there might not have package-to-path. I'm
perfectly fine with falling back to a WARN or BUG.
>
> Faking it is clearly not acceptable. The solution is to *force* all
> platforms to implement a method for obtaining the full path. That
> means BUG() or WARN() if pkg2path is not populated, or if it returns
> NULL. It also means no mucking about with of_pdt_try_pkg2path().
> Call ops->pkg2path directly and complain loudly when it does not work.
>
> I see two choices:
> 1) implement ops->pkg2path for SPARC, but back it with
> build_path_component() instead of a call to OF
> 2) #ifdef of_pdt_build_full_name() to use the current behaviour for
> SPARC, but call pkg2path for everyone else.
Either of these are fine, but I don't think they should be within the
scope of the proposed patch. I'd like to hear from Davem about whether
#1 is doable. Also note that ops are different for sparc32 and sparc64.
>
> Actually, I'd like to see build_path_component() refactored to return
> the full_name instead. It is trivial to get the path component from
Again, this is Davem-land; I don't have a sparc machine to test any of
this with. The proposed patch is a bug fix that I'd like to see go in;
I'm perfectly happy to refactor the APIs after that.
> the full name at any time, but it is not trivial to go the other
> direction.... but that is probably more surgery than should be done in
> this immediate bug-fix. I'm assuming a fix is necessary before 2.6.38
> is finalized?
Well, a fix only affects Daniel's work. It does, however, fix a bug
in sparc code. I don't know if that breaks anything in practice.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2)
@ 2011-02-24 4:36 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 4:36 UTC (permalink / raw)
To: Grant Likely
Cc: sparclinux-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David S. Miller,
Daniel Drake, linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Wed, 23 Feb 2011 21:06:38 -0700
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Wed, Feb 23, 2011 at 04:16:30PM -0800, Andres Salomon wrote:
> > On Wed, 23 Feb 2011 16:28:15 -0700
> > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> >
> > > On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
> > > >
> > > > Commit e2f2a93b changed dp->name from using the 'name' property
> > > > to using package-to-path. This fixed /proc/device-tree
> > > > creation by eliminating conflicts between names (the 'name'
> > > > property provides names like 'battery', whereas package-to-path
> > > > provides names like '/foo/bar/battery@0', which we stripped to
> > > > 'battery@0'). However, it also breaks of_device_id table
> > > > matching.
> > > >
> > > > The fix that we _really_ wanted was to keep dp->name based upon
> > > > the name property ('battery'), but based dp->full_name upon
> > > > package-to-path ('battery@0'). This patch does just that.
> > > >
> > > > This also changes OLPC behavior to use the full result from
> > > > package-to-path for full_name, rather than stripping the
> > > > directory out. In practice, the strings end up being exactly
> > > > the same; this change saves time, code, and memory.
> > > >
> > > > Note that this affects sparc by reverting dp->name back to what
> > > > sparc was originally doing (looking at the name property).
> > > >
> > > > v2: combine two patches and revert of_pdt_node_name to original
> > > > version.
> > > >
> > > > Signed-off-by: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
> > >
> > > Hi Andres, comments below.
> > >
> > > g.
> > >
> > > > ---
> > > > drivers/of/pdt.c | 42
> > > > +++++++++++++++--------------------------- 1 files changed, 15
> > > > insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > > > index 28295d0..a7aa85e 100644
> > > > --- a/drivers/of/pdt.c
> > > > +++ b/drivers/of/pdt.c
> > > > @@ -134,7 +134,7 @@ static char * __init
> > > > of_pdt_get_one_property(phandle node, const char *name)
> > > > static char * __init of_pdt_try_pkg2path(phandle node)
> > > > {
> > > > - char *res, *buf = NULL;
> > > > + char *buf = NULL;
> > > > int len;
> > > >
> > > > if (!of_pdt_prom_ops->pkg2path)
> > > > @@ -147,29 +147,6 @@ static char * __init
> > > > of_pdt_try_pkg2path(phandle node) pr_err("%s: package-to-path
> > > > failed\n", __func__); return NULL;
> > > > }
> > > > -
> > > > - res = strrchr(buf, '/');
> > > > - if (!res) {
> > > > - pr_err("%s: couldn't find / in %s\n", __func__,
> > > > buf);
> > > > - return NULL;
> > > > - }
> > > > - return res+1;
> > > > -}
> > > > -
> > > > -/*
> > > > - * When fetching the node's name, first try using
> > > > package-to-path; if
> > > > - * that fails (either because the arch hasn't supplied a PROM
> > > > callback,
> > > > - * or some other random failure), fall back to just looking at
> > > > the node's
> > > > - * 'name' property.
> > > > - */
> > > > -static char * __init of_pdt_build_name(phandle node)
> > > > -{
> > > > - char *buf;
> > > > -
> > > > - buf = of_pdt_try_pkg2path(node);
> > > > - if (!buf)
> > > > - buf = of_pdt_get_one_property(node, "name");
> > > > -
> > > > return buf;
> > > > }
> > > >
> > >
> > > It seems to me that of_pdt_build_full_name will still be missing
> > > the '@<addr>' component on non-sparc non-olpc builds because it
> > > uses the broken of_pdt_node_name(). That needs to be fixed too,
> > > even if there are no current users (or removed).
> >
> >
> > The intent if for them to use the pkg2path hook. If
> > they can't use that, they'll need an architecture-specific way to
> > figure out the @<addr> component. It may even be possible for sparc
> > to use package-to-path; but either way, if an architecture doesn't
> > have package-to-path (OLPC and powerpc have it; I don't know about
> > sparc), and can't fake it, it'll need to do something special.
> >
> > If the pkg2path hook isn't set and we're not sparc, we fall back to
> > dp->name. That sucks, but I don't know of a better way to do
> > things.
>
> More that sucks, it is just plain *wrong*. :-)
>
> so, to sum up, of_pdt_build_tree goes through the following process:
>
> 1) dp = of_pdt_create_node
> - dp->name = of_pdt_get_one_property(node, "name"); /* name w/o
> addr */
>
> 2) (SPARC) dp->path_component_name = build_path_component(dp);
> - format <node name>@<address>
> - uses dp->name value
> - not on OLPC
Also:
- returns only the node name, not the full path
- implemented differently depending upon bus type (see
pci_path_component/sbus_path_component/ebus_path_component/ambapp_path_component)
- sparc32 implemented differently versus sparc64
>
> 3) dp->full_name = of_pdt_build_full_name(dp)
> - (SPARC) use dp->path_component_name
> - (OLPC) depend on value from of_pdt_try_pkg2path(node);
> - (others) fake it with an incorrect value?
>
> Am I correct?
No.
(others) use of_pdt_try_pkg2path
The reason why we fall back to dp->name is because I don't know what
other architectures out there might not have package-to-path. I'm
perfectly fine with falling back to a WARN or BUG.
>
> Faking it is clearly not acceptable. The solution is to *force* all
> platforms to implement a method for obtaining the full path. That
> means BUG() or WARN() if pkg2path is not populated, or if it returns
> NULL. It also means no mucking about with of_pdt_try_pkg2path().
> Call ops->pkg2path directly and complain loudly when it does not work.
>
> I see two choices:
> 1) implement ops->pkg2path for SPARC, but back it with
> build_path_component() instead of a call to OF
> 2) #ifdef of_pdt_build_full_name() to use the current behaviour for
> SPARC, but call pkg2path for everyone else.
Either of these are fine, but I don't think they should be within the
scope of the proposed patch. I'd like to hear from Davem about whether
#1 is doable. Also note that ops are different for sparc32 and sparc64.
>
> Actually, I'd like to see build_path_component() refactored to return
> the full_name instead. It is trivial to get the path component from
Again, this is Davem-land; I don't have a sparc machine to test any of
this with. The proposed patch is a bug fix that I'd like to see go in;
I'm perfectly happy to refactor the APIs after that.
> the full name at any time, but it is not trivial to go the other
> direction.... but that is probably more surgery than should be done in
> this immediate bug-fix. I'm assuming a fix is necessary before 2.6.38
> is finalized?
Well, a fix only affects Daniel's work. It does, however, fix a bug
in sparc code. I don't know if that breaks anything in practice.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2)
@ 2011-02-24 4:36 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 4:36 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, 23 Feb 2011 21:06:38 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 23, 2011 at 04:16:30PM -0800, Andres Salomon wrote:
> > On Wed, 23 Feb 2011 16:28:15 -0700
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >
> > > On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
> > > >
> > > > Commit e2f2a93b changed dp->name from using the 'name' property
> > > > to using package-to-path. This fixed /proc/device-tree
> > > > creation by eliminating conflicts between names (the 'name'
> > > > property provides names like 'battery', whereas package-to-path
> > > > provides names like '/foo/bar/battery@0', which we stripped to
> > > > 'battery@0'). However, it also breaks of_device_id table
> > > > matching.
> > > >
> > > > The fix that we _really_ wanted was to keep dp->name based upon
> > > > the name property ('battery'), but based dp->full_name upon
> > > > package-to-path ('battery@0'). This patch does just that.
> > > >
> > > > This also changes OLPC behavior to use the full result from
> > > > package-to-path for full_name, rather than stripping the
> > > > directory out. In practice, the strings end up being exactly
> > > > the same; this change saves time, code, and memory.
> > > >
> > > > Note that this affects sparc by reverting dp->name back to what
> > > > sparc was originally doing (looking at the name property).
> > > >
> > > > v2: combine two patches and revert of_pdt_node_name to original
> > > > version.
> > > >
> > > > Signed-off-by: Andres Salomon <dilinger@queued.net>
> > >
> > > Hi Andres, comments below.
> > >
> > > g.
> > >
> > > > ---
> > > > drivers/of/pdt.c | 42
> > > > +++++++++++++++--------------------------- 1 files changed, 15
> > > > insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > > > index 28295d0..a7aa85e 100644
> > > > --- a/drivers/of/pdt.c
> > > > +++ b/drivers/of/pdt.c
> > > > @@ -134,7 +134,7 @@ static char * __init
> > > > of_pdt_get_one_property(phandle node, const char *name)
> > > > static char * __init of_pdt_try_pkg2path(phandle node)
> > > > {
> > > > - char *res, *buf = NULL;
> > > > + char *buf = NULL;
> > > > int len;
> > > >
> > > > if (!of_pdt_prom_ops->pkg2path)
> > > > @@ -147,29 +147,6 @@ static char * __init
> > > > of_pdt_try_pkg2path(phandle node) pr_err("%s: package-to-path
> > > > failed\n", __func__); return NULL;
> > > > }
> > > > -
> > > > - res = strrchr(buf, '/');
> > > > - if (!res) {
> > > > - pr_err("%s: couldn't find / in %s\n", __func__,
> > > > buf);
> > > > - return NULL;
> > > > - }
> > > > - return res+1;
> > > > -}
> > > > -
> > > > -/*
> > > > - * When fetching the node's name, first try using
> > > > package-to-path; if
> > > > - * that fails (either because the arch hasn't supplied a PROM
> > > > callback,
> > > > - * or some other random failure), fall back to just looking at
> > > > the node's
> > > > - * 'name' property.
> > > > - */
> > > > -static char * __init of_pdt_build_name(phandle node)
> > > > -{
> > > > - char *buf;
> > > > -
> > > > - buf = of_pdt_try_pkg2path(node);
> > > > - if (!buf)
> > > > - buf = of_pdt_get_one_property(node, "name");
> > > > -
> > > > return buf;
> > > > }
> > > >
> > >
> > > It seems to me that of_pdt_build_full_name will still be missing
> > > the '@<addr>' component on non-sparc non-olpc builds because it
> > > uses the broken of_pdt_node_name(). That needs to be fixed too,
> > > even if there are no current users (or removed).
> >
> >
> > The intent if for them to use the pkg2path hook. If
> > they can't use that, they'll need an architecture-specific way to
> > figure out the @<addr> component. It may even be possible for sparc
> > to use package-to-path; but either way, if an architecture doesn't
> > have package-to-path (OLPC and powerpc have it; I don't know about
> > sparc), and can't fake it, it'll need to do something special.
> >
> > If the pkg2path hook isn't set and we're not sparc, we fall back to
> > dp->name. That sucks, but I don't know of a better way to do
> > things.
>
> More that sucks, it is just plain *wrong*. :-)
>
> so, to sum up, of_pdt_build_tree goes through the following process:
>
> 1) dp = of_pdt_create_node
> - dp->name = of_pdt_get_one_property(node, "name"); /* name w/o
> addr */
>
> 2) (SPARC) dp->path_component_name = build_path_component(dp);
> - format <node name>@<address>
> - uses dp->name value
> - not on OLPC
Also:
- returns only the node name, not the full path
- implemented differently depending upon bus type (see
pci_path_component/sbus_path_component/ebus_path_component/ambapp_path_component)
- sparc32 implemented differently versus sparc64
>
> 3) dp->full_name = of_pdt_build_full_name(dp)
> - (SPARC) use dp->path_component_name
> - (OLPC) depend on value from of_pdt_try_pkg2path(node);
> - (others) fake it with an incorrect value?
>
> Am I correct?
No.
(others) use of_pdt_try_pkg2path
The reason why we fall back to dp->name is because I don't know what
other architectures out there might not have package-to-path. I'm
perfectly fine with falling back to a WARN or BUG.
>
> Faking it is clearly not acceptable. The solution is to *force* all
> platforms to implement a method for obtaining the full path. That
> means BUG() or WARN() if pkg2path is not populated, or if it returns
> NULL. It also means no mucking about with of_pdt_try_pkg2path().
> Call ops->pkg2path directly and complain loudly when it does not work.
>
> I see two choices:
> 1) implement ops->pkg2path for SPARC, but back it with
> build_path_component() instead of a call to OF
> 2) #ifdef of_pdt_build_full_name() to use the current behaviour for
> SPARC, but call pkg2path for everyone else.
Either of these are fine, but I don't think they should be within the
scope of the proposed patch. I'd like to hear from Davem about whether
#1 is doable. Also note that ops are different for sparc32 and sparc64.
>
> Actually, I'd like to see build_path_component() refactored to return
> the full_name instead. It is trivial to get the path component from
Again, this is Davem-land; I don't have a sparc machine to test any of
this with. The proposed patch is a bug fix that I'd like to see go in;
I'm perfectly happy to refactor the APIs after that.
> the full name at any time, but it is not trivial to go the other
> direction.... but that is probably more surgery than should be done in
> this immediate bug-fix. I'm assuming a fix is necessary before 2.6.38
> is finalized?
Well, a fix only affects Daniel's work. It does, however, fix a bug
in sparc code. I don't know if that breaks anything in practice.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-24 4:36 ` Andres Salomon
@ 2011-02-24 5:38 ` Grant Likely
-1 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-24 5:38 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 08:36:49PM -0800, Andres Salomon wrote:
> On Wed, 23 Feb 2011 21:06:38 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > > If the pkg2path hook isn't set and we're not sparc, we fall back to
> > > dp->name. That sucks, but I don't know of a better way to do
> > > things.
> >
> > More that sucks, it is just plain *wrong*. :-)
> >
> > so, to sum up, of_pdt_build_tree goes through the following process:
> >
> > 1) dp = of_pdt_create_node
> > - dp->name = of_pdt_get_one_property(node, "name"); /* name w/o
> > addr */
> >
> > 2) (SPARC) dp->path_component_name = build_path_component(dp);
> > - format <node name>@<address>
> > - uses dp->name value
> > - not on OLPC
>
> Also:
> - returns only the node name, not the full path
> - implemented differently depending upon bus type (see
> pci_path_component/sbus_path_component/ebus_path_component/ambapp_path_component)
> - sparc32 implemented differently versus sparc64
>
> >
> > 3) dp->full_name = of_pdt_build_full_name(dp)
> > - (SPARC) use dp->path_component_name
> > - (OLPC) depend on value from of_pdt_try_pkg2path(node);
> > - (others) fake it with an incorrect value?
> >
> > Am I correct?
>
> No.
>
> (others) use of_pdt_try_pkg2path
Regardless, in all cases the semantics are absolutely clear; all
platforms *must* have a reliable way of obtaining the full path and
the path_component_name. It it doesn't then it is fundamentally
broken.
> The reason why we fall back to dp->name is because I don't know what
> other architectures out there might not have package-to-path. I'm
> perfectly fine with falling back to a WARN or BUG.
Respin to fallback on WARN() and bail out. I'd be okay with merging a
patch that does that. Falling back to dp->name is absolutely incorrect.
I agree that API refinements can be deferred to 2.6.39.
g.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2)
@ 2011-02-24 5:38 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-24 5:38 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 08:36:49PM -0800, Andres Salomon wrote:
> On Wed, 23 Feb 2011 21:06:38 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > > If the pkg2path hook isn't set and we're not sparc, we fall back to
> > > dp->name. That sucks, but I don't know of a better way to do
> > > things.
> >
> > More that sucks, it is just plain *wrong*. :-)
> >
> > so, to sum up, of_pdt_build_tree goes through the following process:
> >
> > 1) dp = of_pdt_create_node
> > - dp->name = of_pdt_get_one_property(node, "name"); /* name w/o
> > addr */
> >
> > 2) (SPARC) dp->path_component_name = build_path_component(dp);
> > - format <node name>@<address>
> > - uses dp->name value
> > - not on OLPC
>
> Also:
> - returns only the node name, not the full path
> - implemented differently depending upon bus type (see
> pci_path_component/sbus_path_component/ebus_path_component/ambapp_path_component)
> - sparc32 implemented differently versus sparc64
>
> >
> > 3) dp->full_name = of_pdt_build_full_name(dp)
> > - (SPARC) use dp->path_component_name
> > - (OLPC) depend on value from of_pdt_try_pkg2path(node);
> > - (others) fake it with an incorrect value?
> >
> > Am I correct?
>
> No.
>
> (others) use of_pdt_try_pkg2path
Regardless, in all cases the semantics are absolutely clear; all
platforms *must* have a reliable way of obtaining the full path and
the path_component_name. It it doesn't then it is fundamentally
broken.
> The reason why we fall back to dp->name is because I don't know what
> other architectures out there might not have package-to-path. I'm
> perfectly fine with falling back to a WARN or BUG.
Respin to fallback on WARN() and bail out. I'd be okay with merging a
patch that does that. Falling back to dp->name is absolutely incorrect.
I agree that API refinements can be deferred to 2.6.39.
g.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-24 5:38 ` [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v2) Grant Likely
@ 2011-02-24 6:38 ` Andres Salomon
-1 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 6:38 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
Commit e2f2a93b changed dp->name from using the 'name' property to
using package-to-path. This fixed /proc/device-tree creation by
eliminating conflicts between names (the 'name' property provides
names like 'battery', whereas package-to-path provides names like
'/foo/bar/battery@0', which we stripped to 'battery@0'). However,
it also breaks of_device_id table matching.
The fix that we _really_ wanted was to keep dp->name based upon
the name property ('battery'), but based dp->full_name upon
package-to-path ('battery@0'). This patch does just that.
This also changes OLPC behavior to use the full result from
package-to-path for full_name, rather than stripping the directory
out. In practice, the strings end up being exactly the same; this
change saves time, code, and memory.
v2: combine two patches and revert of_pdt_node_name to original version
v3: use dp->phandle instead of passing around node
v4: warn/bail out for non-sparc archs if pkg2path is not set
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 50 ++++++++++++++++++++++----------------------------
1 files changed, 22 insertions(+), 28 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..61f6308 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -48,7 +48,9 @@ static inline void irq_trans_init(struct device_node *dp) { }
static inline const char *of_pdt_node_name(struct device_node *dp)
{
- return dp->name;
+ /* non-sparc archs should be setting a pkg2path hook */
+ WARN_ON(1);
+ return NULL;
}
#endif /* !CONFIG_SPARC */
@@ -134,7 +136,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
static char * __init of_pdt_try_pkg2path(phandle node)
{
- char *res, *buf = NULL;
+ char *buf = NULL;
int len;
if (!of_pdt_prom_ops->pkg2path)
@@ -147,29 +149,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
pr_err("%s: package-to-path failed\n", __func__);
return NULL;
}
-
- res = strrchr(buf, '/');
- if (!res) {
- pr_err("%s: couldn't find / in %s\n", __func__, buf);
- return NULL;
- }
- return res+1;
-}
-
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM callback,
- * or some other random failure), fall back to just looking at the node's
- * 'name' property.
- */
-static char * __init of_pdt_build_name(phandle node)
-{
- char *buf;
-
- buf = of_pdt_try_pkg2path(node);
- if (!buf)
- buf = of_pdt_get_one_property(node, "name");
-
return buf;
}
@@ -187,7 +166,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
kref_init(&dp->kref);
- dp->name = of_pdt_build_name(node);
+ dp->name = of_pdt_get_one_property(node, "name");
dp->type = of_pdt_get_one_property(node, "device_type");
dp->phandle = node;
@@ -201,10 +180,25 @@ static struct device_node * __init of_pdt_create_node(phandle node,
static char * __init of_pdt_build_full_name(struct device_node *dp)
{
int len, ourlen, plen;
+ const char *name;
char *n;
+ /*
+ * When fetching the full name we want the name we see with
+ * package-to-path (ie, '/foo/bar/battery@0') rather than what
+ * we see with the name property (ie, 'battery').
+ */
+ n = of_pdt_try_pkg2path(dp->phandle);
+ if (n)
+ return n;
+
+ name = of_pdt_node_name(dp);
+ if (!name)
+ return NULL;
+
+ /* Older method for determining full name */
plen = strlen(dp->parent->full_name);
- ourlen = strlen(of_pdt_node_name(dp));
+ ourlen = strlen(name);
len = ourlen + plen + 2;
n = prom_early_alloc(len);
@@ -213,7 +207,7 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
strcpy(n + plen, "/");
plen++;
}
- strcpy(n + plen, of_pdt_node_name(dp));
+ strcpy(n + plen, name);
return n;
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v4)
@ 2011-02-24 6:38 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 6:38 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
Commit e2f2a93b changed dp->name from using the 'name' property to
using package-to-path. This fixed /proc/device-tree creation by
eliminating conflicts between names (the 'name' property provides
names like 'battery', whereas package-to-path provides names like
'/foo/bar/battery@0', which we stripped to 'battery@0'). However,
it also breaks of_device_id table matching.
The fix that we _really_ wanted was to keep dp->name based upon
the name property ('battery'), but based dp->full_name upon
package-to-path ('battery@0'). This patch does just that.
This also changes OLPC behavior to use the full result from
package-to-path for full_name, rather than stripping the directory
out. In practice, the strings end up being exactly the same; this
change saves time, code, and memory.
v2: combine two patches and revert of_pdt_node_name to original version
v3: use dp->phandle instead of passing around node
v4: warn/bail out for non-sparc archs if pkg2path is not set
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 50 ++++++++++++++++++++++----------------------------
1 files changed, 22 insertions(+), 28 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..61f6308 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -48,7 +48,9 @@ static inline void irq_trans_init(struct device_node *dp) { }
static inline const char *of_pdt_node_name(struct device_node *dp)
{
- return dp->name;
+ /* non-sparc archs should be setting a pkg2path hook */
+ WARN_ON(1);
+ return NULL;
}
#endif /* !CONFIG_SPARC */
@@ -134,7 +136,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
static char * __init of_pdt_try_pkg2path(phandle node)
{
- char *res, *buf = NULL;
+ char *buf = NULL;
int len;
if (!of_pdt_prom_ops->pkg2path)
@@ -147,29 +149,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
pr_err("%s: package-to-path failed\n", __func__);
return NULL;
}
-
- res = strrchr(buf, '/');
- if (!res) {
- pr_err("%s: couldn't find / in %s\n", __func__, buf);
- return NULL;
- }
- return res+1;
-}
-
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM callback,
- * or some other random failure), fall back to just looking at the node's
- * 'name' property.
- */
-static char * __init of_pdt_build_name(phandle node)
-{
- char *buf;
-
- buf = of_pdt_try_pkg2path(node);
- if (!buf)
- buf = of_pdt_get_one_property(node, "name");
-
return buf;
}
@@ -187,7 +166,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
kref_init(&dp->kref);
- dp->name = of_pdt_build_name(node);
+ dp->name = of_pdt_get_one_property(node, "name");
dp->type = of_pdt_get_one_property(node, "device_type");
dp->phandle = node;
@@ -201,10 +180,25 @@ static struct device_node * __init of_pdt_create_node(phandle node,
static char * __init of_pdt_build_full_name(struct device_node *dp)
{
int len, ourlen, plen;
+ const char *name;
char *n;
+ /*
+ * When fetching the full name we want the name we see with
+ * package-to-path (ie, '/foo/bar/battery@0') rather than what
+ * we see with the name property (ie, 'battery').
+ */
+ n = of_pdt_try_pkg2path(dp->phandle);
+ if (n)
+ return n;
+
+ name = of_pdt_node_name(dp);
+ if (!name)
+ return NULL;
+
+ /* Older method for determining full name */
plen = strlen(dp->parent->full_name);
- ourlen = strlen(of_pdt_node_name(dp));
+ ourlen = strlen(name);
len = ourlen + plen + 2;
n = prom_early_alloc(len);
@@ -213,7 +207,7 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
strcpy(n + plen, "/");
plen++;
}
- strcpy(n + plen, of_pdt_node_name(dp));
+ strcpy(n + plen, name);
return n;
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 51+ messages in thread
* [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-23 23:28 ` Grant Likely
@ 2011-02-24 0:34 ` Andres Salomon
-1 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 0:34 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
Commit e2f2a93b changed dp->name from using the 'name' property to
using package-to-path. This fixed /proc/device-tree creation by
eliminating conflicts between names (the 'name' property provides
names like 'battery', whereas package-to-path provides names like
'/foo/bar/battery@0', which we stripped to 'battery@0'). However,
it also breaks of_device_id table matching.
The fix that we _really_ wanted was to keep dp->name based upon
the name property ('battery'), but based dp->full_name upon
package-to-path ('battery@0'). This patch does just that.
This also changes OLPC behavior to use the full result from
package-to-path for full_name, rather than stripping the directory
out. In practice, the strings end up being exactly the same; this
change saves time, code, and memory.
v2: combine two patches and revert of_pdt_node_name to original version
v3: use dp->phandle instead of passing around node
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 37 ++++++++++++-------------------------
1 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..c4cadeb 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
static char * __init of_pdt_try_pkg2path(phandle node)
{
- char *res, *buf = NULL;
+ char *buf = NULL;
int len;
if (!of_pdt_prom_ops->pkg2path)
@@ -147,29 +147,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
pr_err("%s: package-to-path failed\n", __func__);
return NULL;
}
-
- res = strrchr(buf, '/');
- if (!res) {
- pr_err("%s: couldn't find / in %s\n", __func__, buf);
- return NULL;
- }
- return res+1;
-}
-
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM callback,
- * or some other random failure), fall back to just looking at the node's
- * 'name' property.
- */
-static char * __init of_pdt_build_name(phandle node)
-{
- char *buf;
-
- buf = of_pdt_try_pkg2path(node);
- if (!buf)
- buf = of_pdt_get_one_property(node, "name");
-
return buf;
}
@@ -187,7 +164,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
kref_init(&dp->kref);
- dp->name = of_pdt_build_name(node);
+ dp->name = of_pdt_get_one_property(node, "name");
dp->type = of_pdt_get_one_property(node, "device_type");
dp->phandle = node;
@@ -203,6 +180,16 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
int len, ourlen, plen;
char *n;
+ /*
+ * When fetching the full name we want the name we see with
+ * package-to-path (ie, '/foo/bar/battery@0') rather than what
+ * we see with the name property (ie, 'battery').
+ */
+ n = of_pdt_try_pkg2path(dp->phandle);
+ if (n)
+ return n;
+
+ /* Older method for determining full name */
plen = strlen(dp->parent->full_name);
ourlen = strlen(of_pdt_node_name(dp));
len = ourlen + plen + 2;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v3)
@ 2011-02-24 0:34 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 0:34 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
Commit e2f2a93b changed dp->name from using the 'name' property to
using package-to-path. This fixed /proc/device-tree creation by
eliminating conflicts between names (the 'name' property provides
names like 'battery', whereas package-to-path provides names like
'/foo/bar/battery@0', which we stripped to 'battery@0'). However,
it also breaks of_device_id table matching.
The fix that we _really_ wanted was to keep dp->name based upon
the name property ('battery'), but based dp->full_name upon
package-to-path ('battery@0'). This patch does just that.
This also changes OLPC behavior to use the full result from
package-to-path for full_name, rather than stripping the directory
out. In practice, the strings end up being exactly the same; this
change saves time, code, and memory.
v2: combine two patches and revert of_pdt_node_name to original version
v3: use dp->phandle instead of passing around node
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 37 ++++++++++++-------------------------
1 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..c4cadeb 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
static char * __init of_pdt_try_pkg2path(phandle node)
{
- char *res, *buf = NULL;
+ char *buf = NULL;
int len;
if (!of_pdt_prom_ops->pkg2path)
@@ -147,29 +147,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
pr_err("%s: package-to-path failed\n", __func__);
return NULL;
}
-
- res = strrchr(buf, '/');
- if (!res) {
- pr_err("%s: couldn't find / in %s\n", __func__, buf);
- return NULL;
- }
- return res+1;
-}
-
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM callback,
- * or some other random failure), fall back to just looking at the node's
- * 'name' property.
- */
-static char * __init of_pdt_build_name(phandle node)
-{
- char *buf;
-
- buf = of_pdt_try_pkg2path(node);
- if (!buf)
- buf = of_pdt_get_one_property(node, "name");
-
return buf;
}
@@ -187,7 +164,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
kref_init(&dp->kref);
- dp->name = of_pdt_build_name(node);
+ dp->name = of_pdt_get_one_property(node, "name");
dp->type = of_pdt_get_one_property(node, "device_type");
dp->phandle = node;
@@ -203,6 +180,16 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
int len, ourlen, plen;
char *n;
+ /*
+ * When fetching the full name we want the name we see with
+ * package-to-path (ie, '/foo/bar/battery@0') rather than what
+ * we see with the name property (ie, 'battery').
+ */
+ n = of_pdt_try_pkg2path(dp->phandle);
+ if (n)
+ return n;
+
+ /* Older method for determining full name */
plen = strlen(dp->parent->full_name);
ourlen = strlen(of_pdt_node_name(dp));
len = ourlen + plen + 2;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-24 0:34 ` [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v3) Andres Salomon
@ 2011-02-24 2:47 ` Grant Likely
-1 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-24 2:47 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 04:34:42PM -0800, Andres Salomon wrote:
>
> Commit e2f2a93b changed dp->name from using the 'name' property to
> using package-to-path. This fixed /proc/device-tree creation by
> eliminating conflicts between names (the 'name' property provides
> names like 'battery', whereas package-to-path provides names like
> '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> it also breaks of_device_id table matching.
>
> The fix that we _really_ wanted was to keep dp->name based upon
> the name property ('battery'), but based dp->full_name upon
> package-to-path ('battery@0'). This patch does just that.
>
> This also changes OLPC behavior to use the full result from
> package-to-path for full_name, rather than stripping the directory
> out. In practice, the strings end up being exactly the same; this
> change saves time, code, and memory.
>
> v2: combine two patches and revert of_pdt_node_name to original version
> v3: use dp->phandle instead of passing around node
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
This addresses one of my comments on v2; but it doesn't address the
comment that the broken implementation of of_pdt_node_name for
non-sparc still remains, or am I missing something?
g.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v3)
@ 2011-02-24 2:47 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-24 2:47 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 04:34:42PM -0800, Andres Salomon wrote:
>
> Commit e2f2a93b changed dp->name from using the 'name' property to
> using package-to-path. This fixed /proc/device-tree creation by
> eliminating conflicts between names (the 'name' property provides
> names like 'battery', whereas package-to-path provides names like
> '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> it also breaks of_device_id table matching.
>
> The fix that we _really_ wanted was to keep dp->name based upon
> the name property ('battery'), but based dp->full_name upon
> package-to-path ('battery@0'). This patch does just that.
>
> This also changes OLPC behavior to use the full result from
> package-to-path for full_name, rather than stripping the directory
> out. In practice, the strings end up being exactly the same; this
> change saves time, code, and memory.
>
> v2: combine two patches and revert of_pdt_node_name to original version
> v3: use dp->phandle instead of passing around node
>
> Signed-off-by: Andres Salomon <dilinger@queued.net>
This addresses one of my comments on v2; but it doesn't address the
comment that the broken implementation of of_pdt_node_name for
non-sparc still remains, or am I missing something?
g.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-24 2:47 ` [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v3) Grant Likely
@ 2011-02-24 2:51 ` Andres Salomon
-1 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 2:51 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, 23 Feb 2011 19:47:08 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 23, 2011 at 04:34:42PM -0800, Andres Salomon wrote:
> >
> > Commit e2f2a93b changed dp->name from using the 'name' property to
> > using package-to-path. This fixed /proc/device-tree creation by
> > eliminating conflicts between names (the 'name' property provides
> > names like 'battery', whereas package-to-path provides names like
> > '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> > it also breaks of_device_id table matching.
> >
> > The fix that we _really_ wanted was to keep dp->name based upon
> > the name property ('battery'), but based dp->full_name upon
> > package-to-path ('battery@0'). This patch does just that.
> >
> > This also changes OLPC behavior to use the full result from
> > package-to-path for full_name, rather than stripping the directory
> > out. In practice, the strings end up being exactly the same; this
> > change saves time, code, and memory.
> >
> > v2: combine two patches and revert of_pdt_node_name to original
> > version v3: use dp->phandle instead of passing around node
> >
> > Signed-off-by: Andres Salomon <dilinger@queued.net>
>
> This addresses one of my comments on v2; but it doesn't address the
> comment that the broken implementation of of_pdt_node_name for
> non-sparc still remains, or am I missing something?
>
> g.
>
I responded to that -
http://www.spinics.net/lists/sparclinux/msg08058.html
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v3)
@ 2011-02-24 2:51 ` Andres Salomon
0 siblings, 0 replies; 51+ messages in thread
From: Andres Salomon @ 2011-02-24 2:51 UTC (permalink / raw)
To: Grant Likely
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, 23 Feb 2011 19:47:08 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 23, 2011 at 04:34:42PM -0800, Andres Salomon wrote:
> >
> > Commit e2f2a93b changed dp->name from using the 'name' property to
> > using package-to-path. This fixed /proc/device-tree creation by
> > eliminating conflicts between names (the 'name' property provides
> > names like 'battery', whereas package-to-path provides names like
> > '/foo/bar/battery@0', which we stripped to 'battery@0'). However,
> > it also breaks of_device_id table matching.
> >
> > The fix that we _really_ wanted was to keep dp->name based upon
> > the name property ('battery'), but based dp->full_name upon
> > package-to-path ('battery@0'). This patch does just that.
> >
> > This also changes OLPC behavior to use the full result from
> > package-to-path for full_name, rather than stripping the directory
> > out. In practice, the strings end up being exactly the same; this
> > change saves time, code, and memory.
> >
> > v2: combine two patches and revert of_pdt_node_name to original
> > version v3: use dp->phandle instead of passing around node
> >
> > Signed-off-by: Andres Salomon <dilinger@queued.net>
>
> This addresses one of my comments on v2; but it doesn't address the
> comment that the broken implementation of of_pdt_node_name for
> non-sparc still remains, or am I missing something?
>
> g.
>
I responded to that -
http://www.spinics.net/lists/sparclinux/msg08058.html
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'
2011-02-24 2:51 ` [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v3) Andres Salomon
@ 2011-02-24 3:25 ` Grant Likely
-1 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-24 3:25 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 06:51:52PM -0800, Andres Salomon wrote:
> On Wed, 23 Feb 2011 19:47:08 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
> > This addresses one of my comments on v2; but it doesn't address the
> > comment that the broken implementation of of_pdt_node_name for
> > non-sparc still remains, or am I missing something?
> >
> > g.
> >
>
>
> I responded to that -
> http://www.spinics.net/lists/sparclinux/msg08058.html
Weird; that reply didn't show up in my inbox (but my mailer did
receive it, so it is definitely a problem on my end). I'll reply
now...
g.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] of/pdt: allow DT device matching by fixing 'name' brokenness (v3)
@ 2011-02-24 3:25 ` Grant Likely
0 siblings, 0 replies; 51+ messages in thread
From: Grant Likely @ 2011-02-24 3:25 UTC (permalink / raw)
To: Andres Salomon
Cc: Daniel Drake, linux-kernel, devicetree-discuss, David S. Miller,
sparclinux
On Wed, Feb 23, 2011 at 06:51:52PM -0800, Andres Salomon wrote:
> On Wed, 23 Feb 2011 19:47:08 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
> > This addresses one of my comments on v2; but it doesn't address the
> > comment that the broken implementation of of_pdt_node_name for
> > non-sparc still remains, or am I missing something?
> >
> > g.
> >
>
>
> I responded to that -
> http://www.spinics.net/lists/sparclinux/msg08058.html
Weird; that reply didn't show up in my inbox (but my mailer did
receive it, so it is definitely a problem on my end). I'll reply
now...
g.
^ permalink raw reply [flat|nested] 51+ messages in thread