| Message ID | 1380046016-5811-1-git-send-email-abrestic@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andrew, On Tuesday 24 of September 2013 11:06:51 Andrew Bresticker wrote: > The Exynos AudioSS clock controller will later be modified to allow > input clocks to be specified via device-tree in order to support > multiple Exynos SoCs. This will introduce a dependency on the core > SoC clock controller being initialized first so that the AudioSS driver > can look up its input clocks, but the order in which clock providers > are probed in of_clk_init() is not guaranteed. Since deferred probing > is not supported in of_clk_init() and the AudioSS block is not the core > controller, we can initialize it later as a platform device. > > Signed-off-by: Andrew Bresticker <abrestic@chromium.org> > --- > Changes since v2: > - add error handling to probe callback > - fixed ordering of of_clk_{add,del}_provider > - fixed nits from Tomasz and Sylwester > Changes since v1: > - add clk_unregister() calls to remove callback > - fixed minor nits from Tomasz > --- > drivers/clk/samsung/clk-exynos-audss.c | 109 > +++++++++++++++++++++++++++------ 1 file changed, 90 insertions(+), 19 > deletions(-) For the whole series: Acked-by: Tomasz Figa <t.figa@samsung.com> Thanks for addressing all the comments. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/24/2013 08:06 PM, Andrew Bresticker wrote: > +static int exynos_audss_clk_probe(struct platform_device *pdev) > { [...] > clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss", > mout_audss_p, ARRAY_SIZE(mout_audss_p), > @@ -123,13 +124,83 @@ static void __init exynos_audss_clk_init(struct device_node *np) > "div_pcm0", CLK_SET_RATE_PARENT, > reg_base + ASS_CLK_GATE, 5, 0,&lock); > > + for (i = 0; i< EXYNOS_AUDSS_MAX_CLKS; i++) { > + if (IS_ERR(clk_table[i])) { > + dev_err(&pdev->dev, "failed to regsiter clock %d\n", i); typo: regsiter -> register > + ret = PTR_ERR(clk_table[i]); > + goto unregister; > + } > + } > + > + clk_data.clks = clk_table; > + clk_data.clk_num = EXYNOS_AUDSS_MAX_CLKS; > + ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get, > + &clk_data); > + if (ret) { > + dev_err(&pdev->dev, "failed to add clock provider\n"); > + goto unregister; > + } > + [...] > + return 0; > + > +unregister: > + for (i = 0; i< EXYNOS_AUDSS_MAX_CLKS; i++) { > + if (!IS_ERR_OR_NULL(clk_table[i])) > + clk_unregister(clk_table[i]); > + } Couldn't this instead be: while (--i >= 0) clk_unregister(clk_table[i]); ? > +static int exynos_audss_clk_remove(struct platform_device *pdev) > +{ > + int i; > + > + of_clk_del_provider(pdev->dev.of_node); > + > + for (i = 0; i< EXYNOS_AUDSS_MAX_CLKS; i++) { > + if (!IS_ERR_OR_NULL(clk_table[i])) > + clk_unregister(clk_table[i]); > + } Since we only get here if all the clocks are registered properly and we always register EXYNOS_AUDSS_MAX_CLKS clocks, couldn't this simply be: for (i = 0; i < EXYNOS_AUDSS_MAX_CLKS; i++) clk_unregister(clk_table[i]); ? > + return 0; > +} Otherwise the whole series looks good to me. Feel free to add: Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com> -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> +static int exynos_audss_clk_remove(struct platform_device *pdev) >> +{ >> + int i; >> + >> + of_clk_del_provider(pdev->dev.of_node); >> + >> + for (i = 0; i< EXYNOS_AUDSS_MAX_CLKS; i++) { >> + if (!IS_ERR_OR_NULL(clk_table[i])) >> + clk_unregister(clk_table[i]); >> + } > > > Since we only get here if all the clocks are registered properly and we > always register EXYNOS_AUDSS_MAX_CLKS clocks, couldn't this simply be: > > > for (i = 0; i < EXYNOS_AUDSS_MAX_CLKS; i++) > clk_unregister(clk_table[i]); > > ? Once support is added for Exynos5420, we won't always register EXYNOS_AUDSS_MAX_CLKS clocks, so we'd still need the NULL check. Thanks, Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/24/13 11:06, Andrew Bresticker wrote: > +static int __init exynos_audss_clk_init(void) > +{ > + return platform_driver_register(&exynos_audss_clk_driver); > +} > +core_initcall(exynos_audss_clk_init); > + > +static void __init exynos_audss_clk_exit(void) __exit? > +{ > + platform_driver_unregister(&exynos_audss_clk_driver); > +} > +module_exit(exynos_audss_clk_exit); >
On 09/25/2013 12:12 AM, Andrew Bresticker wrote: >>> +static int exynos_audss_clk_remove(struct platform_device *pdev) >>> +{ >>> + int i; >>> + >>> + of_clk_del_provider(pdev->dev.of_node); >>> + >>> + for (i = 0; i< EXYNOS_AUDSS_MAX_CLKS; i++) { >>> + if (!IS_ERR_OR_NULL(clk_table[i])) >>> + clk_unregister(clk_table[i]); >>> + } >> >> >> Since we only get here if all the clocks are registered properly and we >> always register EXYNOS_AUDSS_MAX_CLKS clocks, couldn't this simply be: >> >> >> for (i = 0; i< EXYNOS_AUDSS_MAX_CLKS; i++) >> clk_unregister(clk_table[i]); >> >> ? > > Once support is added for Exynos5420, we won't always register > EXYNOS_AUDSS_MAX_CLKS clocks, so we'd still need the NULL check. Then couldn't you use clk_data.clk_num in this loop instead of EXYNOS_AUDSS_MAX_CLKS ? What I'm trying to avoid is assuming NULL as an invalid clock. It's not correct, clock pointers should be only tested with ERR_PTR(). Alternatively you could set the whole clk_table array to, e.g. ERR_PTR(-EINVAL) beforehand, but that shouldn't be necessary when clk_data.clk_num is used, unless there are holes in the array. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz Figa wrote: > > Hi Kukjin, Mike, > > On Monday 02 of December 2013 07:43:42 Kukjin Kim wrote: > > On 11/28/13 03:41, Mike Turquette wrote: > > > Quoting Padma Venkat (2013-11-25 22:29:44) > > >> Hi Mike and Kukjin, > > >> > > >> On Tue, Oct 8, 2013 at 10:23 PM, Andrew Bresticker > > >> <abrestic@chromium.org> wrote: > > >>> Hi Mike and Kukjin, > > >>> > > >>> Any decisions regarding this patchset? I believe all comments have > > >>> been addressed. > > >>> > > >>> Thanks, > > >>> Andrew > > >> > > >> Any decisions on this patchset. These can be applied directly on > > >> linux-samsung tree. > > > > > > For all of the clk parts: > > > > > > Acked-by: Mike Turquette<mturquette@linaro.org> > > > > > > I'm happy to take them in as part of a pull request. > > > > > OK, Mike, I will take this series into samsung tree and let me send a > > pull request for your clk tree to avoid useless merge conflicts. > - some persons in Cc > Hmm, I can't find this series in any tree, so I guess it has been lost > in action. > Oops, sorry about that :-( > Should I take it through samsung-clk tree with your acks? > Tomasz, yes please take this series with my ack. Thanks, Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c index 39b40aa..319c6e4 100644 --- a/drivers/clk/samsung/clk-exynos-audss.c +++ b/drivers/clk/samsung/clk-exynos-audss.c @@ -14,6 +14,8 @@ #include <linux/clk-provider.h> #include <linux/of_address.h> #include <linux/syscore_ops.h> +#include <linux/module.h> +#include <linux/platform_device.h> #include <dt-bindings/clk/exynos-audss-clk.h> @@ -62,24 +64,23 @@ static struct syscore_ops exynos_audss_clk_syscore_ops = { #endif /* CONFIG_PM_SLEEP */ /* register exynos_audss clocks */ -static void __init exynos_audss_clk_init(struct device_node *np) +static int exynos_audss_clk_probe(struct platform_device *pdev) { - reg_base = of_iomap(np, 0); - if (!reg_base) { - pr_err("%s: failed to map audss registers\n", __func__); - return; + int i, ret = 0; + struct resource *res; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + reg_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(reg_base)) { + dev_err(&pdev->dev, "failed to map audss registers\n"); + return PTR_ERR(reg_base); } - clk_table = kzalloc(sizeof(struct clk *) * EXYNOS_AUDSS_MAX_CLKS, + clk_table = devm_kzalloc(&pdev->dev, + sizeof(struct clk *) * EXYNOS_AUDSS_MAX_CLKS, GFP_KERNEL); - if (!clk_table) { - pr_err("%s: could not allocate clk lookup table\n", __func__); - return; - } - - clk_data.clks = clk_table; - clk_data.clk_num = EXYNOS_AUDSS_MAX_CLKS; - of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data); + if (!clk_table) + return -ENOMEM; clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss", mout_audss_p, ARRAY_SIZE(mout_audss_p), @@ -123,13 +124,83 @@ static void __init exynos_audss_clk_init(struct device_node *np) "div_pcm0", CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 5, 0, &lock); + for (i = 0; i < EXYNOS_AUDSS_MAX_CLKS; i++) { + if (IS_ERR(clk_table[i])) { + dev_err(&pdev->dev, "failed to regsiter clock %d\n", i); + ret = PTR_ERR(clk_table[i]); + goto unregister; + } + } + + clk_data.clks = clk_table; + clk_data.clk_num = EXYNOS_AUDSS_MAX_CLKS; + ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get, + &clk_data); + if (ret) { + dev_err(&pdev->dev, "failed to add clock provider\n"); + goto unregister; + } + #ifdef CONFIG_PM_SLEEP register_syscore_ops(&exynos_audss_clk_syscore_ops); #endif - pr_info("Exynos: Audss: clock setup completed\n"); + dev_info(&pdev->dev, "setup completed\n"); + + return 0; + +unregister: + for (i = 0; i < EXYNOS_AUDSS_MAX_CLKS; i++) { + if (!IS_ERR_OR_NULL(clk_table[i])) + clk_unregister(clk_table[i]); + } + + return ret; } -CLK_OF_DECLARE(exynos4210_audss_clk, "samsung,exynos4210-audss-clock", - exynos_audss_clk_init); -CLK_OF_DECLARE(exynos5250_audss_clk, "samsung,exynos5250-audss-clock", - exynos_audss_clk_init); + +static int exynos_audss_clk_remove(struct platform_device *pdev) +{ + int i; + + of_clk_del_provider(pdev->dev.of_node); + + for (i = 0; i < EXYNOS_AUDSS_MAX_CLKS; i++) { + if (!IS_ERR_OR_NULL(clk_table[i])) + clk_unregister(clk_table[i]); + } + + return 0; +} + +static const struct of_device_id exynos_audss_clk_of_match[] = { + { .compatible = "samsung,exynos4210-audss-clock", }, + { .compatible = "samsung,exynos5250-audss-clock", }, + {}, +}; + +static struct platform_driver exynos_audss_clk_driver = { + .driver = { + .name = "exynos-audss-clk", + .owner = THIS_MODULE, + .of_match_table = exynos_audss_clk_of_match, + }, + .probe = exynos_audss_clk_probe, + .remove = exynos_audss_clk_remove, +}; + +static int __init exynos_audss_clk_init(void) +{ + return platform_driver_register(&exynos_audss_clk_driver); +} +core_initcall(exynos_audss_clk_init); + +static void __init exynos_audss_clk_exit(void) +{ + platform_driver_unregister(&exynos_audss_clk_driver); +} +module_exit(exynos_audss_clk_exit); + +MODULE_AUTHOR("Padmavathi Venna <padma.v@samsung.com>"); +MODULE_DESCRIPTION("Exynos Audio Subsystem Clock Controller"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:exynos-audss-clk");
The Exynos AudioSS clock controller will later be modified to allow input clocks to be specified via device-tree in order to support multiple Exynos SoCs. This will introduce a dependency on the core SoC clock controller being initialized first so that the AudioSS driver can look up its input clocks, but the order in which clock providers are probed in of_clk_init() is not guaranteed. Since deferred probing is not supported in of_clk_init() and the AudioSS block is not the core controller, we can initialize it later as a platform device. Signed-off-by: Andrew Bresticker <abrestic@chromium.org> --- Changes since v2: - add error handling to probe callback - fixed ordering of of_clk_{add,del}_provider - fixed nits from Tomasz and Sylwester Changes since v1: - add clk_unregister() calls to remove callback - fixed minor nits from Tomasz --- drivers/clk/samsung/clk-exynos-audss.c | 109 +++++++++++++++++++++++++++------ 1 file changed, 90 insertions(+), 19 deletions(-)