Message-ID: <54371d88.6f3fb60a.40e6.ffff9ffd@mx.google.com>
Date:	Thu, 09 Oct 2014 18:42:55 -0500
From:	Kodiak Furr <taskboxtester@...il.com>
To:	Stephen Boyd <sboyd@...eaurora.org>
Cc:	Mike Turquette <mturquette@...aro.org>,
	Paul Walmsley <paul@...an.com>,
	Tony Lindgren <tony@...mide.com>,
	linux-arm-kernel@...ts.infradead.org,
	Tomeu Vizoso <tomeu.vizoso@...labora.com>,
	linux-omap@...r.kernel.org,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	Russell King <linux@....linux.org.uk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 7/8] clk: Make clk API return per-user struct clk
 instances

Kodiak Furr liked your message with Boxer for Android.

On Oct 9, 2014 6:22 PM, Stephen Boyd <sboyd@...eaurora.org> wrote:
>
> On 10/09, Tomeu Vizoso wrote: 
> >  arch/arm/mach-omap2/cclock3xxx_data.c   | 108 ++++-- 
> >  arch/arm/mach-omap2/clock.h             |  11 +- 
> >  arch/arm/mach-omap2/clock_common_data.c |   5 +- 
> >  drivers/clk/clk.c                       | 630 ++++++++++++++++++++------------ 
> >  drivers/clk/clk.h                       |   5 + 
> >  drivers/clk/clkdev.c                    |  24 +- 
> >  include/linux/clk-private.h             |  35 +- 
> >  include/linux/clk-provider.h            |  22 +- 
> >  8 files changed, 549 insertions(+), 291 deletions(-) 
>
> The difstat looks good. 
>
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c 
> > index fb820bf..4db918a 100644 
> > --- a/drivers/clk/clk.c 
> > +++ b/drivers/clk/clk.c 
> > @@ -695,6 +731,13 @@ struct clk *__clk_lookup(const char *name) 
> >  return NULL; 
> >  } 
> >  
> > +struct clk *__clk_lookup(const char *name) 
> > +{ 
> > + struct clk_core *clk = clk_core_lookup(name); 
> > + 
> > + return !clk ? NULL : clk->hw->clk; 
>
> This just looks weird with clk->hw->clk. I know we're trying to 
> keep the diff small by not renaming clk to core when it's used 
> extensively throughout the code, but for small little additions 
> like this I would prefer we use core for clk_core pointers and 
> clk for clk pointers. Then a patch at the end can rename 
> everything to be consistent. This thing also threw me off because 
> I searched for kfree(core) but couldn't find it so I thought we 
> leaked the clk_core structure. 
>
> > +} 
> > + 
> >  /* 
> >   * Helper for finding best parent to provide a given frequency. This can be used 
> >   * directly as a determine_rate callback (e.g. for a mux), or from a more 
> > @@ -2175,24 +2298,24 @@ void clk_unregister(struct clk *clk) 
> >  * a reference to this clock. 
> >  */ 
> >  flags = clk_enable_lock(); 
> > - clk->ops = &clk_nodrv_ops; 
> > + clk->core->ops = &clk_nodrv_ops; 
> >  clk_enable_unlock(flags); 
> >  
> > - if (!hlist_empty(&clk->children)) { 
> > - struct clk *child; 
> > + if (!hlist_empty(&clk->core->children)) { 
> > + struct clk_core *child; 
> >  struct hlist_node *t; 
> >  
> >  /* Reparent all children to the orphan list. */ 
> > - hlist_for_each_entry_safe(child, t, &clk->children, child_node) 
> > - clk_set_parent(child, NULL); 
> > + hlist_for_each_entry_safe(child, t, &clk->core->children, child_node) 
> > + clk_core_set_parent(child, NULL); 
> >  } 
> >  
> > - hlist_del_init(&clk->child_node); 
> > + hlist_del_init(&clk->core->child_node); 
> >  
> > - if (clk->prepare_count) 
> > + if (clk->core->prepare_count) 
> >  pr_warn("%s: unregistering prepared clock: %s\n", 
> > - __func__, clk->name); 
> > - kref_put(&clk->ref, __clk_release); 
> > + __func__, clk->core->name); 
> > + kref_put(&clk->core->ref, __clk_release); 
> >  
> >  clk_prepare_unlock(); 
>
> It might be worth it to make a "core" local variable in this 
> function. 
>
> >  } 
> > @@ -2255,32 +2378,38 @@ void devm_clk_unregister(struct device *dev, struct clk *clk) 
> >  } 
> >  EXPORT_SYMBOL_GPL(devm_clk_unregister); 
> >  
> > +static void clk_core_put(struct clk_core *clk) 
> > +{ 
> > + struct module *owner; 
> > + 
> > + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) 
> > + return; 
> > + 
> > + clk_prepare_lock(); 
> > + owner = clk->owner; 
>
> Same here too, we don't need to protect the access to owner so it 
> can move outside the lock. 
>
> > + kref_put(&clk->ref, __clk_release); 
> > + module_put(owner); 
> > + clk_prepare_unlock(); 
> > +} 
> > + 
> >  /* 
> >   * clkdev helpers 
> >   */ 
> >  int __clk_get(struct clk *clk) 
> >  { 
> >  if (clk) { 
> > - if (!try_module_get(clk->owner)) 
> > + if (!try_module_get(clk->core->owner)) 
> >  return 0; 
> >  
> > - kref_get(&clk->ref); 
> > + kref_get(&clk->core->ref); 
> >  } 
> >  return 1; 
>
> Grow a core pointer? 
>
> >  } 
> > @@ -2391,6 +2520,31 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb) 
> >  } 
> >  EXPORT_SYMBOL_GPL(clk_notifier_unregister); 
> >  
> > +struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id, 
>
> Curious, why the underscore? 
>
> > +      const char *con_id) 
> > +{ 
> > + struct clk *clk; 
> > + 
> > + /* This is to allow this function to be chained to others */ 
> > + if (!hw || IS_ERR(hw)) 
> > + return (struct clk *) hw; 
> > + 
> > + clk = kzalloc(sizeof(*clk), GFP_KERNEL); 
> > + if (!clk) 
> > + return ERR_PTR(-ENOMEM); 
> > + 
> > + clk->core = hw->core; 
> > + clk->dev_id = dev_id; 
> > + clk->con_id = con_id; 
> > + 
> > + return clk; 
> > +} 
> > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c 
> > index da4bda8..4411db6 100644 
> > --- a/drivers/clk/clkdev.c 
> > +++ b/drivers/clk/clkdev.c 
> > @@ -69,6 +70,10 @@ struct clk *of_clk_get(struct device_node *np, int index) 
> >  
> >  clk = of_clk_get_by_clkspec(&clkspec); 
> >  of_node_put(clkspec.np); 
> > + 
> > + if (!IS_ERR(clk)) 
> > + clk = __clk_create_clk(__clk_get_hw(clk), np->full_name, NULL); 
>
> We lost the debugging information here about the device 
> requesting this clock and the name they called it. We get the 
> device node name but that might not match the device name. We 
> probably need to make private functions in here that allow such 
> information to be passed without changing the API for 
> of_clk_get(), of_clk_get_by_name(), etc. 
>
> > + 
> >  return clk; 
> >  } 
> >  EXPORT_SYMBOL(of_clk_get); 
> > @@ -168,14 +173,29 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) 
> >  struct clk *clk_get_sys(const char *dev_id, const char *con_id) 
> >  { 
> >  struct clk_lookup *cl; 
> > + struct clk *clk = NULL; 
> >  
> >  mutex_lock(&clocks_mutex); 
> > + 
> >  cl = clk_find(dev_id, con_id); 
> > - if (cl && !__clk_get(cl->clk)) 
> > + if (!cl) 
> > + goto out; 
> > + 
> > + if (!__clk_get(cl->clk)) { 
> >  cl = NULL; 
> > + goto out; 
> > + } 
> > + 
> > +#if defined(CONFIG_COMMON_CLK) 
> > + clk = __clk_create_clk(__clk_get_hw(cl->clk), dev_id, con_id); 
>
> I was hoping we could put the clk_hw pointer next to the clk 
> pointer in the lookup structure. Then providers don't have to 
> deal with clk pointers at all and can just assign their clk_hw 
> pointer in the lookup. This would make most of the omap usage of 
> struct clk unnecessary. It doesn't seem necessary though so I 
> guess we can do that in another series? 
>
> > +#else 
> > + clk = cl->clk; 
> > +#endif 
> > + 
> > +out: 
> >  mutex_unlock(&clocks_mutex); 
> >  
> > - return cl ? cl->clk : ERR_PTR(-ENOENT); 
> > + return cl ? clk : ERR_PTR(-ENOENT); 
> >  } 
> >  EXPORT_SYMBOL(clk_get_sys); 
> >  
> > @@ -554,6 +559,19 @@ long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate, 
> >        unsigned long *best_parent_rate, 
> >        struct clk_hw **best_parent_p); 
> >  
> > +/** 
> > + * __clk_core_to_clk - return per-user clk 
> > + * @clk: struct clk_core for which we want a per-user clk 
> > + * 
> > + * Returns a per-user clock that is owned by its provider. The caller shall not 
> > + * call clk_get() on it. 
> > + * 
> > + * This function should be only needed by implementors of 
> > + * clk_ops.determine_rate() and should be dropped once all have moved to a 
> > + * variant that returns **clk_core instead. 
> > + */ 
> > +struct clk *__clk_core_to_clk(struct clk_core *clk); 
> > + 
>
> We can drop this now right? 
>
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
> a Linux Foundation Collaborative Project 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in 
> the body of a message to majordomo@...r.kernel.org 
> More majordomo info at  http://vger.kernel.org/majordomo-info.html 
> Please read the FAQ at  http://www.tux.org/lkml/ 

Powered by blists - more mailing lists