Kodiak Furr liked your message with Boxer for Android.
On Oct 9, 2014 6:22 PM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
��.n��������+%������w��{.n�����{�������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
