From: Stephen Boyd <sboyd@codeaurora.org> To: Russell King - ARM Linux <linux@arm.linux.org.uk> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>, Tomeu Vizoso <tomeu.vizoso@collabora.com>, Paul Walmsley <paul@pwsan.com>, Tony Lindgren <tony@atomide.com>, linux-kernel@vger.kernel.org, Mike Turquette <mturquette@linaro.org>, linux-omap@vger.kernel.org, Javier Martinez Canillas <javier.martinez@collabora.co.uk>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances Date: Fri, 06 Feb 2015 11:30:18 -0800 [thread overview] Message-ID: <54D5164A.30503@codeaurora.org> (raw) In-Reply-To: <20150206133920.GC8670@n2100.arm.linux.org.uk> On 02/06/15 05:39, Russell King - ARM Linux wrote: > On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote: > >> From what I can tell this code is >> now broken because we made all clk getting functions (there's quite a >> few...) return unique pointers every time they're called. It seems that >> the driver wants to know if extclk and clk are the same so it can do >> something differently in kirkwood_set_rate(). Do we need some sort of >> clk_equal(struct clk *a, struct clk *b) function for drivers like this? > Well, the clocks in question are the SoC internal clock (which is more or > less fixed, but has a programmable divider) and an externally supplied > clock, and the IP has a multiplexer on its input which allows us to select > between those two sources. > > If it were possible to bind both to the same clock, it wouldn't be a > useful configuration - nothing would be gained from doing so in terms of > available rates. > > What the comparison is there for is to catch the case with legacy lookups > where a clkdev lookup entry with a NULL connection ID results in matching > any connection ID passed to clk_get(). If the patch changes this, then > we will have a regression - and this is something which needs fixing > _before_ we do this "return unique clocks". > Ok. It seems that we might need a clk_equal() or similar API after all. My understanding is that this driver is calling clk_get() twice with NULL for the con_id and then "extclk" in attempts to get the SoC internal clock and the externally supplied clock. If we're using legacy lookups then both clk_get() calls may map to the same clk_lookup entry and before Tomeu's patch that returns unique clocks the driver could detect this case and know that there isn't an external clock. Looking at arch/arm/mach-dove/common.c it seems that there is only one lookup per device and it has a wildcard NULL for con_id so both clk_get() calls here are going to find the same lookup and get a unique struct clk pointer. Why don't we make the legacy lookup more specific and actually indicate "internal" for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. ----8<---- From: Stephen Boyd <sboyd@codeaurora.org> Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup This i2s driver is using the wildcard nature of clkdev lookups to figure out if there's an external clock or not. It does this by calling clk_get() twice with NULL for the con_id first and then "external" for the con_id the second time around and then compares the two pointers. With DT the wildcard feature of clk_get() is gone and so the driver has to handle an error from the second clk_get() call as meaning "no external clock specified". Let's use that logic even with clk lookups to simplify the code and remove the struct clk pointer comparisons which may not work in the future when clk_get() returns unique pointers. Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- arch/arm/mach-dove/common.c | 4 ++-- sound/soc/kirkwood/kirkwood-i2s.c | 13 ++++--------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c index 0d1a89298ece..f290fc944cc1 100644 --- a/arch/arm/mach-dove/common.c +++ b/arch/arm/mach-dove/common.c @@ -124,8 +124,8 @@ static void __init dove_clk_init(void) orion_clkdev_add(NULL, "sdhci-dove.1", sdio1); orion_clkdev_add(NULL, "orion_nand", nand); orion_clkdev_add(NULL, "cafe1000-ccic.0", camera); - orion_clkdev_add(NULL, "mvebu-audio.0", i2s0); - orion_clkdev_add(NULL, "mvebu-audio.1", i2s1); + orion_clkdev_add("internal", "mvebu-audio.0", i2s0); + orion_clkdev_add("internal", "mvebu-audio.1", i2s1); orion_clkdev_add(NULL, "mv_crypto", crypto); orion_clkdev_add(NULL, "dove-ac97", ac97); orion_clkdev_add(NULL, "dove-pdma", pdma); diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index def7d8260c4e..0bfeb712a997 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) return -EINVAL; } - priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL); + priv->clk = devm_clk_get(&pdev->dev, "internal"); if (IS_ERR(priv->clk)) { dev_err(&pdev->dev, "no clock\n"); return PTR_ERR(priv->clk); @@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) if (PTR_ERR(priv->extclk) == -EPROBE_DEFER) return -EPROBE_DEFER; } else { - if (priv->extclk == priv->clk) { - devm_clk_put(&pdev->dev, priv->extclk); - priv->extclk = ERR_PTR(-EINVAL); - } else { - dev_info(&pdev->dev, "found external clock\n"); - clk_prepare_enable(priv->extclk); - soc_dai = kirkwood_i2s_dai_extclk; - } + dev_info(&pdev->dev, "found external clock\n"); + clk_prepare_enable(priv->extclk); + soc_dai = kirkwood_i2s_dai_extclk; } /* Some sensible defaults - this reflects the powerup values */ -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
WARNING: multiple messages have this Message-ID (diff)
From: sboyd@codeaurora.org (Stephen Boyd) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances Date: Fri, 06 Feb 2015 11:30:18 -0800 [thread overview] Message-ID: <54D5164A.30503@codeaurora.org> (raw) In-Reply-To: <20150206133920.GC8670@n2100.arm.linux.org.uk> On 02/06/15 05:39, Russell King - ARM Linux wrote: > On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote: > >> From what I can tell this code is >> now broken because we made all clk getting functions (there's quite a >> few...) return unique pointers every time they're called. It seems that >> the driver wants to know if extclk and clk are the same so it can do >> something differently in kirkwood_set_rate(). Do we need some sort of >> clk_equal(struct clk *a, struct clk *b) function for drivers like this? > Well, the clocks in question are the SoC internal clock (which is more or > less fixed, but has a programmable divider) and an externally supplied > clock, and the IP has a multiplexer on its input which allows us to select > between those two sources. > > If it were possible to bind both to the same clock, it wouldn't be a > useful configuration - nothing would be gained from doing so in terms of > available rates. > > What the comparison is there for is to catch the case with legacy lookups > where a clkdev lookup entry with a NULL connection ID results in matching > any connection ID passed to clk_get(). If the patch changes this, then > we will have a regression - and this is something which needs fixing > _before_ we do this "return unique clocks". > Ok. It seems that we might need a clk_equal() or similar API after all. My understanding is that this driver is calling clk_get() twice with NULL for the con_id and then "extclk" in attempts to get the SoC internal clock and the externally supplied clock. If we're using legacy lookups then both clk_get() calls may map to the same clk_lookup entry and before Tomeu's patch that returns unique clocks the driver could detect this case and know that there isn't an external clock. Looking at arch/arm/mach-dove/common.c it seems that there is only one lookup per device and it has a wildcard NULL for con_id so both clk_get() calls here are going to find the same lookup and get a unique struct clk pointer. Why don't we make the legacy lookup more specific and actually indicate "internal" for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. ----8<---- From: Stephen Boyd <sboyd@codeaurora.org> Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup This i2s driver is using the wildcard nature of clkdev lookups to figure out if there's an external clock or not. It does this by calling clk_get() twice with NULL for the con_id first and then "external" for the con_id the second time around and then compares the two pointers. With DT the wildcard feature of clk_get() is gone and so the driver has to handle an error from the second clk_get() call as meaning "no external clock specified". Let's use that logic even with clk lookups to simplify the code and remove the struct clk pointer comparisons which may not work in the future when clk_get() returns unique pointers. Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- arch/arm/mach-dove/common.c | 4 ++-- sound/soc/kirkwood/kirkwood-i2s.c | 13 ++++--------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c index 0d1a89298ece..f290fc944cc1 100644 --- a/arch/arm/mach-dove/common.c +++ b/arch/arm/mach-dove/common.c @@ -124,8 +124,8 @@ static void __init dove_clk_init(void) orion_clkdev_add(NULL, "sdhci-dove.1", sdio1); orion_clkdev_add(NULL, "orion_nand", nand); orion_clkdev_add(NULL, "cafe1000-ccic.0", camera); - orion_clkdev_add(NULL, "mvebu-audio.0", i2s0); - orion_clkdev_add(NULL, "mvebu-audio.1", i2s1); + orion_clkdev_add("internal", "mvebu-audio.0", i2s0); + orion_clkdev_add("internal", "mvebu-audio.1", i2s1); orion_clkdev_add(NULL, "mv_crypto", crypto); orion_clkdev_add(NULL, "dove-ac97", ac97); orion_clkdev_add(NULL, "dove-pdma", pdma); diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index def7d8260c4e..0bfeb712a997 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) return -EINVAL; } - priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL); + priv->clk = devm_clk_get(&pdev->dev, "internal"); if (IS_ERR(priv->clk)) { dev_err(&pdev->dev, "no clock\n"); return PTR_ERR(priv->clk); @@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) if (PTR_ERR(priv->extclk) == -EPROBE_DEFER) return -EPROBE_DEFER; } else { - if (priv->extclk == priv->clk) { - devm_clk_put(&pdev->dev, priv->extclk); - priv->extclk = ERR_PTR(-EINVAL); - } else { - dev_info(&pdev->dev, "found external clock\n"); - clk_prepare_enable(priv->extclk); - soc_dai = kirkwood_i2s_dai_extclk; - } + dev_info(&pdev->dev, "found external clock\n"); + clk_prepare_enable(priv->extclk); + soc_dai = kirkwood_i2s_dai_extclk; } /* Some sensible defaults - this reflects the powerup values */ -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
next prev parent reply other threads:[~2015-02-06 19:30 UTC|newest] Thread overview: 186+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-01-23 11:03 [PATCH v13 0/6] Per-user clock constraints Tomeu Vizoso 2015-01-23 11:03 ` [PATCH v13 1/6] clk: Remove unneeded NULL checks Tomeu Vizoso 2015-01-23 11:03 ` [PATCH v13 2/6] clk: Remove __clk_register Tomeu Vizoso 2015-01-23 11:03 ` [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances Tomeu Vizoso 2015-01-23 11:03 ` Tomeu Vizoso 2015-02-01 21:24 ` Mike Turquette 2015-02-01 21:24 ` Mike Turquette 2015-02-01 21:24 ` Mike Turquette 2015-02-02 17:04 ` Tony Lindgren 2015-02-02 17:04 ` Tony Lindgren 2015-02-02 17:32 ` Mike Turquette 2015-02-02 17:32 ` Mike Turquette 2015-02-02 19:32 ` Tero Kristo 2015-02-02 19:32 ` Tero Kristo 2015-02-02 19:32 ` Tero Kristo 2015-02-02 20:44 ` Tony Lindgren 2015-02-02 20:44 ` Tony Lindgren 2015-02-02 22:48 ` Mike Turquette 2015-02-02 22:48 ` Mike Turquette 2015-02-02 23:11 ` Tony Lindgren 2015-02-02 23:11 ` Tony Lindgren 2015-02-02 22:41 ` Mike Turquette 2015-02-02 22:41 ` Mike Turquette 2015-02-02 22:52 ` Stephen Boyd 2015-02-02 22:52 ` Stephen Boyd 2015-02-03 7:03 ` Tomeu Vizoso 2015-02-03 7:03 ` Tomeu Vizoso 2015-02-03 8:46 ` Tero Kristo 2015-02-03 8:46 ` Tero Kristo 2015-02-03 8:46 ` Tero Kristo 2015-02-03 15:22 ` Tony Lindgren 2015-02-03 15:22 ` Tony Lindgren 2015-02-02 20:45 ` Stephen Boyd 2015-02-02 20:45 ` Stephen Boyd 2015-02-02 20:45 ` [Cocci] " Stephen Boyd 2015-02-02 21:31 ` Julia Lawall 2015-02-02 21:31 ` Julia Lawall 2015-02-02 21:31 ` [Cocci] " Julia Lawall 2015-02-02 22:35 ` Stephen Boyd 2015-02-02 22:35 ` Stephen Boyd 2015-02-02 22:35 ` [Cocci] " Stephen Boyd 2015-02-02 22:50 ` Mike Turquette 2015-02-02 22:50 ` Mike Turquette 2015-02-02 22:50 ` [Cocci] " Mike Turquette 2015-02-03 16:04 ` Quentin Lambert 2015-02-03 16:04 ` Quentin Lambert 2015-02-03 16:04 ` Quentin Lambert 2015-02-04 23:26 ` Stephen Boyd 2015-02-04 23:26 ` Stephen Boyd 2015-02-04 23:26 ` Stephen Boyd 2015-02-05 15:45 ` Quentin Lambert 2015-02-05 15:45 ` Quentin Lambert 2015-02-05 15:45 ` Quentin Lambert 2015-02-05 16:02 ` Quentin Lambert 2015-02-05 16:02 ` Quentin Lambert 2015-02-05 16:02 ` Quentin Lambert 2015-02-06 1:49 ` Stephen Boyd 2015-02-06 1:49 ` Stephen Boyd 2015-02-06 1:49 ` Stephen Boyd 2015-02-06 2:15 ` Stephen Boyd 2015-02-06 2:15 ` Stephen Boyd 2015-02-06 2:15 ` Stephen Boyd 2015-02-06 9:01 ` Quentin Lambert 2015-02-06 9:01 ` Quentin Lambert 2015-02-06 9:01 ` Quentin Lambert 2015-02-06 9:12 ` Julia Lawall 2015-02-06 9:12 ` Julia Lawall 2015-02-06 9:12 ` Julia Lawall 2015-02-06 17:15 ` Stephen Boyd 2015-02-06 17:15 ` Stephen Boyd 2015-02-06 17:15 ` Stephen Boyd 2015-02-17 22:01 ` Stephen Boyd 2015-02-17 22:01 ` Stephen Boyd 2015-02-17 22:01 ` Stephen Boyd 2015-03-12 17:20 ` Sebastian Andrzej Siewior 2015-03-12 17:20 ` Sebastian Andrzej Siewior 2015-03-12 17:20 ` Sebastian Andrzej Siewior 2015-03-12 19:43 ` Stephen Boyd 2015-03-12 19:43 ` Stephen Boyd 2015-03-12 19:43 ` Stephen Boyd 2015-03-13 3:29 ` Shawn Guo 2015-03-13 3:29 ` Shawn Guo 2015-03-13 3:29 ` Shawn Guo 2015-03-13 8:20 ` Sebastian Andrzej Siewior 2015-03-13 8:20 ` Sebastian Andrzej Siewior 2015-03-13 8:20 ` Sebastian Andrzej Siewior 2015-03-13 13:42 ` Shawn Guo 2015-03-13 13:42 ` Shawn Guo 2015-03-13 13:42 ` Shawn Guo 2015-03-13 17:42 ` Stephen Boyd 2015-03-13 17:42 ` Stephen Boyd 2015-03-13 17:42 ` Stephen Boyd 2015-02-05 19:44 ` Sylwester Nawrocki 2015-02-05 19:44 ` Sylwester Nawrocki 2015-02-05 20:06 ` Sylwester Nawrocki 2015-02-05 20:06 ` Sylwester Nawrocki 2015-02-05 20:07 ` Stephen Boyd 2015-02-05 20:07 ` Stephen Boyd 2015-02-05 22:14 ` Stephen Boyd 2015-02-05 22:14 ` Stephen Boyd 2015-02-06 0:42 ` Russell King - ARM Linux 2015-02-06 0:42 ` Russell King - ARM Linux 2015-02-06 1:35 ` Stephen Boyd 2015-02-06 1:35 ` Stephen Boyd 2015-02-06 13:39 ` Russell King - ARM Linux 2015-02-06 13:39 ` Russell King - ARM Linux 2015-02-06 19:30 ` Stephen Boyd [this message] 2015-02-06 19:30 ` Stephen Boyd 2015-02-06 19:37 ` Russell King - ARM Linux 2015-02-06 19:37 ` Russell King - ARM Linux 2015-02-06 19:41 ` Stephen Boyd 2015-02-06 19:41 ` Stephen Boyd 2015-02-19 21:32 ` Mike Turquette 2015-02-19 21:32 ` Mike Turquette 2015-02-24 14:08 ` Russell King - ARM Linux 2015-02-24 14:08 ` Russell King - ARM Linux 2015-02-25 2:18 ` Mike Turquette 2015-02-25 2:18 ` Mike Turquette 2015-01-23 11:03 ` [PATCH v13 4/6] clk: Add rate constraints to clocks Tomeu Vizoso 2015-01-23 11:03 ` Tomeu Vizoso 2015-01-23 11:03 ` Tomeu Vizoso 2015-01-29 13:31 ` Geert Uytterhoeven 2015-01-29 13:31 ` Geert Uytterhoeven 2015-01-29 13:31 ` Geert Uytterhoeven 2015-01-29 13:31 ` Geert Uytterhoeven 2015-01-29 19:13 ` Stephen Boyd 2015-01-29 19:13 ` Stephen Boyd 2015-01-29 19:13 ` Stephen Boyd 2015-01-29 19:13 ` Stephen Boyd 2015-01-31 1:31 ` Stephen Boyd 2015-01-31 1:31 ` Stephen Boyd 2015-01-31 1:31 ` Stephen Boyd 2015-01-31 1:31 ` Stephen Boyd 2015-01-31 1:31 ` Stephen Boyd 2015-01-31 18:36 ` Tomeu Vizoso 2015-01-31 18:36 ` Tomeu Vizoso 2015-01-31 18:36 ` Tomeu Vizoso 2015-01-31 18:36 ` Tomeu Vizoso 2015-01-31 18:36 ` Tomeu Vizoso 2015-02-01 22:18 ` Mike Turquette 2015-02-01 22:18 ` Mike Turquette 2015-02-01 22:18 ` Mike Turquette 2015-02-01 22:18 ` Mike Turquette 2015-02-01 22:18 ` Mike Turquette 2015-02-02 7:59 ` Geert Uytterhoeven 2015-02-02 7:59 ` Geert Uytterhoeven 2015-02-02 7:59 ` Geert Uytterhoeven 2015-02-02 7:59 ` Geert Uytterhoeven 2015-02-02 7:59 ` Geert Uytterhoeven 2015-02-02 16:12 ` Tony Lindgren 2015-02-02 16:12 ` Tony Lindgren 2015-02-02 16:12 ` Tony Lindgren 2015-02-02 16:12 ` Tony Lindgren 2015-02-02 16:12 ` Tony Lindgren 2015-02-02 17:46 ` Mike Turquette 2015-02-02 17:46 ` Mike Turquette 2015-02-02 17:46 ` Mike Turquette 2015-02-02 17:46 ` Mike Turquette 2015-02-02 17:46 ` Mike Turquette 2015-02-02 17:49 ` Russell King - ARM Linux 2015-02-02 17:49 ` Russell King - ARM Linux 2015-02-02 17:49 ` Russell King - ARM Linux 2015-02-02 17:49 ` Russell King - ARM Linux 2015-02-02 17:49 ` Russell King - ARM Linux 2015-02-02 19:21 ` Tony Lindgren 2015-02-02 19:21 ` Tony Lindgren 2015-02-02 19:21 ` Tony Lindgren 2015-02-02 19:21 ` Tony Lindgren 2015-02-02 19:21 ` Tony Lindgren 2015-02-02 20:47 ` Tony Lindgren 2015-02-02 20:47 ` Tony Lindgren 2015-02-02 20:47 ` Tony Lindgren 2015-02-02 20:47 ` Tony Lindgren 2015-02-02 20:47 ` Tony Lindgren 2015-01-23 11:03 ` [PATCH v13 5/6] clkdev: Export clk_register_clkdev Tomeu Vizoso 2015-01-23 11:03 ` Tomeu Vizoso 2015-02-03 17:35 ` Andy Shevchenko 2015-02-03 17:35 ` Andy Shevchenko 2015-02-03 17:43 ` Andy Shevchenko 2015-02-03 17:43 ` Andy Shevchenko 2015-01-23 11:03 ` [PATCH v13 6/6] clk: Add module for unit tests Tomeu Vizoso 2015-01-27 0:55 ` [PATCH v13 0/6] Per-user clock constraints Stephen Boyd 2015-01-27 6:29 ` Tomeu Vizoso 2015-01-28 6:59 ` Tomeu Vizoso [not found] ` <20150129022633.22722.78592@quantum> 2015-01-29 6:41 ` Tomeu Vizoso 2015-01-29 14:29 ` Mike Turquette
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=54D5164A.30503@codeaurora.org \ --to=sboyd@codeaurora.org \ --cc=javier.martinez@collabora.co.uk \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=mturquette@linaro.org \ --cc=paul@pwsan.com \ --cc=s.nawrocki@samsung.com \ --cc=tomeu.vizoso@collabora.com \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.