Skip to contentSkip to conversation

Fix struct clk pointer comparing

PATCH Applied Shawn Guo 15 participants 41 messages Feb 25, 2015 – Mar 12, 2015

Subsystems (14)
lore
Share & tools
lore

Shawn Guo

author
#0 lore raw

cover 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(-)
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#1 lore raw

PATCH 1/8 clk: add helper function clk_is_match()

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.

---
 drivers/clk/clk.c   |  6 ++++++
 include/linux/clk.h | 16 ++++++++++++++++
 2 files changed, 22 insertions(+)
drivers/clk/clk.c +6 -0
index eb0152961d3c..7df4826d9c7b 100644
@@ -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)
 {
include/linux/clk.h +16 -0
index 8381bbfbc308..f8a8a6dd6a98 100644
@@ -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. */
Signature · 1 line
1.9.1
#2 lore raw

Re: [PATCH 1/8] clk: add helper function clk_is_match()

Quoting Shawn Guo (2015-02-25 06:53:31)

Quoted 13 lines

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.

Quoted 7 lines

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.

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 <[email protected]>
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 <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Shawn Guo <[email protected]>
Cc Tomeu Vizoso <[email protected]>
---
 drivers/clk/clk.c   | 26 ++++++++++++++++++++++++++
 include/linux/clk.h | 18 ++++++++++++++++++
 2 files changed, 44 insertions(+)
drivers/clk/clk.c +26 -0
index eb01529..09670de 100644
@@ -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
include/linux/clk.h +18 -0
index 8381bbf..5c076e4 100644
@@ -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. */
Signature · 1 line
1.9.1
author Tested-by
#3 lore raw

Re: [PATCH 1/8] clk: add helper function clk_is_match()

Quoted 5 lines · Mike Turquette

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 :)

Quoted 10 lines

From: Michael Turquette <[email protected]>
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.

Quoted 7 lines

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 <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Shawn Guo <[email protected]>
Quoted 2 lines
#4 lore raw

Re: [PATCH 1/8] clk: add helper function clk_is_match()

In reply to Mike Turquette
Quoted 37 lines · Mike Turquette

Quoting Shawn Guo (2015-02-25 06:53:31)

Quoted 13 lines

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.

Quoted 7 lines

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.

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 <[email protected]>
Date: Wed, 25 Feb 2015 09:11:01 -0800
Subject: [PATCH] clk: introduce clk_is_match

Quoted 8 lines

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.

Signature · 3 lines
Ben Dooks, [email protected], http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.
#5 lore raw

Re: [alsa-devel] [PATCH 1/8] clk: add helper function clk_is_match()

Am Donnerstag, den 26.02.2015, 09:02 +0000 schrieb Ben Dooks:

Quoted 1 line · Mike Turquette

[...]

Quoted 10 lines
Quoted 10 lines

From: Michael Turquette <[email protected]>
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.

Quoted 4 lines
Quoted 2 lines

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

#6 lore raw

Re: [alsa-devel] [PATCH 1/8] clk: add helper function clk_is_match()

Quoted 13 lines · Philipp Zabel

Am Donnerstag, den 26.02.2015, 09:02 +0000 schrieb Ben Dooks:

Quoted 1 line · Mike Turquette

[...]

Quoted 10 lines
Quoted 10 lines

From: Michael Turquette <[email protected]>
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.

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

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".

Signature · 3 lines
Ben Dooks, [email protected], http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.
#7 lore raw

Re: [alsa-devel] [PATCH 1/8] clk: add helper function clk_is_match()

Quoted 8 lines · Ben Dooks
Quoted 3 lines
Quoted 1 line

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

Signature · 3 lines
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30
#8 lore raw

Re: [PATCH 1/8] clk: add helper function clk_is_match()

In reply to Mike Turquette
Quoted 33 lines · Mike Turquette

From: Michael Turquette <[email protected]>
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 <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Shawn Guo <[email protected]>
Cc Tomeu Vizoso <[email protected]>
---
 drivers/clk/clk.c   | 26 ++++++++++++++++++++++++++
 include/linux/clk.h | 18 ++++++++++++++++++
 2 files changed, 44 insertions(+)
drivers/clk/clk.c +1 -0
index eb01529..09670de 100644
@@ -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

drivers/clk/clk.c int clk_get_phase(struct clk *clk) · line 2184

Quoted 10 lines
drivers/clk/clk.c +10 -0
+ * @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

Quoted 8 lines
include/linux/clk.h +2 -0
@@ -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

include/linux/clk.h struct clk *clk_get_parent(struct clk *clk); · line 390

Quoted 10 lines
include/linux/clk.h +10 -0
+ * @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

include/linux/clk.h static inline struct clk *clk_get_parent(struct clk *clk) · line 445

Quoted 9 lines
include/linux/clk.h +2 -0
+
 #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

include/linux/clk.h static inline struct clk *clk_get_parent(struct clk *clk) · line 447

Quoted 2 lines
include/linux/clk.h +2 -0
+{
+       return p == q ? true : false;

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

#9 lore raw

Re: [PATCH 1/8] clk: add helper function clk_is_match()

In reply to Mike Turquette

include/linux/clk.h static inline struct clk *clk_get_parent(struct clk *clk) · line 447

Quoted 3 lines · Mike Turquette
include/linux/clk.h +3 -0
+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.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
#10 lore raw

PATCH 1/1 clk: provide clk_is_match() dummy for non-common clk

In reply to Mike Turquette

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.

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?

include/linux/clk.h +18 -18
index 5c076e4d90f9..a9b91595d106 100644
@@ -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. */
#11 lore raw

Re: [PATCH] clk: provide clk_is_match() dummy for non-common clk

Quoted 10 lines · Arnd Bergmann

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.

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 -- [email protected]

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
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#12 lore raw

Re: [PATCH] clk: provide clk_is_match() dummy for non-common clk

In reply to Arnd Bergmann

Hello,

include/linux/clk.h static inline long clk_get_phase(struct clk *clk) · line 160

Quoted 3 lines · Arnd Bergmann
include/linux/clk.h +3 -0
+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

Signature · 6 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#13 lore raw

Re: [PATCH] clk: provide clk_is_match() dummy for non-common clk

In reply to Arnd Bergmann
Quoted 8 lines · Arnd Bergmann

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.

Quoted 6 lines
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.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
#14 lore raw

Re: [PATCH] clk: provide clk_is_match() dummy for non-common clk

Hey Russell,

Quoted 9 lines · Russell King - ARM Linux
Quoted 8 lines · Arnd Bergmann

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.

Quoted 8 lines
Quoted 6 lines
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

Signature · 6 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#15 lore raw

Re: [PATCH] clk: provide clk_is_match() dummy for non-common clk

Quoted 19 lines · Uwe Kleine-König

Hey Russell,

Quoted 16 lines · Russell King - ARM Linux
Quoted 14 lines · Arnd Bergmann

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.

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?

Quoted 3 lines
Quoted 1 line

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.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
#16 lore raw

Re: [PATCH] clk: provide clk_is_match() dummy for non-common clk

Quoted 24 lines · Russell King - ARM Linux
Quoted 18 lines · Uwe Kleine-König

Hey Russell,

Quoted 15 lines · Russell King - ARM Linux
Quoted 14 lines · Arnd Bergmann

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.

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?

Quoted 3 lines
Quoted 1 line

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*

Quoted 9 lines

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 <[email protected]>
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 <[email protected]>
Cc Shawn Guo <[email protected]>
Cc Tomeu Vizoso <[email protected]>
[[email protected]: Fix COMMON_CLK=N && HAS_CLK=Y config]

[[email protected]: const arguments to clk_is_match() and remove unnecessary ternary operation]

---
 drivers/clk/clk.c   | 26 ++++++++++++++++++++++++++
 include/linux/clk.h | 18 ++++++++++++++++++
 2 files changed, 44 insertions(+)
drivers/clk/clk.c +26 -0
index b9f85fc2ce3f..237f23f68bfc 100644
@@ -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
include/linux/clk.h +18 -0
index 8381bbfbc308..68c16a6bedb3 100644
@@ -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
 
 /**
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#17 lore raw

PATCH 2/8 ARM: imx: fix struct clk pointer comparing

In reply to Shawn Guo

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.

---
 arch/arm/mach-imx/mach-imx6q.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
arch/arm/mach-imx/mach-imx6q.c +3 -2
index 4ad6e473cf83..9de3412af406 100644
@@ -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,
Signature · 1 line
1.9.1
author
#18 lore raw

PATCH 3/8 drm: armada: fix struct clk pointer comparing

In reply to Shawn Guo

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.

---
 drivers/gpu/drm/armada/armada_510.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/armada/armada_510.c +1 -1
index ad3d2ebf95c9..862deafe8b24 100644
@@ -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;
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#19 lore raw

PATCH 4/8 pwm: atmel-hlcdc: fix struct clk pointer comparing

In reply to Shawn Guo

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.

---
 drivers/pwm/pwm-atmel-hlcdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pwm/pwm-atmel-hlcdc.c +1 -1
index 522f7075bb1a..36475949b829 100644
@@ -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;
 
Signature · 1 line
1.9.1
Acked-by on v1 4/8
#20 lore raw

Re: [PATCH 4/8] pwm: atmel-hlcdc: fix struct clk pointer comparing

Quoted 13 lines · Shawn Guo

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.

Quoted 4 lines

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.

I agree with the fix whichever name is chosen for the function in an future version of this series. So you can add my:

Maybe Boris can double check...

Quoted 18 lines
---
 drivers/pwm/pwm-atmel-hlcdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pwm/pwm-atmel-hlcdc.c +1 -1
index 522f7075bb1a..36475949b829 100644
@@ -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;
 
Signature · 1 line
Nicolas Ferre
#21 lore raw

Re: [PATCH 4/8] pwm: atmel-hlcdc: fix struct clk pointer comparing

Nicolas, Shawn,

Quoted 17 lines · On Thu, 26 Feb 2015 10:22:50 +0100 Nicolas Ferre <[email protected]>…
Quoted 16 lines · Shawn Guo

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.

Quoted 7 lines
Quoted 1 line

I agree with the fix whichever name is chosen for the function in an future version of this series. So you can add my:

Maybe Boris can double check...

Looks good to me.
Thanks for fixing that.

Quoted 21 lines
Quoted 19 lines
---
 drivers/pwm/pwm-atmel-hlcdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pwm/pwm-atmel-hlcdc.c +1 -1
index 522f7075bb1a..36475949b829 100644
@@ -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;
 
Signature · 3 lines
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Acked-by on v1 4/8
#22 lore raw

Re: [PATCH 4/8] pwm: atmel-hlcdc: fix struct clk pointer comparing

In reply to Shawn Guo
Quoted 13 lines · Shawn Guo

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.

Quoted 7 lines

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.

---
 drivers/pwm/pwm-atmel-hlcdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#23 lore raw

PATCH 5/8 serial: samsung: fix struct clk pointer comparing

In reply to Shawn Guo

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.

---
 drivers/tty/serial/samsung.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/tty/serial/samsung.c +1 -1
index af821a908720..b747066247df 100644
@@ -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)) {
Signature · 1 line
1.9.1
author
#24 lore raw

PATCH 6/8 ASoC: fsl_esai: fix struct clk pointer comparing

In reply to Shawn Guo

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.

---
 sound/soc/fsl/fsl_esai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/fsl/fsl_esai.c +1 -1
index 5c7597191e3f..6e00e51b98c2 100644
@@ -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;
Signature · 1 line
1.9.1
maintainer Acked-by on v1 6/8
#25 lore raw

Re: [PATCH 6/8] ASoC: fsl_esai: fix struct clk pointer comparing

Quoted 3 lines · Shawn Guo

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

author
#26 lore raw

PATCH 7/8 ASoC: fsl_spdif: fix struct clk pointer comparing

In reply to Shawn Guo

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.

---
 sound/soc/fsl/fsl_spdif.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
sound/soc/fsl/fsl_spdif.c +2 -2
index 75870c0ea2c9..91eb3aef7f02 100644
@@ -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",
Signature · 1 line
1.9.1
#27 lore raw

Re: [PATCH 7/8] ASoC: fsl_spdif: fix struct clk pointer comparing

Quoted 35 lines · Shawn Guo

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.

---
 sound/soc/fsl/fsl_spdif.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
sound/soc/fsl/fsl_spdif.c +1 -1
index 75870c0ea2c9..91eb3aef7f02 100644
@@ -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;

sound/soc/fsl/fsl_spdif.c static int fsl_spdif_probe_txclk(struct fsl_spdif_priv *spdif_priv, · line 1149

Quoted 8 lines
sound/soc/fsl/fsl_spdif.c +1 -1
@@ -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<----

sound/soc/fsl/fsl_spdif.c +9 -4
index af0429421fc8..1878dd62a247 100644
@@ -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);
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#28 lore raw

Re: [PATCH 7/8] ASoC: fsl_spdif: fix struct clk pointer comparing

Quoted 16 lines · Stephen Boyd
Quoted 16 lines
sound/soc/fsl/fsl_spdif.c +1 -1
index 75870c0ea2c9..91eb3aef7f02 100644
@@ -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",
Quoted 8 lines

sound/soc/fsl/fsl_spdif.c static int fsl_spdif_probe_txclk(struct fsl_spdif_priv *spdif_priv, · line 1149

Quoted 6 lines
sound/soc/fsl/fsl_spdif.c +1 -1
 			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

Quoted 63 lines

----8<----

sound/soc/fsl/fsl_spdif.c +9 -4
index af0429421fc8..1878dd62a247 100644
@@ -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);
Signature · 3 lines
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
maintainer Applied
#29 lore raw

Re: [PATCH 7/8] ASoC: fsl_spdif: fix struct clk pointer comparing

In reply to Shawn Guo
Quoted 3 lines · Shawn Guo

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.

author
#30 lore raw

Re: [PATCH 7/8] ASoC: fsl_spdif: fix struct clk pointer comparing

Quoted 6 lines · Mark Brown
Quoted 3 lines · Shawn Guo

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

maintainer
#31 lore raw

Re: [PATCH 7/8] ASoC: fsl_spdif: fix struct clk pointer comparing

Quoted 4 lines · Shawn Guo

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.

maintainer Acked-by on v1 7/8
#32 lore raw

Re: [PATCH 7/8] ASoC: fsl_spdif: fix struct clk pointer comparing

In reply to Shawn Guo
Quoted 3 lines · Shawn Guo

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

author
#33 lore raw

PATCH 8/8 ASoC: kirkwood: fix struct clk pointer comparing

In reply to Shawn Guo

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.

---
 sound/soc/kirkwood/kirkwood-i2s.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/kirkwood/kirkwood-i2s.c +1 -1
index def7d8260c4e..d19483081f9b 100644
@@ -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 {
Signature · 1 line
1.9.1
maintainer Applied
#34 lore raw

Re: [PATCH 8/8] ASoC: kirkwood: fix struct clk pointer comparing

Quoted 3 lines · Shawn Guo

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.

maintainer Acked-by on v1 8/8
#35 lore raw

Re: [PATCH 8/8] ASoC: kirkwood: fix struct clk pointer comparing

In reply to Shawn Guo
Quoted 3 lines · Shawn Guo

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

#36 lore raw
In reply to Shawn Guo
Quoted 8 lines · Shawn Guo

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.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
#37 lore raw

Quoting Russell King - ARM Linux (2015-02-25 07:03:49)

Quoted 48 lines · Shawn Guo
Quoted 8 lines

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.

Quoted 6 lines

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

Quoted 4 lines
Signature · 3 lines
-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
#38 lore raw
Quoted 4 lines · Mike Turquette

Quoting Russell King - ARM Linux (2015-02-25 07:03:49)

Quoted 3 lines

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.

Quoted 10 lines
Quoted 6 lines

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.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#39 lore raw
Quoted 11 lines · Stephen Boyd
Quoted 10 lines · Mike Turquette

Quoting Russell King - ARM Linux (2015-02-25 07:03:49)

Quoted 9 lines

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.

Quoted 8 lines
Quoted 4 lines

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

#40 lore raw
In reply to Stephen Boyd

Quoting Stephen Boyd (2015-02-25 12:42:45)

Quoted 11 lines · Mike Turquette
Quoted 10 lines

Quoting Russell King - ARM Linux (2015-02-25 07:03:49)

Quoted 9 lines

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.

Quoted 8 lines
Quoted 4 lines

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

Quoted 5 lines
Signature · 3 lines
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project