| Message ID | 20110218190659.7f955e4c@queued.net |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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. -- 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
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. -- 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
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);