| Message ID | 1300108722-3254-6-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 14, 2011 at 09:18:42PM +0800, Shawn Guo wrote: > With the platform clock support, the 'struct clk' should have been > associated with device_node->data. So the use of function > __of_clk_get_from_provider can be eliminated. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > drivers/of/clock.c | 23 ++--------------------- > 1 files changed, 2 insertions(+), 21 deletions(-) > > diff --git a/drivers/of/clock.c b/drivers/of/clock.c > index 7b5ea67..f124d0a 100644 > --- a/drivers/of/clock.c > +++ b/drivers/of/clock.c > @@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np, > mutex_unlock(&of_clk_lock); > } > > -static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output) > -{ > - struct of_clk_provider *provider; > - struct clk *clk = NULL; > - > - /* Check if we have such a provider in our array */ > - mutex_lock(&of_clk_lock); > - list_for_each_entry(provider, &of_clk_providers, link) { > - if (provider->node == np) > - clk = provider->get(np, clk_output, provider->data); > - if (clk) > - break; > - } > - mutex_unlock(&of_clk_lock); > - > - return clk; > -} > - > struct clk *of_clk_get(struct device *dev, const char *id) > { > struct device_node *provnode; > @@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id) > __func__, prop_name, dev->of_node->full_name); > return NULL; > } > - clk = __of_clk_get_from_provider(provnode, prop); > - if (clk) > - dev_dbg(dev, "Using clock from %s\n", provnode->full_name); > + > + clk = provnode->data; Where is the device_node->data pointer getting set? In general the ->data pointer of struct device_node should be avoided. There are no strong rules about its usage which means there is a very real risk that another driver or subsystem will try to use it for a different purpose. Iterating over the whole device tree is safer, and it really isn't very expensive. If you really want to store the struct_clk pointer in the device node, then it would be better to add a struct clk * field to struct device_node. g.
On Tue, Mar 15, 2011 at 01:54:05AM -0600, Grant Likely wrote: > On Mon, Mar 14, 2011 at 09:18:42PM +0800, Shawn Guo wrote: > > With the platform clock support, the 'struct clk' should have been > > associated with device_node->data. So the use of function > > __of_clk_get_from_provider can be eliminated. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > --- > > drivers/of/clock.c | 23 ++--------------------- > > 1 files changed, 2 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/of/clock.c b/drivers/of/clock.c > > index 7b5ea67..f124d0a 100644 > > --- a/drivers/of/clock.c > > +++ b/drivers/of/clock.c > > @@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np, > > mutex_unlock(&of_clk_lock); > > } > > > > -static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output) > > -{ > > - struct of_clk_provider *provider; > > - struct clk *clk = NULL; > > - > > - /* Check if we have such a provider in our array */ > > - mutex_lock(&of_clk_lock); > > - list_for_each_entry(provider, &of_clk_providers, link) { > > - if (provider->node == np) > > - clk = provider->get(np, clk_output, provider->data); > > - if (clk) > > - break; > > - } > > - mutex_unlock(&of_clk_lock); > > - > > - return clk; > > -} > > - > > struct clk *of_clk_get(struct device *dev, const char *id) > > { > > struct device_node *provnode; > > @@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id) > > __func__, prop_name, dev->of_node->full_name); > > return NULL; > > } > > - clk = __of_clk_get_from_provider(provnode, prop); > > - if (clk) > > - dev_dbg(dev, "Using clock from %s\n", provnode->full_name); > > + > > + clk = provnode->data; > > Where is the device_node->data pointer getting set? > +#define ADD_CLK_LOOKUP() \ + do { \ + node->data = clk; \ ^^^^^^^^^^^^^^^^^ + \ + cl->dev_id = dev_id; \ + cl->clk = clk; \ + clkdev_add(cl); \ + \ + return 0; \ + \ + out_kfree: \ + kfree(cl); \ + return ret; \ + } while (0) > In general the ->data pointer of struct device_node should be avoided. > There are no strong rules about its usage which means there is a very > real risk that another driver or subsystem will try to use it for a > different purpose. > > Iterating over the whole device tree is safer, and it really isn't > very expensive. If you really want to store the struct_clk pointer in > the device node, then it would be better to add a struct clk * field > to struct device_node. >
On Tue, Mar 15, 2011 at 03:59:16PM +0800, Shawn Guo wrote: > On Tue, Mar 15, 2011 at 01:54:05AM -0600, Grant Likely wrote: > > On Mon, Mar 14, 2011 at 09:18:42PM +0800, Shawn Guo wrote: > > > With the platform clock support, the 'struct clk' should have been > > > associated with device_node->data. So the use of function > > > __of_clk_get_from_provider can be eliminated. > > > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > > --- > > > drivers/of/clock.c | 23 ++--------------------- > > > 1 files changed, 2 insertions(+), 21 deletions(-) > > > > > > diff --git a/drivers/of/clock.c b/drivers/of/clock.c > > > index 7b5ea67..f124d0a 100644 > > > --- a/drivers/of/clock.c > > > +++ b/drivers/of/clock.c > > > @@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np, > > > mutex_unlock(&of_clk_lock); > > > } > > > > > > -static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output) > > > -{ > > > - struct of_clk_provider *provider; > > > - struct clk *clk = NULL; > > > - > > > - /* Check if we have such a provider in our array */ > > > - mutex_lock(&of_clk_lock); > > > - list_for_each_entry(provider, &of_clk_providers, link) { > > > - if (provider->node == np) > > > - clk = provider->get(np, clk_output, provider->data); > > > - if (clk) > > > - break; > > > - } > > > - mutex_unlock(&of_clk_lock); > > > - > > > - return clk; > > > -} > > > - > > > struct clk *of_clk_get(struct device *dev, const char *id) > > > { > > > struct device_node *provnode; > > > @@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id) > > > __func__, prop_name, dev->of_node->full_name); > > > return NULL; > > > } > > > - clk = __of_clk_get_from_provider(provnode, prop); > > > - if (clk) > > > - dev_dbg(dev, "Using clock from %s\n", provnode->full_name); > > > + > > > + clk = provnode->data; > > > > Where is the device_node->data pointer getting set? > > > +#define ADD_CLK_LOOKUP() \ > + do { \ > + node->data = clk; \ > ^^^^^^^^^^^^^^^^^ > + \ > + cl->dev_id = dev_id; \ > + cl->clk = clk; \ > + clkdev_add(cl); \ > + \ > + return 0; \ > + \ > + out_kfree: \ > + kfree(cl); \ > + return ret; \ > + } while (0) Yeah... don't do that! :-) g.
On Tue, Mar 15, 2011 at 01:54:05AM -0600, Grant Likely wrote: > On Mon, Mar 14, 2011 at 09:18:42PM +0800, Shawn Guo wrote: > > With the platform clock support, the 'struct clk' should have been > > associated with device_node->data. So the use of function > > __of_clk_get_from_provider can be eliminated. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > --- > > drivers/of/clock.c | 23 ++--------------------- > > 1 files changed, 2 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/of/clock.c b/drivers/of/clock.c > > index 7b5ea67..f124d0a 100644 > > --- a/drivers/of/clock.c > > +++ b/drivers/of/clock.c > > @@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np, > > mutex_unlock(&of_clk_lock); > > } > > > > -static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output) > > -{ > > - struct of_clk_provider *provider; > > - struct clk *clk = NULL; > > - > > - /* Check if we have such a provider in our array */ > > - mutex_lock(&of_clk_lock); > > - list_for_each_entry(provider, &of_clk_providers, link) { > > - if (provider->node == np) > > - clk = provider->get(np, clk_output, provider->data); > > - if (clk) > > - break; > > - } > > - mutex_unlock(&of_clk_lock); > > - > > - return clk; > > -} > > - > > struct clk *of_clk_get(struct device *dev, const char *id) > > { > > struct device_node *provnode; > > @@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id) > > __func__, prop_name, dev->of_node->full_name); > > return NULL; > > } > > - clk = __of_clk_get_from_provider(provnode, prop); > > - if (clk) > > - dev_dbg(dev, "Using clock from %s\n", provnode->full_name); > > + > > + clk = provnode->data; > > Where is the device_node->data pointer getting set? > > In general the ->data pointer of struct device_node should be avoided. > There are no strong rules about its usage which means there is a very > real risk that another driver or subsystem will try to use it for a > different purpose. > > Iterating over the whole device tree is safer, and it really isn't > very expensive. If you really want to store the struct_clk pointer in I do not understand how we can get the pointer to the 'struct clk' from device tree. The clock code reads nodes from device tree and creates 'struct clk' dynamically. If we do not save the pointer somewhere, the pointer will get lost outside the clock code. How can we possibly get it back from device tree later? > the device node, then it would be better to add a struct clk * field > to struct device_node. >
diff --git a/drivers/of/clock.c b/drivers/of/clock.c index 7b5ea67..f124d0a 100644 --- a/drivers/of/clock.c +++ b/drivers/of/clock.c @@ -71,24 +71,6 @@ void of_clk_del_provider(struct device_node *np, mutex_unlock(&of_clk_lock); } -static struct clk *__of_clk_get_from_provider(struct device_node *np, const char *clk_output) -{ - struct of_clk_provider *provider; - struct clk *clk = NULL; - - /* Check if we have such a provider in our array */ - mutex_lock(&of_clk_lock); - list_for_each_entry(provider, &of_clk_providers, link) { - if (provider->node == np) - clk = provider->get(np, clk_output, provider->data); - if (clk) - break; - } - mutex_unlock(&of_clk_lock); - - return clk; -} - struct clk *of_clk_get(struct device *dev, const char *id) { struct device_node *provnode; @@ -123,9 +105,8 @@ struct clk *of_clk_get(struct device *dev, const char *id) __func__, prop_name, dev->of_node->full_name); return NULL; } - clk = __of_clk_get_from_provider(provnode, prop); - if (clk) - dev_dbg(dev, "Using clock from %s\n", provnode->full_name); + + clk = provnode->data; of_node_put(provnode);
With the platform clock support, the 'struct clk' should have been associated with device_node->data. So the use of function __of_clk_get_from_provider can be eliminated. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/of/clock.c | 23 ++--------------------- 1 files changed, 2 insertions(+), 21 deletions(-)