| Message ID | 1507166751-2012-1-git-send-email-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 10/05, Masahiro Yamada wrote: > The clock consumer, drivers/video/fbdev/simplefb.c, includes > <linux/clk-provider.h> just for calling of_clk_get_parent_count(). > This is ugly. > > Looking at simplefb_clocks_get(), of_clk_get_parent_count() seems > useful for clock consumers as well as for clock providers. > > Unfortunately, we do not have a good home for declarations shared > between consumers and providers. > > Create a new header <linux/__clk.h>, and move it over to it. This > header must be included via <linux/clk.h> or <linux/clk-provider.h> > (this is why it is prefixed with double-underscore). Add #error > so the build terminates if it is included directly. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- How about we add a get_all_the_clks_for_the_device() sort of API that uses the bulk clk code but also allocates the array by reading the number of clks from DT? Please think of a better name of course. We can figure out how to handle non-DT platforms if we need to. With clkdev we could probably handle it with some sort of lookup table search given a device name. Don't add that part until we have a user though. I assume simplefb is all DT platforms? Either way, it looks like what we really want here is a way to get every clk for a device and not look at the details. USB has a similar case, which I think Shawn Guo/Dong Aisheng was trying to add an OF based bulk clk_get() API called of_clk_bulk_get() for[1]. If this get all clks API works there too then we should use it. If it can be non-DT specific, even better. [1] http://lkml.kernel.org/r/1506415441-4435-1-git-send-email-aisheng.dong@nxp.com
Hi Stephen, 2017-10-13 8:17 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>: > On 10/05, Masahiro Yamada wrote: >> The clock consumer, drivers/video/fbdev/simplefb.c, includes >> <linux/clk-provider.h> just for calling of_clk_get_parent_count(). >> This is ugly. >> >> Looking at simplefb_clocks_get(), of_clk_get_parent_count() seems >> useful for clock consumers as well as for clock providers. >> >> Unfortunately, we do not have a good home for declarations shared >> between consumers and providers. >> >> Create a new header <linux/__clk.h>, and move it over to it. This >> header must be included via <linux/clk.h> or <linux/clk-provider.h> >> (this is why it is prefixed with double-underscore). Add #error >> so the build terminates if it is included directly. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- > > How about we add a get_all_the_clks_for_the_device() sort of API > that uses the bulk clk code but also allocates the array by > reading the number of clks from DT? Please think of a better name > of course. We can figure out how to handle non-DT platforms if we > need to. With clkdev we could probably handle it with some sort > of lookup table search given a device name. Don't add that part > until we have a user though. I assume simplefb is all DT > platforms? of_reset_control_array_get() is implemented like that. It gets all the resets specified in DT, and automatically allocates the array of necessary number of pointers. clk-bulk and reset-array were implemented in a different way. > Either way, it looks like what we really want here is a way to > get every clk for a device and not look at the details. Yes. This is helpful for generic drivers. > USB has a > similar case, which I think Shawn Guo/Dong Aisheng was trying to > add an OF based bulk clk_get() API called of_clk_bulk_get() > for[1]. If this get all clks API works there too then we should > use it. If it can be non-DT specific, even better. > > [1] http://lkml.kernel.org/r/1506415441-4435-1-git-send-email-aisheng.dong@nxp.com OK, we can implement it based on Shawn/Dong's work. My concern is clk-bulk.c still needs to include #<linux/clk-provider.h>, but it is a bit odd since clk-bulk.c is a helper for clk consumers.
On 10/29, Masahiro Yamada wrote: > 2017-10-13 8:17 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>: > > > USB has a > > similar case, which I think Shawn Guo/Dong Aisheng was trying to > > add an OF based bulk clk_get() API called of_clk_bulk_get() > > for[1]. If this get all clks API works there too then we should > > use it. If it can be non-DT specific, even better. > > > > [1] http://lkml.kernel.org/r/1506415441-4435-1-git-send-email-aisheng.dong@nxp.com > > OK, we can implement it based on Shawn/Dong's work. > > My concern is clk-bulk.c still needs to include #<linux/clk-provider.h>, > but it is a bit odd since clk-bulk.c is a helper for clk consumers. > > Understood. I'm not concerned though, it's one place where this happens, not in a bunch of random drivers. What exactly do you need from clk-provider.h in clk-bulk.c though?
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c index a3c44ec..17f0aec 100644 --- a/drivers/video/fbdev/simplefb.c +++ b/drivers/video/fbdev/simplefb.c @@ -27,7 +27,6 @@ #include <linux/platform_data/simplefb.h> #include <linux/platform_device.h> #include <linux/clk.h> -#include <linux/clk-provider.h> #include <linux/of.h> #include <linux/of_platform.h> #include <linux/parser.h> diff --git a/include/linux/__clk.h b/include/linux/__clk.h new file mode 100644 index 0000000..a8b86bf --- /dev/null +++ b/include/linux/__clk.h @@ -0,0 +1,31 @@ +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __LINUX___CLK_H +#define __LINUX___CLK_H + +/* This header contains stuff shared between consumers and providers. */ + +#if !defined(__LINUX_CLK_H) && !defined(__LINUX_CLK_PROVIDER_H) +#error "Please don't include <linux/__clk.h> directly, include <linux/clk.h> or <linux/clk-provider.h> instead." +#endif + +struct device_node; + +#if defined(CONFIG_COMMON_CLK) && defined(CONFIG_OF) + +unsigned int of_clk_get_parent_count(struct device_node *np); + +#else + +static inline unsigned int of_clk_get_parent_count(struct device_node *np) +{ + return 0; +} + +#endif + +#endif /* __LINUX___CLK_H */ diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 5100ec1..cd9bca1 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -14,6 +14,8 @@ #include <linux/io.h> #include <linux/of.h> +#include "__clk.h" + #ifdef CONFIG_COMMON_CLK /* @@ -823,7 +825,6 @@ struct clk_hw *of_clk_hw_simple_get(struct of_phandle_args *clkspec, struct clk *of_clk_src_onecell_get(struct of_phandle_args *clkspec, void *data); struct clk_hw *of_clk_hw_onecell_get(struct of_phandle_args *clkspec, void *data); -unsigned int of_clk_get_parent_count(struct device_node *np); int of_clk_parent_fill(struct device_node *np, const char **parents, unsigned int size); const char *of_clk_get_parent_name(struct device_node *np, int index); @@ -868,10 +869,6 @@ of_clk_hw_onecell_get(struct of_phandle_args *clkspec, void *data) { return ERR_PTR(-ENOENT); } -static inline unsigned int of_clk_get_parent_count(struct device_node *np) -{ - return 0; -} static inline int of_clk_parent_fill(struct device_node *np, const char **parents, unsigned int size) { diff --git a/include/linux/clk.h b/include/linux/clk.h index 12c96d9..83d24cb 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -16,6 +16,8 @@ #include <linux/kernel.h> #include <linux/notifier.h> +#include "__clk.h" + struct device; struct clk; struct device_node;
The clock consumer, drivers/video/fbdev/simplefb.c, includes <linux/clk-provider.h> just for calling of_clk_get_parent_count(). This is ugly. Looking at simplefb_clocks_get(), of_clk_get_parent_count() seems useful for clock consumers as well as for clock providers. Unfortunately, we do not have a good home for declarations shared between consumers and providers. Create a new header <linux/__clk.h>, and move it over to it. This header must be included via <linux/clk.h> or <linux/clk-provider.h> (this is why it is prefixed with double-underscore). Add #error so the build terminates if it is included directly. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- drivers/video/fbdev/simplefb.c | 1 - include/linux/__clk.h | 31 +++++++++++++++++++++++++++++++ include/linux/clk-provider.h | 7 ++----- include/linux/clk.h | 2 ++ 4 files changed, 35 insertions(+), 6 deletions(-) create mode 100644 include/linux/__clk.h