| Message ID | 1519055046-2399-2-git-send-email-m.purski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Maciej, On 19/02/18 15:43, Maciej Purski wrote: > When a driver is going to use clk_bulk_get() function, it has to > initialize an array of clk_bulk_data, by filling its id fields. > > Add a new function to the core, which dynamically allocates > clk_bulk_data array and fills its id fields. Add clk_bulk_free() > function, which frees the array allocated by clk_bulk_alloc() function. > Add a managed version of clk_bulk_alloc(). Seeing how every subsequent patch ends up with the roughly this same stanza: x = devm_clk_bulk_alloc(dev, num, names); if (IS_ERR(x) return PTR_ERR(x); ret = devm_clk_bulk_get(dev, x, num); I wonder if it might be better to simply implement: int devm_clk_bulk_alloc_get(dev, &x, num, names) that does the whole lot in one go, and let drivers that want to do more fiddly things continue to open-code the allocation. But perhaps that's an abstraction too far... I'm not all that familiar with the lie of the land here. > Signed-off-by: Maciej Purski <m.purski@samsung.com> > --- > drivers/clk/clk-bulk.c | 16 ++++++++++++ > drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++--- > include/linux/clk.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 113 insertions(+), 4 deletions(-) > [...] > @@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id); > > #else /* !CONFIG_HAVE_CLK */ > > +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks, > + const char **clk_ids) > +{ > + return NULL; Either way, is it intentional not returning an ERR_PTR() value in this case? Since NULL will pass an IS_ERR() check, it seems a bit fragile for an allocation call to apparently succeed when the whole API is configured out (and I believe introducing new uses of IS_ERR_OR_NULL() is in general strongly discouraged.) Robin. > +} > + > +static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev, > + int num_clks, > + const char **clk_ids) > +{ > + return NULL; > +} > + > +static inline void clk_bulk_free(struct clk_bulk_data *clks) > +{ > +} > + > static inline struct clk *clk_get(struct device *dev, const char *id) > { > return NULL; >
HI Maciej, Just sharing a couple of fly-by ideas - please don't read too much into them. On 19 February 2018 at 15:43, Maciej Purski <m.purski@samsung.com> wrote: > When a driver is going to use clk_bulk_get() function, it has to > initialize an array of clk_bulk_data, by filling its id fields. > > Add a new function to the core, which dynamically allocates > clk_bulk_data array and fills its id fields. Add clk_bulk_free() > function, which frees the array allocated by clk_bulk_alloc() function. > Add a managed version of clk_bulk_alloc(). > Most places use a small fixed number of struct clk pointers. Using devres + kalloc to allocate 1-4 pointers feels a bit strange. Quick grep shows over 150 instances that could be updated to use the new API. Adding a cocci script to simplify the transition would be a good idea. > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -15,6 +15,7 @@ > #include <linux/err.h> > #include <linux/kernel.h> > #include <linux/notifier.h> > +#include <linux/slab.h> > The extra header declaration should not be needed. One should be able to forward declare any undefined structs. HTH Emil
Hi Robin, On 2018-02-19 17:29, Robin Murphy wrote: > Hi Maciej, > > On 19/02/18 15:43, Maciej Purski wrote: >> When a driver is going to use clk_bulk_get() function, it has to >> initialize an array of clk_bulk_data, by filling its id fields. >> >> Add a new function to the core, which dynamically allocates >> clk_bulk_data array and fills its id fields. Add clk_bulk_free() >> function, which frees the array allocated by clk_bulk_alloc() function. >> Add a managed version of clk_bulk_alloc(). > > Seeing how every subsequent patch ends up with the roughly this same > stanza: > > x = devm_clk_bulk_alloc(dev, num, names); > if (IS_ERR(x) > return PTR_ERR(x); > ret = devm_clk_bulk_get(dev, x, num); > > I wonder if it might be better to simply implement: > > int devm_clk_bulk_alloc_get(dev, &x, num, names) > > that does the whole lot in one go, and let drivers that want to do > more fiddly things continue to open-code the allocation. > > But perhaps that's an abstraction too far... I'm not all that familiar > with the lie of the land here. Hmmm. This patchset clearly shows, that it would be much simpler if we get rid of clk_bulk_data structure at all and let clk_bulk_* functions to operate on struct clk *array[]. Typically the list of clock names is already in some kind of array (taken from driver data or statically embedded into driver), so there is little benefit from duplicating it in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe there are other benefits from this approach. If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data structure and switching to clock ptr array: int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[], const char *clk_names[]); int clk_bulk_prepare(int num_clks, struct clk *clks[]); int clk_bulk_enable(int num_clks, struct clk *clks[]); ... > >> Signed-off-by: Maciej Purski <m.purski@samsung.com> >> --- >> drivers/clk/clk-bulk.c | 16 ++++++++++++ >> drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++--- >> include/linux/clk.h | 64 >> ++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 113 insertions(+), 4 deletions(-) >> > > [...] >> @@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, >> const char *con_id); >> #else /* !CONFIG_HAVE_CLK */ >> +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks, >> + const char **clk_ids) >> +{ >> + return NULL; > > Either way, is it intentional not returning an ERR_PTR() value in this > case? Since NULL will pass an IS_ERR() check, it seems a bit fragile > for an allocation call to apparently succeed when the whole API is > configured out (and I believe introducing new uses of IS_ERR_OR_NULL() > is in general strongly discouraged.) > > Robin. > >> +} >> + >> +static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct >> device *dev, >> + int num_clks, >> + const char **clk_ids) >> +{ >> + return NULL; >> +} >> + >> +static inline void clk_bulk_free(struct clk_bulk_data *clks) >> +{ >> +} >> + >> static inline struct clk *clk_get(struct device *dev, const char *id) >> { >> return NULL; > Best regards
Hi Marek, On 20/02/18 09:36, Marek Szyprowski wrote: > Hi Robin, > > On 2018-02-19 17:29, Robin Murphy wrote: >> Hi Maciej, >> >> On 19/02/18 15:43, Maciej Purski wrote: >>> When a driver is going to use clk_bulk_get() function, it has to >>> initialize an array of clk_bulk_data, by filling its id fields. >>> >>> Add a new function to the core, which dynamically allocates >>> clk_bulk_data array and fills its id fields. Add clk_bulk_free() >>> function, which frees the array allocated by clk_bulk_alloc() function. >>> Add a managed version of clk_bulk_alloc(). >> >> Seeing how every subsequent patch ends up with the roughly this same >> stanza: >> >> x = devm_clk_bulk_alloc(dev, num, names); >> if (IS_ERR(x) >> return PTR_ERR(x); >> ret = devm_clk_bulk_get(dev, x, num); >> >> I wonder if it might be better to simply implement: >> >> int devm_clk_bulk_alloc_get(dev, &x, num, names) >> >> that does the whole lot in one go, and let drivers that want to do >> more fiddly things continue to open-code the allocation. >> >> But perhaps that's an abstraction too far... I'm not all that familiar >> with the lie of the land here. > > Hmmm. This patchset clearly shows, that it would be much simpler if we > get rid of clk_bulk_data structure at all and let clk_bulk_* functions > to operate on struct clk *array[]. Typically the list of clock names > is already in some kind of array (taken from driver data or statically > embedded into driver), so there is little benefit from duplicating it > in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe > there are other benefits from this approach. > > If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data > structure and switching to clock ptr array: > > int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[], > const char *clk_names[]); > int clk_bulk_prepare(int num_clks, struct clk *clks[]); > int clk_bulk_enable(int num_clks, struct clk *clks[]); > ... Yes, that's certainly a possibility; if on the other hand there are desirable reasons for the encapsulation (personally, I do think it's quite neat), then maybe num_clocks should get pushed down into clk_bulk_data as well - then with dedicated alloc/free functions as proposed here it could become a simple opaque cookie as far as callers are concerned. I also haven't looked into the origins of the bulk API design, though; I've just been familiarising myself from the perspective of reviewing general clk API usage in drivers. Robin. >>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>> --- >>> drivers/clk/clk-bulk.c | 16 ++++++++++++ >>> drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++--- >>> include/linux/clk.h | 64 >>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 113 insertions(+), 4 deletions(-) >>> >> >> [...] >>> @@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, >>> const char *con_id); >>> #else /* !CONFIG_HAVE_CLK */ >>> +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks, >>> + const char **clk_ids) >>> +{ >>> + return NULL; >> >> Either way, is it intentional not returning an ERR_PTR() value in this >> case? Since NULL will pass an IS_ERR() check, it seems a bit fragile >> for an allocation call to apparently succeed when the whole API is >> configured out (and I believe introducing new uses of IS_ERR_OR_NULL() >> is in general strongly discouraged.) >> >> Robin. >> >>> +} >>> + >>> +static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct >>> device *dev, >>> + int num_clks, >>> + const char **clk_ids) >>> +{ >>> + return NULL; >>> +} >>> + >>> +static inline void clk_bulk_free(struct clk_bulk_data *clks) >>> +{ >>> +} >>> + >>> static inline struct clk *clk_get(struct device *dev, const char *id) >>> { >>> return NULL; >> > Best regards
Quoting Marek Szyprowski (2018-02-20 01:36:03) > Hi Robin, > > On 2018-02-19 17:29, Robin Murphy wrote: > > > > Seeing how every subsequent patch ends up with the roughly this same > > stanza: > > > > x = devm_clk_bulk_alloc(dev, num, names); > > if (IS_ERR(x) > > return PTR_ERR(x); > > ret = devm_clk_bulk_get(dev, x, num); > > > > I wonder if it might be better to simply implement: > > > > int devm_clk_bulk_alloc_get(dev, &x, num, names) > > > > that does the whole lot in one go, and let drivers that want to do > > more fiddly things continue to open-code the allocation. > > > > But perhaps that's an abstraction too far... I'm not all that familiar > > with the lie of the land here. > > Hmmm. This patchset clearly shows, that it would be much simpler if we > get rid of clk_bulk_data structure at all and let clk_bulk_* functions > to operate on struct clk *array[]. Typically the list of clock names > is already in some kind of array (taken from driver data or statically > embedded into driver), so there is little benefit from duplicating it > in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe > there are other benefits from this approach. > > If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data > structure and switching to clock ptr array: > > int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[], > const char *clk_names[]); > int clk_bulk_prepare(int num_clks, struct clk *clks[]); > int clk_bulk_enable(int num_clks, struct clk *clks[]); > ... > If you have an array of pointers to names of clks then we can put the struct clk pointers adjacent to them at the same time. I suppose the problem there is that some drivers want to mark that array of pointers to names as const. And then we can't update the clk pointers next to them. This is the same design that regulators has done so that's why it's written like this for clks. Honestly, we're talking about a handful of pointers here so I'm not sure it really matters much. Just duplicate the pointer and be done if you want to mark the array of names as const or have your const 'setup' structure point to the bulk_data array that you define statically non-const somewhere globally.
diff --git a/drivers/clk/clk-bulk.c b/drivers/clk/clk-bulk.c index 4c10456..2f16941 100644 --- a/drivers/clk/clk-bulk.c +++ b/drivers/clk/clk-bulk.c @@ -19,6 +19,22 @@ #include <linux/clk.h> #include <linux/device.h> #include <linux/export.h> +#include <linux/slab.h> + +struct clk_bulk_data *clk_bulk_alloc(int num_clocks, const char *const *clk_ids) +{ + struct clk_bulk_data *ptr; + int i; + + ptr = kcalloc(num_clocks, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + for (i = 0; i < num_clocks; i++) + ptr[i].id = clk_ids[i]; + + return ptr; +} void clk_bulk_put(int num_clks, struct clk_bulk_data *clks) { diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index d854e26..2115b97 100644 --- a/drivers/clk/clk-devres.c +++ b/drivers/clk/clk-devres.c @@ -9,6 +9,39 @@ #include <linux/export.h> #include <linux/gfp.h> +struct clk_bulk_devres { + struct clk_bulk_data *clks; + int num_clks; +}; + +static void devm_clk_alloc_release(struct device *dev, void *res) +{ + struct clk_bulk_devres *devres = res; + + clk_bulk_free(devres->clks); +} + +struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev, int num_clks, + const char *const *clk_ids) +{ + struct clk_bulk_data **ptr, *clk_bulk; + + ptr = devres_alloc(devm_clk_alloc_release, + num_clks * sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + clk_bulk = clk_bulk_alloc(num_clks, clk_ids); + if (clk_bulk) { + *ptr = clk_bulk; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return clk_bulk; +} + static void devm_clk_release(struct device *dev, void *res) { clk_put(*(struct clk **)res); @@ -34,10 +67,6 @@ struct clk *devm_clk_get(struct device *dev, const char *id) } EXPORT_SYMBOL(devm_clk_get); -struct clk_bulk_devres { - struct clk_bulk_data *clks; - int num_clks; -}; static void devm_clk_bulk_release(struct device *dev, void *res) { diff --git a/include/linux/clk.h b/include/linux/clk.h index 4c4ef9f..7d66f41 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -15,6 +15,7 @@ #include <linux/err.h> #include <linux/kernel.h> #include <linux/notifier.h> +#include <linux/slab.h> struct device; struct clk; @@ -240,6 +241,52 @@ static inline void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks) #endif #ifdef CONFIG_HAVE_CLK + +/** + * clk_bulk_alloc - allocates an array of clk_bulk_data and fills their + * id field + * @num_clks: number of clk_bulk_data + * @clk_ids: array of clock consumer ID's + * + * This function allows drivers to dynamically create an array of clk_bulk_data + * and fill their id field in one operation. If successful, it allows calling + * clk_bulk_get on the pointer returned by this function. + * + * Returns a pointer to a clk_bulk_data array, or valid IS_ERR() condition + * containing errno. + */ +struct clk_bulk_data *clk_bulk_alloc(int num_clks, const char *const *clk_ids); + +/** + * devm_clk_bulk_alloc - allocates an array of clk_bulk_data and fills their + * id field + * @dev: device for clock "consumer" + * @num_clks: number of clk_bulk_data + * @clk_ids: array of clock consumer ID's + * + * This function allows drivers to dynamically create an array of clk_bulk_data + * and fill their id field in one operation with management, the array will + * automatically be freed when the device is unbound. If successful, it allows + * calling clk_bulk_get on the pointer returned by this function. + * + * Returns a pointer to a clk_bulk_data array, or valid IS_ERR() condition + * containing errno. + */ +struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev, int num_clks, + const char * const *clk_ids); + +/** + * clk_bulk_free - frees the array of clk_bulk_data + * @clks: pointer to clk_bulk_data array + * + * This function frees the array allocated by clk_bulk_data. It must be called + * when all clks are freed. + */ +static inline void clk_bulk_free(struct clk_bulk_data *clks) +{ + kfree(clks); +} + /** * clk_get - lookup and obtain a reference to a clock producer. * @dev: device for clock "consumer" @@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id); #else /* !CONFIG_HAVE_CLK */ +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks, + const char **clk_ids) +{ + return NULL; +} + +static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev, + int num_clks, + const char **clk_ids) +{ + return NULL; +} + +static inline void clk_bulk_free(struct clk_bulk_data *clks) +{ +} + static inline struct clk *clk_get(struct device *dev, const char *id) { return NULL;
When a driver is going to use clk_bulk_get() function, it has to initialize an array of clk_bulk_data, by filling its id fields. Add a new function to the core, which dynamically allocates clk_bulk_data array and fills its id fields. Add clk_bulk_free() function, which frees the array allocated by clk_bulk_alloc() function. Add a managed version of clk_bulk_alloc(). Signed-off-by: Maciej Purski <m.purski@samsung.com> --- drivers/clk/clk-bulk.c | 16 ++++++++++++ drivers/clk/clk-devres.c | 37 +++++++++++++++++++++++++--- include/linux/clk.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 4 deletions(-)