RE: [RFC PATCH 1/2] clk: add new APIs to operate on all available clocks
From: A.s. Dong
Date: Mon Mar 19 2018 - 23:21:30 EST
> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx]
> Sent: Tuesday, March 20, 2018 12:22 AM
> To: A.s. Dong <aisheng.dong@xxxxxxx>; linux-clk@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; dl-
> linux-imx <linux-imx@xxxxxxx>; aisheng.dong@xxxxxxxxxxxxxx; linux-
> fbdev@xxxxxxxxxxxxxxx; yamada.masahiro@xxxxxxxxxxxxx;
> sboyd@xxxxxxxxxxxxxx; hdegoede@xxxxxxxxxx;
> b.zolnierkie@xxxxxxxxxxx; A.s. Dong <aisheng.dong@xxxxxxx>
> Subject: Re: [RFC PATCH 1/2] clk: add new APIs to operate on all available
> clocks
>
> Quoting Dong Aisheng (2018-01-23 04:50:40)
> > This patch introduces of_clk_bulk_get_all and clk_bulk_x_all APIs to
> > users who just want to handle all available clocks from device tree
> > without need to know the detailed clock information likes clock
> > numbers and names. This is useful in writing some generic drivers to
> > handle clock part.
> >
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> > Cc: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> > Signed-off-by: Dong Aisheng <aisheng.dong@xxxxxxx>
> >
> > ---
> > A few question may need discuss:
> > 1) This patch is written based on of_clk_bulk_get.
> > [V4,1/1] clk: bulk: add of_clk_bulk_get()
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >
> chwork.kernel.org%2Fpatch%2F9971527%2F&data=02%7C01%7Caisheng.don
> g%40n
> >
> xp.com%7C449209dd3e19491afb9908d58db5966d%7C686ea1d3bc2b4c6fa92c
> d99c5c
> >
> 301635%7C0%7C0%7C636570733411754127&sdata=4m5emXOR%2Bui3Mqw
> MbAJQDQxJcE
> > %2BxLcB51hEi2q3V2O8%3D&reserved=0 Stepen once said we may not
> need it,
> > but i guess as we already have clk_bulk_get, there may be guys who
> > want of_clk_bulk_get as well if they need specify the clock count
> > information, becaues of_clk_bulk_get_all will not check the count.
> > And of_clk_bulk_get is also helpful when implementing
> > of_clk_bulk_get_all.
>
> Hmm ok. It's ok to implement it on top of of_clk_bulk_get I suppose, but
> maybe that API can be kept private until someone can prove they need it
> because they don't have a struct device pointer. Can you pick that patch from
> the list and resend in the series?
>
Of course. I will do that.
> >
> > 2) It only implements the DT type clk_get_all as i see Stephen said we
> > probably may not need to implement non-dt type as there're still no
> > users.
>
> Good.
>
> >
> > If we do want to implement non-dt type as well, we could revise the
> > patch to add it too.
> > ---
> > drivers/clk/clk-bulk.c | 64
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/clk.h | 61
> ++++++++++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 124 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk-bulk.c b/drivers/clk/clk-bulk.c index
> > 1c1a79d..bac2aae 100644
> > --- a/drivers/clk/clk-bulk.c
> > +++ b/drivers/clk/clk-bulk.c
> > @@ -17,9 +17,11 @@
> > */
> >
> > #include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > #include <linux/device.h>
> > #include <linux/export.h>
> > #include <linux/of.h>
> > +#include <linux/slab.h>
> >
> > #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) int
> > __must_check of_clk_bulk_get(struct device_node *np, int num_clks, @@
> > -50,6 +52,45 @@ int __must_check of_clk_bulk_get(struct device_node
> *np, int num_clks,
> > return ret;
> > }
> > EXPORT_SYMBOL(of_clk_bulk_get);
> > +
> > +struct clk_bulk __must_check *of_clk_bulk_get_all(struct device_node
> > +*np) {
> > + struct clk_bulk *clk_bulk;
> > + int num_clks;
> > + int ret;
> > +
> > + num_clks = of_clk_get_parent_count(np);
> > + if (!num_clks)
> > + return NULL;
> > +
> > + clk_bulk = kzalloc(sizeof(*clk_bulk) +
> > + num_clks * sizeof(struct clk_bulk_data),
> > + GFP_KERNEL);
> > + if (!clk_bulk)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + clk_bulk->num_clks = num_clks;
> > + ret = of_clk_bulk_get(np, clk_bulk->num_clks, clk_bulk->clks);
> > + if (ret) {
> > + kfree(clk_bulk);
> > + return ERR_PTR(ret);
> > + }
>
> Has this been tested? clk_bulk->clks probably points to junk?
>
You're right. Will fix in a formal patch.
> > +
> > + return clk_bulk;
> > +}
> > +EXPORT_SYMBOL(of_clk_bulk_get_all);
> > +
> > +void of_clk_bulk_put_all(struct clk_bulk *clk_bulk) {
> > + if (IS_ERR_OR_NULL(clk_bulk))
> > + return;
> > +
> > + clk_bulk_put(clk_bulk->num_clks, clk_bulk->clks);
> > +
> > + kfree(clk_bulk->clks);
> > + kfree(clk_bulk);
> > +}
> > +EXPORT_SYMBOL(of_clk_bulk_put_all);
> > #endif
>
> My goal was to make device drivers pass in their struct device to
> clk_bulk_get_all() and then have that call into a private DT helper function
> like of_clk_bulk_get_all(). With your patch 2 that could still be done so let's
> go that direction?
>
Sounds good to me.
> >
> > void clk_bulk_put(int num_clks, struct clk_bulk_data *clks) @@ -107,6
> > +148,12 @@ void clk_bulk_unprepare(int num_clks, const struct
> > clk_bulk_data *clks) } EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
> >
> > +void inline clk_bulk_unprepare_all(const struct clk_bulk *clk_bulk) {
> > + clk_bulk_unprepare(clk_bulk->num_clks, clk_bulk->clks); }
> > +EXPORT_SYMBOL_GPL(clk_bulk_unprepare_all);
>
> Do we really need these _all APIs though?
>
> > +
> > /**
> > * clk_bulk_prepare - prepare a set of clocks
> > * @num_clks: the number of clk_bulk_data @@ -139,6 +186,11 @@ int
> > __must_check clk_bulk_prepare(int num_clks, }
> > EXPORT_SYMBOL_GPL(clk_bulk_prepare);
> >
> > +int inline __must_check clk_bulk_prepare_all(const struct clk_bulk
> > +*clk_bulk)
>
> inline won't help much here. Maybe these wrappers should go into the
> header file?
>
Will try that.
> > +{
> > + return clk_bulk_prepare(clk_bulk->num_clks, clk_bulk->clks); }
> > +
> > #endif /* CONFIG_HAVE_CLK_PREPARE */
> >
> > /**
> > @@ -158,6 +210,12 @@ void clk_bulk_disable(int num_clks, const struct
> > clk_bulk_data *clks) } EXPORT_SYMBOL_GPL(clk_bulk_disable);
> >
> > +void inline clk_bulk_disable_all(const struct clk_bulk *clk_bulk)
>
> ditto
>
> > +{
> > + return clk_bulk_disable(clk_bulk->num_clks, clk_bulk->clks); }
> > +EXPORT_SYMBOL_GPL(clk_bulk_disable_all);
> > +
> > /**
> > * clk_bulk_enable - ungate a set of clocks
> > * @num_clks: the number of clk_bulk_data @@ -188,3 +246,9 @@ int
> > __must_check clk_bulk_enable(int num_clks, const struct clk_bulk_data
> *clks)
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(clk_bulk_enable);
> > +
> > +int inline __must_check clk_bulk_enable_all(const struct clk_bulk
> > +*clk_bulk)
>
> again
>
> > +{
> > + return clk_bulk_enable(clk_bulk->num_clks, clk_bulk->clks); }
> > +EXPORT_SYMBOL_GPL(clk_bulk_enable_all);
> > diff --git a/include/linux/clk.h b/include/linux/clk.h index
> > 09ae760..ccafda1 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -92,6 +92,21 @@ struct clk_bulk_data {
> > struct clk *clk;
> > };
> >
> > +/**
> > + * struct clk_bulk - bulk clk structure
> > + *
> > + * @num_clks: the number of avaliable clocks in this bulk
> > + * @clks: struct clk_bulk_data * to store the associated clock
> > + *
> > + * The CLK APIs provide a series of clk_bulk_x_all() API calls as
> > + * a convenience to consumers which require multiple clks. This
> > + * structure is used to manage data for these calls.
> > + */
> > +struct clk_bulk {
> > + int num_clks;
> > + struct clk_bulk_data *clks;
> > +};
>
> This could go away, and we could return the number of clks from
> clk_bulk_get_all() as a positive number, 0 if there are none, and a negative
> number if something failed. Then we don't need the _all variants or a
> wrapper struct. It would be nice to avoid the extra variants just because we
> need to tell it the number of clks to handle.
This looks like a good idea.
I will try it.
Thanks for your good suggestions.
Regards
Dong Aisheng