[alsa-devel] [PATCH 0/8] Fix struct clk pointer comparing
On the first day back from Chinese new year holiday, I got a regression report from rmk, saying Ethernet stops working on HimmingBoard with v4.0-rc1.
I read through the thread [1] and found a couple of i.MX audio drivers are also affected per Stephen's Coccinelle report. That's why I came up with this series based on Quentin's structclk.cocci, Stephen's result and Mike's input (thanks all).
The first two patches are run-time tested and all others are compile-tested only.
[1] http://thread.gmane.org/gmane.linux.kernel/1872604/focus=1880862
Shawn Guo (8): clk: add helper function clk_is_match() ARM: imx: fix struct clk pointer comparing drm: armada: fix struct clk pointer comparing pwm: atmel-hlcdc: fix struct clk pointer comparing serial: samsung: fix struct clk pointer comparing ASoC: fsl_esai: fix struct clk pointer comparing ASoC: fsl_spdif: fix struct clk pointer comparing ASoC: kirkwood: fix struct clk pointer comparing
arch/arm/mach-imx/mach-imx6q.c | 5 +++-- drivers/clk/clk.c | 6 ++++++ drivers/gpu/drm/armada/armada_510.c | 2 +- drivers/pwm/pwm-atmel-hlcdc.c | 2 +- drivers/tty/serial/samsung.c | 2 +- include/linux/clk.h | 16 ++++++++++++++++ sound/soc/fsl/fsl_esai.c | 2 +- sound/soc/fsl/fsl_spdif.c | 4 ++-- sound/soc/kirkwood/kirkwood-i2s.c | 2 +- 9 files changed, 32 insertions(+), 9 deletions(-)
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
A number of client drivers detecting if two struct clk pointers point to the same one hardware clock by comparing the pointers are broken now. As a stop-gap solution, this patch adds a helper function clk_is_match() to test if two struct clk pointers point to the same hardware clock, so that these client drivers can use to fix the regression.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/clk/clk.c | 6 ++++++ include/linux/clk.h | 16 ++++++++++++++++ 2 files changed, 22 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index eb0152961d3c..7df4826d9c7b 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -597,6 +597,12 @@ struct clk *__clk_get_parent(struct clk *clk) } EXPORT_SYMBOL_GPL(__clk_get_parent);
+bool clk_is_match(struct clk *clk1, struct clk *clk2) +{ + return __clk_get_hw(clk1) == __clk_get_hw(clk2) ? true : false; +} +EXPORT_SYMBOL_GPL(clk_is_match); + static struct clk_core *clk_core_get_parent_by_index(struct clk_core *clk, u8 index) { diff --git a/include/linux/clk.h b/include/linux/clk.h index 8381bbfbc308..f8a8a6dd6a98 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -376,6 +376,17 @@ struct clk *clk_get_parent(struct clk *clk); */ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
+/** + * clk_is_match - test if two given struct clk pointers point to the same + * hardware clock, i.e. struct clk_hw. + * @clk1: the first clk + * @clk2: the second clk + * + * Returns true if two clk pointers point to the same hardware clock, + * or false otherwise. + */ +bool clk_is_match(struct clk *clk1, struct clk *clk2); + #else /* !CONFIG_HAVE_CLK */
static inline struct clk *clk_get(struct device *dev, const char *id) @@ -429,6 +440,11 @@ static inline struct clk *clk_get_parent(struct clk *clk) return NULL; }
+static inline bool clk_is_match(struct clk *clk1, struct clk *clk2) +{ + return clk1 == clk2 ? true : false; +} + #endif
/* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
Quoting Shawn Guo (2015-02-25 06:53:31)
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
A number of client drivers detecting if two struct clk pointers point to the same one hardware clock by comparing the pointers are broken now. As a stop-gap solution, this patch adds a helper function clk_is_match() to test if two struct clk pointers point to the same hardware clock, so that these client drivers can use to fix the regression.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Hi Shawn,
Thanks for the patch. I wrote a similar one last night but did not finish fixing up the drivers (and thus did not post it). I prefer my implementation below, and I'm happy to merge your driver fixes with it.
Regards, Mike
From: Michael Turquette mturquette@linaro.org Date: Wed, 25 Feb 2015 09:11:01 -0800 Subject: [PATCH] clk: introduce clk_is_match
Some drivers compare struct clk pointers as a means of knowing if the two pointers reference the same clock hardware. This behavior is dubious (drivers must not dereference struct clk), but did not cause any regressions until the per-user struct clk patch was merged. Now the test for matching clk's will always fail with per-user struct clk's.
clk_is_match is introduced to fix the regression and prevent drivers from comparing the pointers manually.
Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances") Cc: Russell King linux@arm.linux.org.uk Cc: Stephen Boyd sboyd@codeaurora.org Cc: Shawn Guo shawn.guo@linaro.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Signed-off-by: Michael Turquette mturquette@linaro.org --- drivers/clk/clk.c | 26 ++++++++++++++++++++++++++ include/linux/clk.h | 18 ++++++++++++++++++ 2 files changed, 44 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index eb01529..09670de 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2171,6 +2171,32 @@ int clk_get_phase(struct clk *clk) }
/** + * clk_is_match - check if two clk's point to the same hardware clock + * @p: clk compared against q + * @q: clk compared against p + * + * Returns true if the two struct clk pointers both point to the same hardware + * clock node. Put differently, returns true if struct clk *p and struct clk *q + * share the same struct clk_core object. + * + * Returns false otherwise. Note that two NULL clks are treated as matching. + */ +bool clk_is_match(struct clk *p, struct clk *q) +{ + /* trivial case: identical struct clk's or both NULL */ + if (p == q) + return true; + + /* true if clk->core pointers match. Avoid derefing garbage */ + if (!IS_ERR_OR_NULL(p) && !IS_ERR_OR_NULL(q)) + if (p->core == q->core) + return true; + + return false; +} +EXPORT_SYMBOL_GPL(clk_is_match); + +/** * __clk_init - initialize the data structures in a struct clk * @dev: device initializing this clk, placeholder for now * @clk: clk being initialized diff --git a/include/linux/clk.h b/include/linux/clk.h index 8381bbf..5c076e4 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -376,6 +376,19 @@ struct clk *clk_get_parent(struct clk *clk); */ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
+/** + * clk_is_match - check if two clk's point to the same hardware clock + * @p: clk compared against q + * @q: clk compared against p + * + * Returns true if the two struct clk pointers both point to the same hardware + * clock node. Put differently, returns true if struct clk *p and struct clk *q + * share the same struct clk_core object. + * + * Returns false otherwise. Note that two NULL clks are treated as matching. + */ +bool clk_is_match(struct clk *p, struct clk *q); + #else /* !CONFIG_HAVE_CLK */
static inline struct clk *clk_get(struct device *dev, const char *id) @@ -429,6 +442,11 @@ static inline struct clk *clk_get_parent(struct clk *clk) return NULL; }
+static inline bool clk_is_match(struct clk *p, struct clk *q) +{ + return p == q ? true : false; +} + #endif
/* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
On Wed, Feb 25, 2015 at 09:27:57AM -0800, Mike Turquette wrote:
Hi Shawn,
Thanks for the patch. I wrote a similar one last night but did not finish fixing up the drivers (and thus did not post it). I prefer my implementation below, and I'm happy to merge your driver fixes with it.
Sure, no problem. My intention was to get rmk's HummingBoard back to work ASAP :)
From: Michael Turquette mturquette@linaro.org Date: Wed, 25 Feb 2015 09:11:01 -0800 Subject: [PATCH] clk: introduce clk_is_match
Some drivers compare struct clk pointers as a means of knowing if the two pointers reference the same clock hardware. This behavior is dubious (drivers must not dereference struct clk), but did not cause any regressions until the per-user struct clk patch was merged. Now the test for matching clk's will always fail with per-user struct clk's.
clk_is_match is introduced to fix the regression and prevent drivers from comparing the pointers manually.
Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances") Cc: Russell King linux@arm.linux.org.uk Cc: Stephen Boyd sboyd@codeaurora.org Cc: Shawn Guo shawn.guo@linaro.org
Tested-by: Shawn Guo shawn.guo@linaro.org
Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Signed-off-by: Michael Turquette mturquette@linaro.org
On Wed, Feb 25, 2015 at 09:27:57AM -0800, Mike Turquette wrote:
Quoting Shawn Guo (2015-02-25 06:53:31)
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
A number of client drivers detecting if two struct clk pointers point to the same one hardware clock by comparing the pointers are broken now. As a stop-gap solution, this patch adds a helper function clk_is_match() to test if two struct clk pointers point to the same hardware clock, so that these client drivers can use to fix the regression.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Hi Shawn,
Thanks for the patch. I wrote a similar one last night but did not finish fixing up the drivers (and thus did not post it). I prefer my implementation below, and I'm happy to merge your driver fixes with it.
Regards, Mike
From: Michael Turquette mturquette@linaro.org Date: Wed, 25 Feb 2015 09:11:01 -0800 Subject: [PATCH] clk: introduce clk_is_match
Some drivers compare struct clk pointers as a means of knowing if the two pointers reference the same clock hardware. This behavior is dubious (drivers must not dereference struct clk), but did not cause any regressions until the per-user struct clk patch was merged. Now the test for matching clk's will always fail with per-user struct clk's.
clk_is_match is introduced to fix the regression and prevent drivers from comparing the pointers manually.
small observaton, clk_is_same() is linguistically nicer.
Am Donnerstag, den 26.02.2015, 09:02 +0000 schrieb Ben Dooks:
On Wed, Feb 25, 2015 at 09:27:57AM -0800, Mike Turquette wrote:
[...]
From: Michael Turquette mturquette@linaro.org Date: Wed, 25 Feb 2015 09:11:01 -0800 Subject: [PATCH] clk: introduce clk_is_match
Some drivers compare struct clk pointers as a means of knowing if the two pointers reference the same clock hardware. This behavior is dubious (drivers must not dereference struct clk), but did not cause any regressions until the per-user struct clk patch was merged. Now the test for matching clk's will always fail with per-user struct clk's.
clk_is_match is introduced to fix the regression and prevent drivers from comparing the pointers manually.
small observaton, clk_is_same() is linguistically nicer.
How about clk_equal() ?
regards Philipp
On Thu, Feb 26, 2015 at 10:56:58AM +0100, Philipp Zabel wrote:
Am Donnerstag, den 26.02.2015, 09:02 +0000 schrieb Ben Dooks:
On Wed, Feb 25, 2015 at 09:27:57AM -0800, Mike Turquette wrote:
[...]
From: Michael Turquette mturquette@linaro.org Date: Wed, 25 Feb 2015 09:11:01 -0800 Subject: [PATCH] clk: introduce clk_is_match
Some drivers compare struct clk pointers as a means of knowing if the two pointers reference the same clock hardware. This behavior is dubious (drivers must not dereference struct clk), but did not cause any regressions until the per-user struct clk patch was merged. Now the test for matching clk's will always fail with per-user struct clk's.
clk_is_match is introduced to fix the regression and prevent drivers from comparing the pointers manually.
small observaton, clk_is_same() is linguistically nicer.
How about clk_equal() ?
That's good, the only issue that's not clear in any of these names is that does this mean "the same clock", a "clock of the same rate" or a "clock that is equivalent to in the rate and phase but not subject to the same gate".
On Thu, 26 Feb 2015, Ben Dooks wrote:
small observaton, clk_is_same() is linguistically nicer.
How about clk_equal() ?
That's good, the only issue that's not clear in any of these names is that does this mean "the same clock", a "clock of the same rate" or a "clock that is equivalent to in the rate and phase but not subject to the same gate".
clk_identical() ?
/Ricard
On Wed, Feb 25, 2015 at 6:27 PM, Mike Turquette mturquette@linaro.org wrote:
From: Michael Turquette mturquette@linaro.org Date: Wed, 25 Feb 2015 09:11:01 -0800 Subject: [PATCH] clk: introduce clk_is_match
Some drivers compare struct clk pointers as a means of knowing if the two pointers reference the same clock hardware. This behavior is dubious (drivers must not dereference struct clk), but did not cause any regressions until the per-user struct clk patch was merged. Now the test for matching clk's will always fail with per-user struct clk's.
clk_is_match is introduced to fix the regression and prevent drivers from comparing the pointers manually.
Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances") Cc: Russell King linux@arm.linux.org.uk Cc: Stephen Boyd sboyd@codeaurora.org Cc: Shawn Guo shawn.guo@linaro.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Signed-off-by: Michael Turquette mturquette@linaro.org
drivers/clk/clk.c | 26 ++++++++++++++++++++++++++ include/linux/clk.h | 18 ++++++++++++++++++ 2 files changed, 44 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index eb01529..09670de 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2171,6 +2171,32 @@ int clk_get_phase(struct clk *clk) }
/**
- clk_is_match - check if two clk's point to the same hardware clock
- @p: clk compared against q
- @q: clk compared against p
- Returns true if the two struct clk pointers both point to the same hardware
- clock node. Put differently, returns true if struct clk *p and struct clk *q
- share the same struct clk_core object.
- Returns false otherwise. Note that two NULL clks are treated as matching.
- */
+bool clk_is_match(struct clk *p, struct clk *q)
const struct clk *p, const struct clk *q
--- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -376,6 +376,19 @@ struct clk *clk_get_parent(struct clk *clk); */ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
+/**
- clk_is_match - check if two clk's point to the same hardware clock
- @p: clk compared against q
- @q: clk compared against p
- Returns true if the two struct clk pointers both point to the same hardware
- clock node. Put differently, returns true if struct clk *p and struct clk *q
- share the same struct clk_core object.
- Returns false otherwise. Note that two NULL clks are treated as matching.
- */
+bool clk_is_match(struct clk *p, struct clk *q);
const struct clk *p, const struct clk *q
#else /* !CONFIG_HAVE_CLK */
static inline struct clk *clk_get(struct device *dev, const char *id) @@ -429,6 +442,11 @@ static inline struct clk *clk_get_parent(struct clk *clk) return NULL; }
+static inline bool clk_is_match(struct clk *p, struct clk *q)
const struct clk *p, const struct clk *q
+{
return p == q ? true : false;
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
On Wed, Feb 25, 2015 at 09:27:57AM -0800, Mike Turquette wrote:
+static inline bool clk_is_match(struct clk *p, struct clk *q) +{
- return p == q ? true : false;
When they created the C standard, they already taught the compiler that p == q returns a true / false boolean value; there's no need for ?: here.
ARM randconfig build tests found a new error for configurations with COMMON_CLK disabled but HAS_CLK selected by the platform:
ERROR: "clk_is_match" [sound/soc/fsl/snd-soc-fsl-spdif.ko] undefined!
This moves the declaration around, so this case is covered by the existing static inline helper function.
Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: c69e182e51d89 ("clk: introduce clk_is_match") ---- BTW, we have a preexisting problem in clk_get_parent, clk_round_rate and clk_set_parent, which I've worked around in my randconfig builds so far. Should we do that the same way?
diff --git a/include/linux/clk.h b/include/linux/clk.h index 5c076e4d90f9..a9b91595d106 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -125,6 +125,19 @@ int clk_set_phase(struct clk *clk, int degrees); */ int clk_get_phase(struct clk *clk);
+/** + * clk_is_match - check if two clk's point to the same hardware clock + * @p: clk compared against q + * @q: clk compared against p + * + * Returns true if the two struct clk pointers both point to the same hardware + * clock node. Put differently, returns true if struct clk *p and struct clk *q + * share the same struct clk_core object. + * + * Returns false otherwise. Note that two NULL clks are treated as matching. + */ +bool clk_is_match(struct clk *p, struct clk *q); + #else
static inline long clk_get_accuracy(struct clk *clk) @@ -142,6 +155,11 @@ static inline long clk_get_phase(struct clk *clk) return -ENOTSUPP; }
+static inline bool clk_is_match(struct clk *p, struct clk *q) +{ + return p == q ? true : false; +} + #endif
/** @@ -376,19 +394,6 @@ struct clk *clk_get_parent(struct clk *clk); */ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
-/** - * clk_is_match - check if two clk's point to the same hardware clock - * @p: clk compared against q - * @q: clk compared against p - * - * Returns true if the two struct clk pointers both point to the same hardware - * clock node. Put differently, returns true if struct clk *p and struct clk *q - * share the same struct clk_core object. - * - * Returns false otherwise. Note that two NULL clks are treated as matching. - */ -bool clk_is_match(struct clk *p, struct clk *q); - #else /* !CONFIG_HAVE_CLK */
static inline struct clk *clk_get(struct device *dev, const char *id) @@ -442,11 +447,6 @@ static inline struct clk *clk_get_parent(struct clk *clk) return NULL; }
-static inline bool clk_is_match(struct clk *p, struct clk *q) -{ - return p == q ? true : false; -} - #endif
/* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
On Sun, Mar 8, 2015 at 10:05 PM, Arnd Bergmann arnd@arndb.de wrote:
ARM randconfig build tests found a new error for configurations with COMMON_CLK disabled but HAS_CLK selected by the platform:
ERROR: "clk_is_match" [sound/soc/fsl/snd-soc-fsl-spdif.ko] undefined!
This moves the declaration around, so this case is covered by the existing static inline helper function.
Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: c69e182e51d89 ("clk: introduce clk_is_match")
FYI, this also happens for sh/allmodconfig http://kisskb.ellerman.id.au/kisskb/buildresult/12379884/
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
Hello,
On Sun, Mar 08, 2015 at 10:05:29PM +0100, Arnd Bergmann wrote:
+static inline bool clk_is_match(struct clk *p, struct clk *q) +{
- return p == q ? true : false;
OK, this is only a move, but I wonder why Russell's comment that this is equivalent to the easier
return p == q;
wasn't addressed?!
Best regards Uwe
On Sun, Mar 08, 2015 at 10:05:29PM +0100, Arnd Bergmann wrote:
ARM randconfig build tests found a new error for configurations with COMMON_CLK disabled but HAS_CLK selected by the platform:
ERROR: "clk_is_match" [sound/soc/fsl/snd-soc-fsl-spdif.ko] undefined!
This moves the declaration around, so this case is covered by the existing static inline helper function.
Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: c69e182e51d89 ("clk: introduce clk_is_match")
BTW, we have a preexisting problem in clk_get_parent, clk_round_rate and clk_set_parent, which I've worked around in my randconfig builds so far. Should we do that the same way?
NAK, as Uwe points out, you didn't address my comment.
Hey Russell,
On Wed, Mar 11, 2015 at 10:22:09AM +0000, Russell King - ARM Linux wrote:
On Sun, Mar 08, 2015 at 10:05:29PM +0100, Arnd Bergmann wrote:
ARM randconfig build tests found a new error for configurations with COMMON_CLK disabled but HAS_CLK selected by the platform:
ERROR: "clk_is_match" [sound/soc/fsl/snd-soc-fsl-spdif.ko] undefined!
This moves the declaration around, so this case is covered by the existing static inline helper function.
Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: c69e182e51d89 ("clk: introduce clk_is_match")
BTW, we have a preexisting problem in clk_get_parent, clk_round_rate and clk_set_parent, which I've worked around in my randconfig builds so far. Should we do that the same way?
NAK, as Uwe points out, you didn't address my comment.
You commented on the patch that is c69e182e51d8 ("clk: introduce clk_is_match") now in next. Arnd just moved this around.
Best regards Uwe
On Wed, Mar 11, 2015 at 12:17:55PM +0100, Uwe Kleine-König wrote:
Hey Russell,
On Wed, Mar 11, 2015 at 10:22:09AM +0000, Russell King - ARM Linux wrote:
On Sun, Mar 08, 2015 at 10:05:29PM +0100, Arnd Bergmann wrote:
ARM randconfig build tests found a new error for configurations with COMMON_CLK disabled but HAS_CLK selected by the platform:
ERROR: "clk_is_match" [sound/soc/fsl/snd-soc-fsl-spdif.ko] undefined!
This moves the declaration around, so this case is covered by the existing static inline helper function.
Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: c69e182e51d89 ("clk: introduce clk_is_match")
BTW, we have a preexisting problem in clk_get_parent, clk_round_rate and clk_set_parent, which I've worked around in my randconfig builds so far. Should we do that the same way?
NAK, as Uwe points out, you didn't address my comment.
You commented on the patch that is c69e182e51d8 ("clk: introduce clk_is_match") now in next. Arnd just moved this around.
*Sigh*
Mike - please remove this commit until proper kernel patch process is honoured. We'll have some order here, and mutual respect of fellow kernel developers, rather than people selectively ignoring people. Yes, I realise that it fixes a bug, but it's utterly disgusting that comments on a patch are ignored and it's just picked up irrespective of comments being addressed.
If you don't like that, bloody well do a better job.
On 03/11/15 05:29, Russell King - ARM Linux wrote:
On Wed, Mar 11, 2015 at 12:17:55PM +0100, Uwe Kleine-König wrote:
Hey Russell,
On Wed, Mar 11, 2015 at 10:22:09AM +0000, Russell King - ARM Linux wrote:
On Sun, Mar 08, 2015 at 10:05:29PM +0100, Arnd Bergmann wrote:
ARM randconfig build tests found a new error for configurations with COMMON_CLK disabled but HAS_CLK selected by the platform:
ERROR: "clk_is_match" [sound/soc/fsl/snd-soc-fsl-spdif.ko] undefined!
This moves the declaration around, so this case is covered by the existing static inline helper function.
Signed-off-by: Arnd Bergmann arnd@arndb.de Fixes: c69e182e51d89 ("clk: introduce clk_is_match")
BTW, we have a preexisting problem in clk_get_parent, clk_round_rate and clk_set_parent, which I've worked around in my randconfig builds so far. Should we do that the same way?
NAK, as Uwe points out, you didn't address my comment.
You commented on the patch that is c69e182e51d8 ("clk: introduce clk_is_match") now in next. Arnd just moved this around.
*Sigh*
Mike - please remove this commit until proper kernel patch process is honoured. We'll have some order here, and mutual respect of fellow kernel developers, rather than people selectively ignoring people. Yes, I realise that it fixes a bug, but it's utterly disgusting that comments on a patch are ignored and it's just picked up irrespective of comments being addressed.
If you don't like that, bloody well do a better job.
The patch was *not* picked up after your mail was sent. The patch was picked up the same day it was posted to the list (Feb 25). You sent review comments a week later (March 4). I don't see any selective ignoring here.
I'll fold the const comments from Geert, your comments, and Arnd's fix into the patch. Here's the new patch:
----8<----
From: Michael Turquette mturquette@linaro.org Subject: [PATCH] clk: introduce clk_is_match
Some drivers compare struct clk pointers as a means of knowing if the two pointers reference the same clock hardware. This behavior is dubious (drivers must not dereference struct clk), but did not cause any regressions until the per-user struct clk patch was merged. Now the test for matching clk's will always fail with per-user struct clk's.
clk_is_match is introduced to fix the regression and prevent drivers from comparing the pointers manually.
Fixes: 035a61c314eb ("clk: Make clk API return per-user struct clk instances") Cc: Russell King linux@arm.linux.org.uk Cc: Shawn Guo shawn.guo@linaro.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Signed-off-by: Michael Turquette mturquette@linaro.org [arnd@arndb.de: Fix COMMON_CLK=N && HAS_CLK=Y config] Signed-off-by: Arnd Bergmann arnd@arndb.de [sboyd@codeaurora.org: const arguments to clk_is_match() and remove unnecessary ternary operation] Signed-off-by: Stephen Boyd sboyd@codeaurora.org --- drivers/clk/clk.c | 26 ++++++++++++++++++++++++++ include/linux/clk.h | 18 ++++++++++++++++++ 2 files changed, 44 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index b9f85fc2ce3f..237f23f68bfc 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2170,6 +2170,32 @@ int clk_get_phase(struct clk *clk) }
/** + * clk_is_match - check if two clk's point to the same hardware clock + * @p: clk compared against q + * @q: clk compared against p + * + * Returns true if the two struct clk pointers both point to the same hardware + * clock node. Put differently, returns true if struct clk *p and struct clk *q + * share the same struct clk_core object. + * + * Returns false otherwise. Note that two NULL clks are treated as matching. + */ +bool clk_is_match(const struct clk *p, const struct clk *q) +{ + /* trivial case: identical struct clk's or both NULL */ + if (p == q) + return true; + + /* true if clk->core pointers match. Avoid derefing garbage */ + if (!IS_ERR_OR_NULL(p) && !IS_ERR_OR_NULL(q)) + if (p->core == q->core) + return true; + + return false; +} +EXPORT_SYMBOL_GPL(clk_is_match); + +/** * __clk_init - initialize the data structures in a struct clk * @dev: device initializing this clk, placeholder for now * @clk: clk being initialized diff --git a/include/linux/clk.h b/include/linux/clk.h index 8381bbfbc308..68c16a6bedb3 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -125,6 +125,19 @@ int clk_set_phase(struct clk *clk, int degrees); */ int clk_get_phase(struct clk *clk);
+/** + * clk_is_match - check if two clk's point to the same hardware clock + * @p: clk compared against q + * @q: clk compared against p + * + * Returns true if the two struct clk pointers both point to the same hardware + * clock node. Put differently, returns true if struct clk *p and struct clk *q + * share the same struct clk_core object. + * + * Returns false otherwise. Note that two NULL clks are treated as matching. + */ +bool clk_is_match(const struct clk *p, const struct clk *q); + #else
static inline long clk_get_accuracy(struct clk *clk) @@ -142,6 +155,11 @@ static inline long clk_get_phase(struct clk *clk) return -ENOTSUPP; }
+static inline bool clk_is_match(const struct clk *p, const struct clk *q) +{ + return p == q; +} + #endif
/**
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- arch/arm/mach-imx/mach-imx6q.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index 4ad6e473cf83..9de3412af406 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -211,8 +211,9 @@ static void __init imx6q_1588_init(void) * set bit IOMUXC_GPR1[21]. Or the PTP clock must be from pad * (external OSC), and we need to clear the bit. */ - clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP : - IMX6Q_GPR1_ENET_CLK_SEL_PAD; + clksel = clk_is_match(ptp_clk, enet_ref) ? + IMX6Q_GPR1_ENET_CLK_SEL_ANATOP : + IMX6Q_GPR1_ENET_CLK_SEL_PAD; gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr"); if (!IS_ERR(gpr)) regmap_update_bits(gpr, IOMUXC_GPR1,
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/gpu/drm/armada/armada_510.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/armada/armada_510.c b/drivers/gpu/drm/armada/armada_510.c index ad3d2ebf95c9..862deafe8b24 100644 --- a/drivers/gpu/drm/armada/armada_510.c +++ b/drivers/gpu/drm/armada/armada_510.c @@ -53,7 +53,7 @@ static int armada510_crtc_compute_clock(struct armada_crtc *dcrtc, if (IS_ERR(clk)) return PTR_ERR(clk);
- if (dcrtc->clk != clk) { + if (!clk_is_match(dcrtc->clk, clk)) { ret = clk_prepare_enable(clk); if (ret) return ret;
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/pwm/pwm-atmel-hlcdc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c index 522f7075bb1a..36475949b829 100644 --- a/drivers/pwm/pwm-atmel-hlcdc.c +++ b/drivers/pwm/pwm-atmel-hlcdc.c @@ -97,7 +97,7 @@ static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
pwmcfg = ATMEL_HLCDC_PWMPS(pres);
- if (new_clk != chip->cur_clk) { + if (!clk_is_match(new_clk, chip->cur_clk)) { u32 gencfg = 0; int ret;
Le 25/02/2015 15:53, Shawn Guo a écrit :
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
I agree with the fix whichever name is chosen for the function in an future version of this series. So you can add my: Acked-by: Nicolas Ferre nicolas.ferre@atmel.com
Maybe Boris can double check...
drivers/pwm/pwm-atmel-hlcdc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c index 522f7075bb1a..36475949b829 100644 --- a/drivers/pwm/pwm-atmel-hlcdc.c +++ b/drivers/pwm/pwm-atmel-hlcdc.c @@ -97,7 +97,7 @@ static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
pwmcfg = ATMEL_HLCDC_PWMPS(pres);
- if (new_clk != chip->cur_clk) {
- if (!clk_is_match(new_clk, chip->cur_clk)) { u32 gencfg = 0; int ret;
Nicolas, Shawn,
On Thu, 26 Feb 2015 10:22:50 +0100 Nicolas Ferre nicolas.ferre@atmel.com wrote:
Le 25/02/2015 15:53, Shawn Guo a écrit :
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
I agree with the fix whichever name is chosen for the function in an future version of this series. So you can add my: Acked-by: Nicolas Ferre nicolas.ferre@atmel.com
Maybe Boris can double check...
Looks good to me. Thanks for fixing that.
Acked-by: Boris Brezillon boris.brezillon@free-electrons.com
drivers/pwm/pwm-atmel-hlcdc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c index 522f7075bb1a..36475949b829 100644 --- a/drivers/pwm/pwm-atmel-hlcdc.c +++ b/drivers/pwm/pwm-atmel-hlcdc.c @@ -97,7 +97,7 @@ static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
pwmcfg = ATMEL_HLCDC_PWMPS(pres);
- if (new_clk != chip->cur_clk) {
- if (!clk_is_match(new_clk, chip->cur_clk)) { u32 gencfg = 0; int ret;
On Wed, Feb 25, 2015 at 10:53:34PM +0800, Shawn Guo wrote:
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
drivers/pwm/pwm-atmel-hlcdc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Thierry Reding thierry.reding@gmail.com
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/tty/serial/samsung.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c index af821a908720..b747066247df 100644 --- a/drivers/tty/serial/samsung.c +++ b/drivers/tty/serial/samsung.c @@ -1277,7 +1277,7 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
/* check to see if we need to change clock source */
- if (ourport->baudclk != clk) { + if (!clk_is_match(ourport->baudclk, clk)) { s3c24xx_serial_setsource(port, clk_sel);
if (!IS_ERR(ourport->baudclk)) {
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/fsl_esai.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index 5c7597191e3f..6e00e51b98c2 100644 --- a/sound/soc/fsl/fsl_esai.c +++ b/sound/soc/fsl/fsl_esai.c @@ -269,7 +269,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id, }
/* Only EXTAL source can be output directly without using PSR and PM */ - if (ratio == 1 && clksrc == esai_priv->extalclk) { + if (ratio == 1 && clk_is_match(clksrc, esai_priv->extalclk)) { /* Bypass all the dividers if not being needed */ ecr |= tx ? ESAI_ECR_ETO : ESAI_ECR_ERO; goto out;
On Wed, Feb 25, 2015 at 10:53:36PM +0800, Shawn Guo wrote:
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by
Acked-by: Mark Brown broonie@kernel.org
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/fsl_spdif.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c index 75870c0ea2c9..91eb3aef7f02 100644 --- a/sound/soc/fsl/fsl_spdif.c +++ b/sound/soc/fsl/fsl_spdif.c @@ -1049,7 +1049,7 @@ static u32 fsl_spdif_txclk_caldiv(struct fsl_spdif_priv *spdif_priv, enum spdif_txrate index, bool round) { const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 }; - bool is_sysclk = clk == spdif_priv->sysclk; + bool is_sysclk = clk_is_match(clk, spdif_priv->sysclk); u64 rate_ideal, rate_actual, sub; u32 sysclk_dfmin, sysclk_dfmax; u32 txclk_df, sysclk_df, arate; @@ -1143,7 +1143,7 @@ static int fsl_spdif_probe_txclk(struct fsl_spdif_priv *spdif_priv, spdif_priv->txclk_src[index], rate[index]); dev_dbg(&pdev->dev, "use txclk df %d for %dHz sample rate\n", spdif_priv->txclk_df[index], rate[index]); - if (spdif_priv->txclk[index] == spdif_priv->sysclk) + if (clk_is_match(spdif_priv->txclk[index], spdif_priv->sysclk)) dev_dbg(&pdev->dev, "use sysclk df %d for %dHz sample rate\n", spdif_priv->sysclk_df[index], rate[index]); dev_dbg(&pdev->dev, "the best rate for %dHz sample rate is %dHz\n",
On 02/25/15 06:53, Shawn Guo wrote:
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
sound/soc/fsl/fsl_spdif.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c index 75870c0ea2c9..91eb3aef7f02 100644 --- a/sound/soc/fsl/fsl_spdif.c +++ b/sound/soc/fsl/fsl_spdif.c @@ -1049,7 +1049,7 @@ static u32 fsl_spdif_txclk_caldiv(struct fsl_spdif_priv *spdif_priv, enum spdif_txrate index, bool round) { const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 };
- bool is_sysclk = clk == spdif_priv->sysclk;
- bool is_sysclk = clk_is_match(clk, spdif_priv->sysclk); u64 rate_ideal, rate_actual, sub; u32 sysclk_dfmin, sysclk_dfmax; u32 txclk_df, sysclk_df, arate;
@@ -1143,7 +1143,7 @@ static int fsl_spdif_probe_txclk(struct fsl_spdif_priv *spdif_priv, spdif_priv->txclk_src[index], rate[index]); dev_dbg(&pdev->dev, "use txclk df %d for %dHz sample rate\n", spdif_priv->txclk_df[index], rate[index]);
- if (spdif_priv->txclk[index] == spdif_priv->sysclk)
- if (clk_is_match(spdif_priv->txclk[index], spdif_priv->sysclk)) dev_dbg(&pdev->dev, "use sysclk df %d for %dHz sample rate\n", spdif_priv->sysclk_df[index], rate[index]); dev_dbg(&pdev->dev, "the best rate for %dHz sample rate is %dHz\n",
Couldn't this be fixed like this?
----8<----
diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c index af0429421fc8..1878dd62a247 100644 --- a/sound/soc/fsl/fsl_spdif.c +++ b/sound/soc/fsl/fsl_spdif.c @@ -20,6 +20,7 @@ #include <linux/of_device.h> #include <linux/of_irq.h> #include <linux/regmap.h> +#include <linux/stringify.h>
#include <sound/asoundef.h> #include <sound/dmaengine_pcm.h> @@ -46,6 +47,8 @@ static u8 srpc_dpll_locked[] = { 0x0, 0x1, 0x2, 0x3, 0x4, 0xa, 0xb };
#define DEFAULT_RXCLK_SRC 1
+#define SYSCLK_NUM 5 + /* * SPDIF control structure * Defines channel status, subcode and Q sub @@ -1051,10 +1054,10 @@ static const struct regmap_config fsl_spdif_regmap_config = {
static u32 fsl_spdif_txclk_caldiv(struct fsl_spdif_priv *spdif_priv, struct clk *clk, u64 savesub, - enum spdif_txrate index, bool round) + enum spdif_txrate index, bool round, + bool is_sysclk) { const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 }; - bool is_sysclk = clk == spdif_priv->sysclk; u64 rate_ideal, rate_actual, sub; u32 sysclk_dfmin, sysclk_dfmax; u32 txclk_df, sysclk_df, arate; @@ -1131,7 +1134,8 @@ static int fsl_spdif_probe_txclk(struct fsl_spdif_priv *spdif_priv, continue;
ret = fsl_spdif_txclk_caldiv(spdif_priv, clk, savesub, index, - i == STC_TXCLK_SPDIF_ROOT); + i == STC_TXCLK_SPDIF_ROOT, + i == SYSCLK_NUM); if (savesub == ret) continue;
@@ -1210,7 +1214,8 @@ static int fsl_spdif_probe(struct platform_device *pdev) }
/* Get system clock for rx clock rate calculation */ - spdif_priv->sysclk = devm_clk_get(&pdev->dev, "rxtx5"); + spdif_priv->sysclk = devm_clk_get(&pdev->dev, + "rxtx" __stringify(SYSCLK_NUM)); if (IS_ERR(spdif_priv->sysclk)) { dev_err(&pdev->dev, "no sys clock (rxtx5) in devicetree\n"); return PTR_ERR(spdif_priv->sysclk);
On Wed, Feb 25, 2015 at 01:04:41PM -0800, Stephen Boyd wrote:
diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c index 75870c0ea2c9..91eb3aef7f02 100644 --- a/sound/soc/fsl/fsl_spdif.c +++ b/sound/soc/fsl/fsl_spdif.c @@ -1049,7 +1049,7 @@ static u32 fsl_spdif_txclk_caldiv(struct fsl_spdif_priv *spdif_priv, enum spdif_txrate index, bool round) { const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 };
- bool is_sysclk = clk == spdif_priv->sysclk;
- bool is_sysclk = clk_is_match(clk, spdif_priv->sysclk); u64 rate_ideal, rate_actual, sub; u32 sysclk_dfmin, sysclk_dfmax; u32 txclk_df, sysclk_df, arate;
@@ -1143,7 +1143,7 @@ static int fsl_spdif_probe_txclk(struct fsl_spdif_priv *spdif_priv, spdif_priv->txclk_src[index], rate[index]); dev_dbg(&pdev->dev, "use txclk df %d for %dHz sample rate\n", spdif_priv->txclk_df[index], rate[index]);
- if (spdif_priv->txclk[index] == spdif_priv->sysclk)
- if (clk_is_match(spdif_priv->txclk[index], spdif_priv->sysclk)) dev_dbg(&pdev->dev, "use sysclk df %d for %dHz sample rate\n", spdif_priv->sysclk_df[index], rate[index]); dev_dbg(&pdev->dev, "the best rate for %dHz sample rate is %dHz\n",
Couldn't this be fixed like this?
It could. But this fix is less-intuitive and makes the code change unnecessarily complex, considering we are introducing helper clk_is_match() anyway.
Shawn
----8<----
diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c index af0429421fc8..1878dd62a247 100644 --- a/sound/soc/fsl/fsl_spdif.c +++ b/sound/soc/fsl/fsl_spdif.c @@ -20,6 +20,7 @@ #include <linux/of_device.h> #include <linux/of_irq.h> #include <linux/regmap.h> +#include <linux/stringify.h>
#include <sound/asoundef.h> #include <sound/dmaengine_pcm.h> @@ -46,6 +47,8 @@ static u8 srpc_dpll_locked[] = { 0x0, 0x1, 0x2, 0x3, 0x4, 0xa, 0xb };
#define DEFAULT_RXCLK_SRC 1
+#define SYSCLK_NUM 5
/*
- SPDIF control structure
- Defines channel status, subcode and Q sub
@@ -1051,10 +1054,10 @@ static const struct regmap_config fsl_spdif_regmap_config = {
static u32 fsl_spdif_txclk_caldiv(struct fsl_spdif_priv *spdif_priv, struct clk *clk, u64 savesub,
enum spdif_txrate index, bool round)
enum spdif_txrate index, bool round,bool is_sysclk){ const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 };
- bool is_sysclk = clk == spdif_priv->sysclk; u64 rate_ideal, rate_actual, sub; u32 sysclk_dfmin, sysclk_dfmax; u32 txclk_df, sysclk_df, arate;
@@ -1131,7 +1134,8 @@ static int fsl_spdif_probe_txclk(struct fsl_spdif_priv *spdif_priv, continue;
ret = fsl_spdif_txclk_caldiv(spdif_priv, clk, savesub, index,
i == STC_TXCLK_SPDIF_ROOT);
i == STC_TXCLK_SPDIF_ROOT,i == SYSCLK_NUM);@@ -1210,7 +1214,8 @@ static int fsl_spdif_probe(struct platform_device *pdev) }
/* Get system clock for rx clock rate calculation */
- spdif_priv->sysclk = devm_clk_get(&pdev->dev, "rxtx5");
- spdif_priv->sysclk = devm_clk_get(&pdev->dev,
"rxtx" __stringify(SYSCLK_NUM));-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On Wed, Feb 25, 2015 at 10:53:37PM +0800, Shawn Guo wrote:
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by
Applied, thanks.
On Thu, Feb 26, 2015 at 11:12:50AM +0900, Mark Brown wrote:
On Wed, Feb 25, 2015 at 10:53:37PM +0800, Shawn Guo wrote:
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by
Applied, thanks.
Mark,
Sorry that I did not make it clear in the cover-letter. But the first patch introduces a helper function clk_is_match(), on which all the other patches in the series depend. That said, the patch series should probably be handled by Mike as a whole.
Shawn
On Thu, Feb 26, 2015 at 10:20:00AM +0800, Shawn Guo wrote:
Sorry that I did not make it clear in the cover-letter. But the first patch introduces a helper function clk_is_match(), on which all the other patches in the series depend. That said, the patch series should probably be handled by Mike as a whole.
Ugh, right. I've dropped those patches then.
On Wed, Feb 25, 2015 at 10:53:37PM +0800, Shawn Guo wrote:
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by
Acked-by: Mark Brown broonie@kernel.org
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by simply comparing two pointers. That's because with the per-user clk change, a brand new struct clk is created whenever clients try to look up the clock by calling clk_get() or sister functions like clk_get_sys() and of_clk_get(). This changes the original behavior where the struct clk is only created for once when clock driver registers the clock to CCF in the first place. The net change here is before commit 035a61c314eb the struct clk pointer is unique for given hardware clock, while after the commit the pointers returned by clk lookup calls become different for the same hardware clock.
That said, the struct clk pointer comparing in the code doesn't work any more. Call helper function clk_is_match() instead to fix the problem.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/kirkwood/kirkwood-i2s.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index def7d8260c4e..d19483081f9b 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -579,7 +579,7 @@ 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) { + if (clk_is_match(priv->extclk, priv->clk)) { devm_clk_put(&pdev->dev, priv->extclk); priv->extclk = ERR_PTR(-EINVAL); } else {
On Wed, Feb 25, 2015 at 10:53:38PM +0800, Shawn Guo wrote:
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by
Applied, thanks.
On Wed, Feb 25, 2015 at 10:53:38PM +0800, Shawn Guo wrote:
Since commit 035a61c314eb ("clk: Make clk API return per-user struct clk instances"), clk API users can no longer check if two struct clk pointers are pointing to the same hardware clock, i.e. struct clk_hw, by
Acked-by: Mark Brown broonie@kernel.org
On Wed, Feb 25, 2015 at 10:53:30PM +0800, Shawn Guo wrote:
On the first day back from Chinese new year holiday, I got a regression report from rmk, saying Ethernet stops working on HimmingBoard with v4.0-rc1.
I read through the thread [1] and found a couple of i.MX audio drivers are also affected per Stephen's Coccinelle report. That's why I came up with this series based on Quentin's structclk.cocci, Stephen's result and Mike's input (thanks all).
Not all places need to be fixed in this way.
There are two broad cases:
1. Where we are trying to determine if two clocks obtained from clk_get() are the same clock. IOW:
probe() { clk1 = clk_get(dev, ...); clk2 = clk_get(dev, ...);
if (clk1 == clk2) do_something(); }
2. Where we are trying to determine if a clock selected from a set of previously obtained clk_get()'d clocks is the same as a one of those clocks. IOW:
probe() { clk1 = clk_get(dev, ...); clk2 = clk_get(dev, ...); } ... some_fn() { clk = select_best_clock(clk1, clk2); if (clk == previously_selected_clk) { previously_selected_clk = clk; do_something(); } }
Case 1 applies in places like the Kirkwood I2S driver, and the iMX6 ethernet code, and it's these cases which need to be fixed.
Case 2 applies in the Armada DRM driver, and these cases need not be "fixed".
To put it a different way: case 1 is when you're testing to see whether two clocks refer to the same clock. case 2 is when you're testing whether the cached clk cookie is the same.
Quoting Russell King - ARM Linux (2015-02-25 07:03:49)
On Wed, Feb 25, 2015 at 10:53:30PM +0800, Shawn Guo wrote:
On the first day back from Chinese new year holiday, I got a regression report from rmk, saying Ethernet stops working on HimmingBoard with v4.0-rc1.
I read through the thread [1] and found a couple of i.MX audio drivers are also affected per Stephen's Coccinelle report. That's why I came up with this series based on Quentin's structclk.cocci, Stephen's result and Mike's input (thanks all).
Not all places need to be fixed in this way.
There are two broad cases:
Where we are trying to determine if two clocks obtained from clk_get() are the same clock. IOW:
probe() { clk1 = clk_get(dev, ...); clk2 = clk_get(dev, ...); if (clk1 == clk2) do_something(); }Where we are trying to determine if a clock selected from a set of previously obtained clk_get()'d clocks is the same as a one of those clocks. IOW:
probe() { clk1 = clk_get(dev, ...); clk2 = clk_get(dev, ...); }... some_fn() { clk = select_best_clock(clk1, clk2); if (clk == previously_selected_clk) { previously_selected_clk = clk; do_something(); } }
Case 1 applies in places like the Kirkwood I2S driver, and the iMX6 ethernet code, and it's these cases which need to be fixed.
Case 2 applies in the Armada DRM driver, and these cases need not be "fixed".
To put it a different way: case 1 is when you're testing to see whether two clocks refer to the same clock. case 2 is when you're testing whether the cached clk cookie is the same.
It looks like patches 2, 7 and 8 are correct in this series. I'll apply them towards -rc2 if nobody objects.
Regards, Mike
-- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
On 02/25/15 09:55, Mike Turquette wrote:
Quoting Russell King - ARM Linux (2015-02-25 07:03:49)
Case 1 applies in places like the Kirkwood I2S driver, and the iMX6 ethernet code, and it's these cases which need to be fixed.
Case 2 applies in the Armada DRM driver, and these cases need not be "fixed".
To put it a different way: case 1 is when you're testing to see whether two clocks refer to the same clock. case 2 is when you're testing whether the cached clk cookie is the same.
It looks like patches 2, 7 and 8 are correct in this series. I'll apply them towards -rc2 if nobody objects.
For patch 8 can't we use the alternative patch that I came up with before? I have a patch for sound/soc/fsl/fsl_esai.c and drivers/pwm/pwm-atmel-hlcdc.c which I can send shortly.
On Wed, Feb 25, 2015 at 12:42:45PM -0800, Stephen Boyd wrote:
On 02/25/15 09:55, Mike Turquette wrote:
Quoting Russell King - ARM Linux (2015-02-25 07:03:49)
Case 1 applies in places like the Kirkwood I2S driver, and the iMX6 ethernet code, and it's these cases which need to be fixed.
Case 2 applies in the Armada DRM driver, and these cases need not be "fixed".
To put it a different way: case 1 is when you're testing to see whether two clocks refer to the same clock. case 2 is when you're testing whether the cached clk cookie is the same.
It looks like patches 2, 7 and 8 are correct in this series. I'll apply them towards -rc2 if nobody objects.
For patch 8 can't we use the alternative patch that I came up with before? I have a patch for sound/soc/fsl/fsl_esai.c and drivers/pwm/pwm-atmel-hlcdc.c which I can send shortly.
This beats the idea of clk_is_match() helper. I prefer to fix this generic problem in a generic way, i.e. using clk_is_match(), which should be much easier and safer during -rc cycle.
Shawn
Quoting Stephen Boyd (2015-02-25 12:42:45)
On 02/25/15 09:55, Mike Turquette wrote:
Quoting Russell King - ARM Linux (2015-02-25 07:03:49)
Case 1 applies in places like the Kirkwood I2S driver, and the iMX6 ethernet code, and it's these cases which need to be fixed.
Case 2 applies in the Armada DRM driver, and these cases need not be "fixed".
To put it a different way: case 1 is when you're testing to see whether two clocks refer to the same clock. case 2 is when you're testing whether the cached clk cookie is the same.
It looks like patches 2, 7 and 8 are correct in this series. I'll apply them towards -rc2 if nobody objects.
For patch 8 can't we use the alternative patch that I came up with before? I have a patch for sound/soc/fsl/fsl_esai.c and drivers/pwm/pwm-atmel-hlcdc.c which I can send shortly.
Probably we can use them. I'll need to spend a few minutes with both patches and make sure it looks good. clkdev stuff always spins my head around.
Regards, Mike
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
participants (15)
-
Arnd Bergmann -
Ben Dooks -
Ben Dooks -
Boris Brezillon -
Geert Uytterhoeven -
Mark Brown -
Mike Turquette -
Nicolas Ferre -
Philipp Zabel -
Ricard Wanderlof -
Russell King - ARM Linux -
Shawn Guo -
Stephen Boyd -
Thierry Reding -
Uwe Kleine-König