| Message ID | 1379711637-5226-1-git-send-email-abrestic@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andrew, This patch looks good overall, but I have some minor comments inline. On Friday 20 of September 2013 14:13:52 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> > --- > drivers/clk/samsung/clk-exynos-audss.c | 71 > +++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 13 > deletions(-) [snip] > +static int exynos_audss_clk_remove(struct platform_device *pdev) > +{ > + of_clk_del_provider(pdev->dev.of_node); > + > + return 0; > } Don't we need to unregister all the registered clocks in remove? This also leads to another question: Do we even need removal support for this driver? > -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 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); Does it need to be core_initcall? Drivers depending on clocks provided by this driver should be able to defer probing if they are probed before this driver. Then you would be able to simply use module_platform_driver() below. > +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 AudioSS Clock Controller"); nit: IMHO Audio Subsystem instead of AudioSS would be more meaningful. > +MODULE_LICENSE("GPL"); This should be GPL v2. Best regards, Tomasz
Hi Tomasz, >> +static int exynos_audss_clk_remove(struct platform_device *pdev) >> +{ >> + of_clk_del_provider(pdev->dev.of_node); >> + >> + return 0; >> } > > Don't we need to unregister all the registered clocks in remove? This also > leads to another question: Do we even need removal support for this > driver? Agreed - I don't think we should support removal of this device, but it looks like __device_release_driver() just ignores the lack of a remove callback or the return value from remove. I suppose we could just yell that removal is not supported if it is ever attempted. >> +static int __init exynos_audss_clk_init(void) >> +{ >> + return platform_driver_register(&exynos_audss_clk_driver); >> +} >> +core_initcall(exynos_audss_clk_init); > > Does it need to be core_initcall? Drivers depending on clocks provided by > this driver should be able to defer probing if they are probed before this > driver. Unfortunately there are a couple of issues with making this a module_initcall: 1. On the Exynos5420, the AudioSS block provides the apb_pclk gate for the ADMA bus, which is probed at postcore_initcall time and does not support deferred probing, and 2. the common clock framework doesn't differentiate between the clock not being specified at all and the clock being specified, but the provider not being registered yet (i.e. the case where probe deferral would be appropriate) - it just returns ENOENT in both cases. Given that this driver has no dependencies other than the core clock controller, it should be fine to initialize at core_initcall time. Both of these issues could be fixed, though. Thanks, Andrew
On Monday 23 of September 2013 14:25:12 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); > > > > Does it need to be core_initcall? Drivers depending on clocks provided > > by this driver should be able to defer probing if they are probed > > before this driver. > > Unfortunately there are a couple of issues with making this a > module_initcall: 1. On the Exynos5420, the AudioSS block provides the > apb_pclk gate for the ADMA bus, which is probed at postcore_initcall > time and does not support deferred probing, and Just out of curiosity, what is this ADMA bus? Anyway, I'm fine with this, just wanted to make sure that there is a reason behind it. Best regards, Tomasz
On Mon, Sep 23, 2013 at 2:30 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On Monday 23 of September 2013 14:25:12 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); >> > >> > Does it need to be core_initcall? Drivers depending on clocks provided >> > by this driver should be able to defer probing if they are probed >> > before this driver. >> >> Unfortunately there are a couple of issues with making this a >> module_initcall: 1. On the Exynos5420, the AudioSS block provides the >> apb_pclk gate for the ADMA bus, which is probed at postcore_initcall >> time and does not support deferred probing, and > > Just out of curiosity, what is this ADMA bus? It's the DMA controller used for PCM and I2S. Looks like the DMA nodes aren't in the 5420 device-tree yet... -Andrew
Hi, On 09/23/2013 11:25 PM, Andrew Bresticker wrote: >>> +static int exynos_audss_clk_remove(struct platform_device *pdev) >>> +{ >>> + of_clk_del_provider(pdev->dev.of_node); >>> + >>> + return 0; >>> } >> >> Don't we need to unregister all the registered clocks in remove? This also >> leads to another question: Do we even need removal support for this >> driver? > > Agreed - I don't think we should support removal of this device, but > it looks like __device_release_driver() just ignores the lack of a > remove callback or the return value from remove. I suppose we could > just yell that removal is not supported if it is ever attempted. That might be a good idea, without proper remove() method deferred probing will also not work. I'd assume there should be only, e.g. WARN() in the remove() callback or it should be properly implemented, with clk_unregister() call for each currently registered clock. Note that clk_unregister() is currently not implemented and removal of this driver cannot be properly supported at the moment anyway. Not sure what's more appropriate, it's probably better to add clk_unregister() calls. This would be effectively a dead code though, as long as core_initcall is used. >>> +static int __init exynos_audss_clk_init(void) >>> +{ >>> + return platform_driver_register(&exynos_audss_clk_driver); >>> +} >>> +core_initcall(exynos_audss_clk_init); >> >> Does it need to be core_initcall? Drivers depending on clocks provided by >> this driver should be able to defer probing if they are probed before this >> driver. > > Unfortunately there are a couple of issues with making this a module_initcall: > 1. On the Exynos5420, the AudioSS block provides the apb_pclk gate > for the ADMA bus, which is probed at postcore_initcall time and does > not support deferred probing, and > 2. the common clock framework doesn't differentiate between the > clock not being specified at all and the clock being specified, but > the provider not being registered yet (i.e. the case where probe > deferral would be appropriate) - it just returns ENOENT in both cases. AFAICS this shouldn't be difficult to improve. I guess it has not been properly addressed so far because there is currently no properly working modular clock provider drivers, using the common clock framework, yet. Unless someone bits me to it, I might have a look at that, as I also found it a bit it inconvenient. -- Regards, Sylwester
diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c index 39b40aa..7571e88 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,29 @@ 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; + 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; + dev_err(&pdev->dev, "could not allocate clk lookup table\n"); + return -ENOMEM; } 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); + of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get, + &clk_data); clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss", mout_audss_p, ARRAY_SIZE(mout_audss_p), @@ -128,8 +135,46 @@ static void __init exynos_audss_clk_init(struct device_node *np) #endif pr_info("Exynos: Audss: clock setup completed\n"); + + return 0; +} + +static int exynos_audss_clk_remove(struct platform_device *pdev) +{ + of_clk_del_provider(pdev->dev.of_node); + + return 0; } -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 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 AudioSS Clock Controller"); +MODULE_LICENSE("GPL"); +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> --- drivers/clk/samsung/clk-exynos-audss.c | 71 +++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 13 deletions(-)