| Message ID | 1516375101-29836-3-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Geert, Am Freitag, 19. Januar 2018, 16:18:20 CET schrieb Geert Uytterhoeven: > As of_clk_get_parent_count() returns zero on failure, while > of_count_phandle_with_args() might return a negative error code, this > also fixes the issue of possibly using a negative number in the > allocation below. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/soc/rockchip/pm_domains.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c > index 5c342167b9db7a16..9936038d38771784 100644 > --- a/drivers/soc/rockchip/pm_domains.c > +++ b/drivers/soc/rockchip/pm_domains.c > @@ -16,6 +16,7 @@ > #include <linux/of_address.h> > #include <linux/of_platform.h> > #include <linux/clk.h> > +#include <linux/clk-provider.h> not wanting to be to nitpicky, but clk-provider.h seems that it should be used from drivers exposing clocks to the system, not from clock users. I just remember that in the past clk maintainers tried to keep their layering intact, so I've included them for clarification. Mike, Stephen: the power-domain driver of Rockchip socs needs to toggle all clocks of domain peripherals when turning a domain on or off, so has a variable list of clocks in the clocks property in dt. Hence the of_count_... call which could be replaced with of_clk_get_parent_count to not open-code this. Heiko > #include <linux/regmap.h> > #include <linux/mfd/syscon.h> > #include <dt-bindings/power/rk3288-power.h> > @@ -391,7 +392,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu, > return -EINVAL; > } > > - clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells"); > + clk_cnt = of_clk_get_parent_count(node); > pd = devm_kzalloc(pmu->dev, > sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]), > GFP_KERNEL); >
Heiko, Geert, On Sat, Feb 10, 2018 at 8:35 AM, Heiko Stuebner <heiko@sntech.de> wrote: > Hi Geert, > > Am Freitag, 19. Januar 2018, 16:18:20 CET schrieb Geert Uytterhoeven: >> As of_clk_get_parent_count() returns zero on failure, while >> of_count_phandle_with_args() might return a negative error code, this >> also fixes the issue of possibly using a negative number in the >> allocation below. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> drivers/soc/rockchip/pm_domains.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c >> index 5c342167b9db7a16..9936038d38771784 100644 >> --- a/drivers/soc/rockchip/pm_domains.c >> +++ b/drivers/soc/rockchip/pm_domains.c >> @@ -16,6 +16,7 @@ >> #include <linux/of_address.h> >> #include <linux/of_platform.h> >> #include <linux/clk.h> >> +#include <linux/clk-provider.h> > > not wanting to be to nitpicky, but clk-provider.h seems that it should be > used from drivers exposing clocks to the system, not from clock users. > > I just remember that in the past clk maintainers tried to keep their > layering intact, so I've included them for clarification. > > Mike, Stephen: the power-domain driver of Rockchip socs needs > to toggle all clocks of domain peripherals when turning a domain > on or off, so has a variable list of clocks in the clocks property in dt. > Hence the of_count_... call which could be replaced with > of_clk_get_parent_count to not open-code this. Moving of_clk_get_parent_count to clk.h seems reasonable to me. It's more of an of-specific helper than a provider-specific helper. Anyone up to submit a patch so that this one doesn't need to include clk-provider.h? Regards, Mike > > > Heiko > >> #include <linux/regmap.h> >> #include <linux/mfd/syscon.h> >> #include <dt-bindings/power/rk3288-power.h> >> @@ -391,7 +392,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu, >> return -EINVAL; >> } >> >> - clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells"); >> + clk_cnt = of_clk_get_parent_count(node); >> pd = devm_kzalloc(pmu->dev, >> sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]), >> GFP_KERNEL); >> > >
Hi Mike, On Sun, Feb 18, 2018 at 8:09 PM, Michael Turquette <mturquette@baylibre.com> wrote: > On Sat, Feb 10, 2018 at 8:35 AM, Heiko Stuebner <heiko@sntech.de> wrote: >> Am Freitag, 19. Januar 2018, 16:18:20 CET schrieb Geert Uytterhoeven: >>> As of_clk_get_parent_count() returns zero on failure, while >>> of_count_phandle_with_args() might return a negative error code, this >>> also fixes the issue of possibly using a negative number in the >>> allocation below. >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> --- >>> drivers/soc/rockchip/pm_domains.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c >>> index 5c342167b9db7a16..9936038d38771784 100644 >>> --- a/drivers/soc/rockchip/pm_domains.c >>> +++ b/drivers/soc/rockchip/pm_domains.c >>> @@ -16,6 +16,7 @@ >>> #include <linux/of_address.h> >>> #include <linux/of_platform.h> >>> #include <linux/clk.h> >>> +#include <linux/clk-provider.h> >> >> not wanting to be to nitpicky, but clk-provider.h seems that it should be >> used from drivers exposing clocks to the system, not from clock users. >> >> I just remember that in the past clk maintainers tried to keep their >> layering intact, so I've included them for clarification. >> >> Mike, Stephen: the power-domain driver of Rockchip socs needs >> to toggle all clocks of domain peripherals when turning a domain >> on or off, so has a variable list of clocks in the clocks property in dt. >> Hence the of_count_... call which could be replaced with >> of_clk_get_parent_count to not open-code this. > > Moving of_clk_get_parent_count to clk.h seems reasonable to me. It's > more of an of-specific helper than a provider-specific helper. > > Anyone up to submit a patch so that this one doesn't need to include > clk-provider.h? I gave it a try. Just moving of_clk_get_parent_count() from clk-provider.h to clk.h causes breakage, as half of the clock providers using of_clk_get_parent_count() do not include <linux/clk.h>: drivers/clk/at91/clk-generated.c drivers/clk/at91/clk-main.c drivers/clk/at91/clk-master.c drivers/clk/at91/clk-programmable.c drivers/clk/at91/clk-slow.c drivers/clk/at91/clk-smd.c drivers/clk/at91/clk-usb.c drivers/clk/at91/sckc.c drivers/clk/clk-axi-clkgen.c drivers/clk/clk-gpio.c drivers/clk/clk-hsdk-pll.c drivers/clk/h8300/clk-div.c drivers/clk/h8300/clk-h8s2678.c drivers/clk/renesas/clk-div6.c drivers/clk/ti/composite.c drivers/clk/ti/gate.c drivers/clk/ti/mux.c So our options are: 1. Add #include <linux/clk.h> to each of the above, 2. Add #include <linux/clk.h> to <linux/clk-provider.h>, 3. Do nothing, i.e. users of of_clk_get_parent_count () need to #include <linux/clk-provider.h>, cfr. this patch series. Any preference? Any option I'm missing? Thanks! >>> #include <linux/regmap.h> >>> #include <linux/mfd/syscon.h> >>> #include <dt-bindings/power/rk3288-power.h> >>> @@ -391,7 +392,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu, >>> return -EINVAL; >>> } >>> >>> - clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells"); >>> + clk_cnt = of_clk_get_parent_count(node); >>> pd = devm_kzalloc(pmu->dev, >>> sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]), >>> GFP_KERNEL); >>> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Quoting Geert Uytterhoeven (2018-03-02 05:15:35) > Hi Mike, > > On Sun, Feb 18, 2018 at 8:09 PM, Michael Turquette > <mturquette@baylibre.com> wrote: > > Moving of_clk_get_parent_count to clk.h seems reasonable to me. It's > > more of an of-specific helper than a provider-specific helper. > > > > Anyone up to submit a patch so that this one doesn't need to include > > clk-provider.h? > > I gave it a try. > Just moving of_clk_get_parent_count() from clk-provider.h to clk.h causes > breakage, as half of the clock providers using of_clk_get_parent_count() > do not include <linux/clk.h>: > > drivers/clk/at91/clk-generated.c > drivers/clk/at91/clk-main.c > drivers/clk/at91/clk-master.c > drivers/clk/at91/clk-programmable.c > drivers/clk/at91/clk-slow.c > drivers/clk/at91/clk-smd.c > drivers/clk/at91/clk-usb.c > drivers/clk/at91/sckc.c > drivers/clk/clk-axi-clkgen.c > drivers/clk/clk-gpio.c > drivers/clk/clk-hsdk-pll.c > drivers/clk/h8300/clk-div.c > drivers/clk/h8300/clk-h8s2678.c > drivers/clk/renesas/clk-div6.c > drivers/clk/ti/composite.c > drivers/clk/ti/gate.c > drivers/clk/ti/mux.c > > So our options are: > 1. Add #include <linux/clk.h> to each of the above, > 2. Add #include <linux/clk.h> to <linux/clk-provider.h>, > 3. Do nothing, i.e. users of of_clk_get_parent_count () need to > #include <linux/clk-provider.h>, cfr. this patch series. > > Any preference? Any option I'm missing? > #2 is a definite no. How about option #4? Move of_clk_get_parent_count() and of_clk_get_parent_name() to a new include/linux/of_clk.h, include that in clk-provider.h, and then have non-clk provider drivers include that file? After that, we can also move of_clk_init() into this new file, and then non-provider arch code can call of_clk_init() without including the provider APIs. I'd also like to get rid of the include of linux/of.h in clk-provider.h, but that's a ways away.
diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c index 5c342167b9db7a16..9936038d38771784 100644 --- a/drivers/soc/rockchip/pm_domains.c +++ b/drivers/soc/rockchip/pm_domains.c @@ -16,6 +16,7 @@ #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/clk.h> +#include <linux/clk-provider.h> #include <linux/regmap.h> #include <linux/mfd/syscon.h> #include <dt-bindings/power/rk3288-power.h> @@ -391,7 +392,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu, return -EINVAL; } - clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells"); + clk_cnt = of_clk_get_parent_count(node); pd = devm_kzalloc(pmu->dev, sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]), GFP_KERNEL);
As of_clk_get_parent_count() returns zero on failure, while of_count_phandle_with_args() might return a negative error code, this also fixes the issue of possibly using a negative number in the allocation below. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/soc/rockchip/pm_domains.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)