> -----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 ��.n��������+%������w��{.n�����{����n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
