Skip to contentSkip to conversation

clk.h: Fix shim ifdef guard (HAVE_CLK -> COMMON_CLK)

PATCH In review Jeff Mahoney 2 participants 4 messages Aug 27, 2012 – Aug 28, 2012 Clk API

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 clk.h: Fix shim ifdef guard (HAVE_CLK -> COMMON_CLK)

Commit 93abe8e4 (clk: add non HAVE_CLK routines) added shims for the clk code but HAVE_CLK isn't enough. It's possible to have the clk support but not enable it. We end up with full prototypes for code that is never built - causing module linking to fail later.

This patch changes the guard to use COMMON_CLK, which actually guards the code.

---
 include/linux/clk.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/clk.h +2 -2
@@ -120,7 +120,7 @@ static inline void clk_unprepare(struct
 }
 #endif
 
-#ifdef CONFIG_HAVE_CLK
+#ifdef CONFIG_COMMON_CLK
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
  * @dev: device for clock "consumer"
@@ -276,7 +276,7 @@ struct clk *clk_get_parent(struct clk *c
  */
 struct clk *clk_get_sys(const char *dev_id, const char *con_id);
 
-#else /* !CONFIG_HAVE_CLK */
+#else /* !CONFIG_COMMON_CLK */
 
 static inline struct clk *clk_get(struct device *dev, const char *id)
 {
Signature · 2 lines
Jeff Mahoney
SUSE Labs
#1 lore raw
Quoted 7 lines · Jeff Mahoney

Commit 93abe8e4 (clk: add non HAVE_CLK routines) added shims for the clk code but HAVE_CLK isn't enough. It's possible to have the clk support but not enable it. We end up with full prototypes for code that is never built - causing module linking to fail later.

This patch changes the guard to use COMMON_CLK, which actually guards the code.

This is wrong. COMMON_CLK is an _implementation_ of the CLK API. It is not the only implementation in the kernel. Conditionalizing like this breaks existing users.

HAVE_CLK is the right thing here - if you define HAVE_CLK then you _are_ providing an implementation of clk_get() et.al. If you're not, then you do not define HAVE_CLK. Simples.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
#2 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 10 lines · Russell King
Quoted 8 lines · Jeff Mahoney

Commit 93abe8e4 (clk: add non HAVE_CLK routines) added shims for the clk code but HAVE_CLK isn't enough. It's possible to have the clk support but not enable it. We end up with full prototypes for code that is never built - causing module linking to fail later.

This patch changes the guard to use COMMON_CLK, which actually guards the code.

Quoted 8 lines

This is wrong. COMMON_CLK is an _implementation_ of the CLK API. It is not the only implementation in the kernel. Conditionalizing like this breaks existing users.

HAVE_CLK is the right thing here - if you define HAVE_CLK then you _are_ providing an implementation of clk_get() et.al. If you're not, then you do not define HAVE_CLK. Simples.

Ok. Thanks for the feedback.

It would seem, then, that the powerpc clock implementation doesn't implement the full API and that's where we're running into problems.

Specifically, the use of clk_devm_get in drivers/usb/chipidea/ci13xxx_imx.c and drivers/char/hw_random/exynos-rng.ko is causing module link errors on powerpc configs for the EFIKA hardware. The EXYNOS RNG driver should probably have a dependency on ARCH_EXYNOS. The chipidea driver looks like it was developed on ARM hardware - does it exist anywhere else?

Should the clk_devm stuff be moved someplace generic? I don't have a horse in this race. My only interest here is to get these configs to build again. :)

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.18 (Darwin)
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJQO95XAAoJEB57S2MheeWy+QQQAK8HRdlhtyj3yy9S1zQ8c15x 72xwM52Yhhwl1L4OsuD4oNjzwGxj54j7VMfe9TMkrrozC7eySlnm3ErSnVNfn6Dp RnUvjAs+Wr9vYR18f3IgFrYMnu2M3PkxBFciv/K94soCZ90iKuFs6yebqtkVUzP3 XfVmVdJpJpe+GXvGBW7lo8PfZNyLbxhleTqSXftTszuLcTlF8361aZmxz3UKpiWD EB5a079s1CKf1KAu61P0drEIVHgAayffHjm4TDAE3AYzM7nwcvqjZQcjxSfI9eHF iPOeS0Kt2+9TrLJjp9p/dZX3dLFHhG5LtS6qZq372JVjZggPGknAdV/MK33qEzuN Fnr+ag5DeB/Kujfw2NcC+RQoayrMBK52eSxZO451xwHZef0+JP2eVLhht9zfmzDh 4cRj6gybKklEry/kABhy5O59iNVrjn+WrbVFwGO5aezZbAoqnZm2Sb5pU6aDLV6m p3IMTfFMA13rh8sn/XstFN50T2xG4SP1DMqiV6AbfcfSXyRM0vQ9etOkfjp2ftq+ blbjwNBVp8Hp47q04LIw1XPxDMOEpBX6bVFiIqOWT9GvhvFysetNpB6Ae00FIo7p 1Zdv0Bq9IkWhZ+BxLeqgMUO445GIqUk7k2AElI8G3cCIKZlNp5Od/hFx7rQJ6Gwq dqydYWi1ogVqg3YpSI7a
=/UyR
-----END PGP SIGNATURE-----

#3 lore raw
Quoted 3 lines · Jeff Mahoney

Should the clk_devm stuff be moved someplace generic? I don't have a horse in this race. My only interest here is to get these configs to build again. :)

devm_clk_get() should not be in clkdev.c, but should be entirely separate, allowing anyone who is using the clk API to use it. But unfortunately, that's not how it ended up getting added... Moving it is something that's on my very long todo list.

It should still remain in drivers/clk, but it should be in a separate file, probably built conditionally by HAVE_CLK being defined.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of: