Hi all,
These patches are an attempt to allow platforms to share clock code. At
present, the definitions of 'struct clk' are local to platform code,
which makes allocating and initialising cross-platform clock sources
difficult, and makes it impossible to compile a single image containing
support for two ARM platforms with different struct clks.
The two patches are for the architecture-independent kernel code,
introducing the common clk infrastructure. The changelog for the first
patch includes details about the new clock definitions.
As requested by rmk, I've put together a small series of patches
illustrating the usage of the common struct clock on the ARM imx51
platform. These are available in my git tree:
git://kernel.ubuntu.com/jk/dt/linux-2.6
in the clk-common-mx51 branch (clk-common..clk-common-mx51).
The approach I've taken with the imx51 port is to temporarly duplicate
the platform-common clock code (ie, for all mxc-based boards) to enable
usage of the common struct clk on one machine (imx51), while leaving the
others as-is. For a proper platform-wide usage of the common struct clk,
we'd be better off doing the whole platform at once. However, mx51 is
the only mxc-based HW I have, hence the duplicated example port.
In the example port, the first change simply converts the mxc's struct
clk to a struct clk_mxc, using the new API. The subsequent patches move
certain clocks to more specific data structures (eg clk_fixed and
clk_pll) where possible.
Ben Herrenschmidt is looking at using common struct clk code for powerpc
too, hence the kernel-wide approach.
Comments most welcome.
Cheers,
Jeremy
--
v5:
* uninline main API, and share definitions with !USE_COMMON_STRUCT_CLK
* add __clk_get
* delay mutex init
* kerneldoc for struct clk
v4:
* use mutex for enable/disable locking
* DEFINE_CLK -> INIT_CLK, and pass the clk name for mutex init
* struct clk_operations -> struct clk_ops
v3:
* do clock usage refcounting in common code
* provide sample port
v2:
* no longer ARM-specific
* use clk_operations
---
Jeremy Kerr (2):
Add a common struct clk
clk: Generic support for fixed-rate clocks
We currently have 21 definitions of struct clk in the ARM architecture,
each defined on a per-platform basis. This makes it difficult to define
platform- (or architecture-) independent clock sources without making
assumptions about struct clk, and impossible to compile two
platforms with different struct clks into a single image.
This change is an effort to unify struct clk where possible, by defining
a common struct clk, containing a set of clock operations. Different
clock implementations can set their own operations, and have a standard
interface for generic code. The callback interface is exposed to the
kernel proper, while the clock implementations only need to be seen by
the platform internals.
This allows us to share clock code among platforms, and makes it
possible to dynamically create clock devices in platform-independent
code.
Platforms can enable the generic struct clock through
CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
consists of a common struct clk:
struct clk {
const struct clk_ops *ops;
unsigned int enable_count;
struct mutex mutex;
};
And a set of clock operations (defined per type of clock):
struct clk_operations {
int (*enable)(struct clk *);
void (*disable)(struct clk *);
unsigned long (*get_rate)(struct clk *);
[...]
};
To define a hardware-specific clock, machine code can "subclass" the
struct clock into a new struct (adding any device-specific data), and
provide a set of operations:
struct clk_foo {
struct clk clk;
void __iomem *some_register;
};
struct clk_operations clk_foo_ops = {
.get_rate = clk_foo_get_rate,
};
The common clock definitions are based on a development patch from Ben
Herrenschmidt <[email protected]>.
Signed-off-by: Jeremy Kerr <[email protected]>
---
arch/Kconfig | 3 +
include/linux/clk.h | 77 +++++++++++++++++++++++++++++----
kernel/Makefile | 1
kernel/clk.c | 101 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 173 insertions(+), 9 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index acda512..2458b5e 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
config HAVE_USER_RETURN_NOTIFIER
bool
+config USE_COMMON_STRUCT_CLK
+ bool
+
source "kernel/gcov/Kconfig"
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1d37f42..5c1098b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -3,6 +3,7 @@
*
* Copyright (C) 2004 ARM Limited.
* Written by Deep Blue Solutions Limited.
+ * Copyright (c) 2010 Jeremy Kerr <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -11,18 +12,82 @@
#ifndef __LINUX_CLK_H
#define __LINUX_CLK_H
+#include <linux/err.h>
+#include <linux/mutex.h>
+
struct device;
-/*
- * The base API.
+#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+
+/* If we're using the common struct clk, we define the base clk object here */
+
+/**
+ * struct clk - hardware independent clock structure
+ * @clk_ops: implementation-specific ops for this clock
+ * @enable_count: count of clk_enable() calls active on this clock
+ * @mutex: lock for enable/disable or other HW-specific ops
+ *
+ * The base clock object, used by drivers for hardware-independent manipulation
+ * of clock lines. This will be 'subclassed' by device-specific implementations,
+ * which add device-specific data to struct clk. For example:
+ *
+ * struct clk_foo {
+ * struct clk;
+ * [device specific fields]
+ * };
+ *
+ * The clock driver code will manage the device-specific data, and pass
+ * clk_foo.clk to the common clock code. The clock driver will be called
+ * through the @ops callbacks.
+ *
+ * The @enable_count and @mutex members are initialised when a clock is
+ * registered with the arch-specific clock management code; the clock driver
+ * code does not need to handle these.
+ */
+struct clk {
+ const struct clk_ops *ops;
+ unsigned int enable_count;
+ struct mutex mutex;
+};
+
+#define INIT_CLK(o) { .ops = &o, }
+
+struct clk_ops {
+ int (*enable)(struct clk *);
+ void (*disable)(struct clk *);
+ int (*get)(struct clk *);
+ void (*put)(struct clk *);
+ unsigned long (*get_rate)(struct clk *);
+ long (*round_rate)(struct clk *, unsigned long);
+ int (*set_rate)(struct clk *, unsigned long);
+ int (*set_parent)(struct clk *, struct clk *);
+ struct clk * (*get_parent)(struct clk *);
+};
+
+/**
+ * __clk_get - update clock-specific refcounter
+ *
+ * @clk: The clock to refcount
+ *
+ * Before a clock is returned from clk_get, this function should be called
+ * to update any clock-specific refcounting.
+ *
+ * Returns non-zero on success, zero on failure.
+ *
+ * Drivers should not need this function; it is only needed by the
+ * arch-specific clk_get() implementations.
*/
+int __clk_get(struct clk *clk);
+#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
/*
- * struct clk - an machine class defined object / cookie.
+ * Global clock object, actual structure is declared per-machine
*/
struct clk;
+#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
+
/**
* clk_get - lookup and obtain a reference to a clock producer.
* @dev: device for clock "consumer"
@@ -83,12 +148,6 @@ unsigned long clk_get_rate(struct clk *clk);
*/
void clk_put(struct clk *clk);
-
-/*
- * The remaining APIs are optional for machine class support.
- */
-
-
/**
* clk_round_rate - adjust a rate to the exact rate a clock can provide
* @clk: clock source
diff --git a/kernel/Makefile b/kernel/Makefile
index 057472f..1ae15aa 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -105,6 +105,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
obj-$(CONFIG_PADATA) += padata.o
+obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o
ifneq ($(CONFIG_SCHED_OMIT_FRAME_POINTER),y)
# According to Alan Modra <[email protected]>, the -fno-omit-frame-pointer is
diff --git a/kernel/clk.c b/kernel/clk.c
new file mode 100644
index 0000000..cdea25f
--- /dev/null
+++ b/kernel/clk.c
@@ -0,0 +1,101 @@
+/*
+ * Copyright (C) 2010 Canonical Ltd <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Standard functionality for the common clock API.
+ */
+
+#include <linux/clk.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+
+int clk_enable(struct clk *clk)
+{
+ int ret = 0;
+
+ if (!clk->ops->enable)
+ return 0;
+
+ mutex_lock(&clk->mutex);
+ if (!clk->enable_count)
+ ret = clk->ops->enable(clk);
+
+ if (!ret)
+ clk->enable_count++;
+ mutex_unlock(&clk->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(clk_enable);
+
+void clk_disable(struct clk *clk)
+{
+ if (!clk->ops->disable)
+ return;
+
+ mutex_lock(&clk->mutex);
+
+ if (!--clk->enable_count)
+ clk->ops->disable(clk);
+
+ mutex_unlock(&clk->mutex);
+}
+EXPORT_SYMBOL_GPL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+ if (clk->ops->get_rate)
+ return clk->ops->get_rate(clk);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(clk_get_rate);
+
+int __clk_get(struct clk *clk)
+{
+ if (clk->ops->get)
+ return clk->ops->get(clk);
+ return 1;
+}
+EXPORT_SYMBOL_GPL(__clk_get);
+
+void clk_put(struct clk *clk)
+{
+ if (clk->ops->put)
+ clk->ops->put(clk);
+}
+EXPORT_SYMBOL_GPL(clk_put);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ if (clk->ops->round_rate)
+ return clk->ops->round_rate(clk, rate);
+ return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ if (clk->ops->set_rate)
+ return clk->ops->set_rate(clk, rate);
+ return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ if (clk->ops->set_parent)
+ return clk->ops->set_parent(clk, parent);
+ return -ENOSYS;
+}
+EXPORT_SYMBOL_GPL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+ if (clk->ops->get_parent)
+ return clk->ops->get_parent(clk);
+ return ERR_PTR(-ENOSYS);
+}
+EXPORT_SYMBOL_GPL(clk_get_parent);
Since most platforms will need a fixed-rate clock, add one. This will
also serve as a basic example of an implementation of struct clk.
Signed-off-by: Jeremy Kerr <[email protected]>
---
include/linux/clk.h | 13 +++++++++++++
kernel/clk.c | 14 ++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 5c1098b..1dce473 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -79,6 +79,19 @@ struct clk_ops {
*/
int __clk_get(struct clk *clk);
+/* Simple fixed-rate clock */
+struct clk_fixed {
+ struct clk clk;
+ unsigned long rate;
+};
+
+extern struct clk_ops clk_fixed_ops;
+
+#define INIT_CLK_FIXED(r) { \
+ .clk = INIT_CLK(clk_fixed_ops), \
+ .rate = (r) \
+}
+
#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
/*
diff --git a/kernel/clk.c b/kernel/clk.c
index cdea25f..32f25ef 100644
--- a/kernel/clk.c
+++ b/kernel/clk.c
@@ -99,3 +99,17 @@ struct clk *clk_get_parent(struct clk *clk)
return ERR_PTR(-ENOSYS);
}
EXPORT_SYMBOL_GPL(clk_get_parent);
+
+/* clk_fixed support */
+
+#define to_clk_fixed(clk) (container_of(clk, struct clk_fixed, clk))
+
+static unsigned long clk_fixed_get_rate(struct clk *clk)
+{
+ return to_clk_fixed(clk)->rate;
+}
+
+struct clk_ops clk_fixed_ops = {
+ .get_rate = clk_fixed_get_rate,
+};
+EXPORT_SYMBOL_GPL(clk_fixed_ops);
2010/6/21 Jeremy Kerr <[email protected]>:
> Comments most welcome.
I think this is a step in the right direction, if it gets merged I will switch
the U300 over to it.
Yours,
Linus Walleij
On 06/21/2010 05:35 PM, Jeremy Kerr wrote:
> Since most platforms will need a fixed-rate clock, add one. This will
> also serve as a basic example of an implementation of struct clk.
>
> Signed-off-by: Jeremy Kerr <[email protected]>
>
> ---
> include/linux/clk.h | 13 +++++++++++++
> kernel/clk.c | 14 ++++++++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 5c1098b..1dce473 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -79,6 +79,19 @@ struct clk_ops {
> */
> int __clk_get(struct clk *clk);
>
> +/* Simple fixed-rate clock */
> +struct clk_fixed {
> + struct clk clk;
> + unsigned long rate;
> +};
Not sure if this has been discussed already, but shouldn't rate be const
for a fixed clock?
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
Hi Ryan,
> Not sure if this has been discussed already, but shouldn't rate be const
> for a fixed clock?
No, we may need to set the rate at some point - eg, when setting up the
clocks, or parsing from the device tree.
Cheers,
Jeremy
Hi Jeremy,
On Mon, Jun 21, 2010 at 01:35:13PM +0800, Jeremy Kerr wrote:
> We currently have 21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.
>
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, containing a set of clock operations. Different
> clock implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
>
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
>
> Platforms can enable the generic struct clock through
> CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> consists of a common struct clk:
>
> struct clk {
> const struct clk_ops *ops;
> unsigned int enable_count;
> struct mutex mutex;
> };
>
> And a set of clock operations (defined per type of clock):
>
> struct clk_operations {
That's clk_ops above, and in the code.
> int (*enable)(struct clk *);
> void (*disable)(struct clk *);
> unsigned long (*get_rate)(struct clk *);
> [...]
> };
>
> To define a hardware-specific clock, machine code can "subclass" the
> struct clock into a new struct (adding any device-specific data), and
> provide a set of operations:
>
> struct clk_foo {
> struct clk clk;
> void __iomem *some_register;
> };
>
> struct clk_operations clk_foo_ops = {
Ditto.
> .get_rate = clk_foo_get_rate,
> };
>
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <[email protected]>.
>
> Signed-off-by: Jeremy Kerr <[email protected]>
baruch
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
Hello Jeremy,
On Mon, Jun 21, 2010 at 2:35 PM, Jeremy Kerr <[email protected]> wrote:
> We currently have 21 definitions of struct clk in the ARM architecture,
> each defined on a per-platform basis. This makes it difficult to define
> platform- (or architecture-) independent clock sources without making
> assumptions about struct clk, and impossible to compile two
> platforms with different struct clks into a single image.
>
> This change is an effort to unify struct clk where possible, by defining
> a common struct clk, containing a set of clock operations. Different
> clock implementations can set their own operations, and have a standard
> interface for generic code. The callback interface is exposed to the
> kernel proper, while the clock implementations only need to be seen by
> the platform internals.
>
> This allows us to share clock code among platforms, and makes it
> possible to dynamically create clock devices in platform-independent
> code.
>
> Platforms can enable the generic struct clock through
> CONFIG_USE_COMMON_STRUCT_CLK. In this case, the clock infrastructure
> consists of a common struct clk:
>
> struct clk {
> const struct clk_ops *ops;
> unsigned int enable_count;
> struct mutex mutex;
> };
>
> And a set of clock operations (defined per type of clock):
>
> struct clk_operations {
> int (*enable)(struct clk *);
> void (*disable)(struct clk *);
> unsigned long (*get_rate)(struct clk *);
> [...]
> };
>
> To define a hardware-specific clock, machine code can "subclass" the
> struct clock into a new struct (adding any device-specific data), and
> provide a set of operations:
>
> struct clk_foo {
> struct clk clk;
> void __iomem *some_register;
> };
>
> struct clk_operations clk_foo_ops = {
> .get_rate = clk_foo_get_rate,
> };
>
> The common clock definitions are based on a development patch from Ben
> Herrenschmidt <[email protected]>.
>
> Signed-off-by: Jeremy Kerr <[email protected]>
>
> ---
> arch/Kconfig | 3 +
> include/linux/clk.h | 77 +++++++++++++++++++++++++++++----
> kernel/Makefile | 1
> kernel/clk.c | 101 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 173 insertions(+), 9 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index acda512..2458b5e 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -151,4 +151,7 @@ config HAVE_MIXED_BREAKPOINTS_REGS
> config HAVE_USER_RETURN_NOTIFIER
> bool
>
> +config USE_COMMON_STRUCT_CLK
> + bool
> +
> source "kernel/gcov/Kconfig"
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1d37f42..5c1098b 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) 2004 ARM Limited.
> * Written by Deep Blue Solutions Limited.
> + * Copyright (c) 2010 Jeremy Kerr <[email protected]>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> @@ -11,18 +12,82 @@
> #ifndef __LINUX_CLK_H
> #define __LINUX_CLK_H
>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +
> struct device;
>
> -/*
> - * The base API.
> +#ifdef CONFIG_USE_COMMON_STRUCT_CLK
> +
> +/* If we're using the common struct clk, we define the base clk object here */
> +
> +/**
> + * struct clk - hardware independent clock structure
> + * @clk_ops: implementation-specific ops for this clock
> + * @enable_count: count of clk_enable() calls active on this clock
> + * @mutex: lock for enable/disable or other HW-specific ops
> + *
> + * The base clock object, used by drivers for hardware-independent manipulation
> + * of clock lines. This will be 'subclassed' by device-specific implementations,
> + * which add device-specific data to struct clk. For example:
> + *
> + * struct clk_foo {
> + * struct clk;
> + * [device specific fields]
> + * };
> + *
> + * The clock driver code will manage the device-specific data, and pass
> + * clk_foo.clk to the common clock code. The clock driver will be called
> + * through the @ops callbacks.
> + *
> + * The @enable_count and @mutex members are initialised when a clock is
> + * registered with the arch-specific clock management code; the clock driver
> + * code does not need to handle these.
> + */
> +struct clk {
> + const struct clk_ops *ops;
> + unsigned int enable_count;
> + struct mutex mutex;
> +};
> +
> +#define INIT_CLK(o) { .ops = &o, }
> +
> +struct clk_ops {
> + int (*enable)(struct clk *);
> + void (*disable)(struct clk *);
> + int (*get)(struct clk *);
> + void (*put)(struct clk *);
> + unsigned long (*get_rate)(struct clk *);
> + long (*round_rate)(struct clk *, unsigned long);
> + int (*set_rate)(struct clk *, unsigned long);
> + int (*set_parent)(struct clk *, struct clk *);
> + struct clk * (*get_parent)(struct clk *);
> +};
> +
> +/**
> + * __clk_get - update clock-specific refcounter
> + *
> + * @clk: The clock to refcount
> + *
> + * Before a clock is returned from clk_get, this function should be called
> + * to update any clock-specific refcounting.
> + *
> + * Returns non-zero on success, zero on failure.
> + *
> + * Drivers should not need this function; it is only needed by the
> + * arch-specific clk_get() implementations.
> */
> +int __clk_get(struct clk *clk);
>
> +#else /* !CONFIG_USE_COMMON_STRUCT_CLK */
>
> /*
> - * struct clk - an machine class defined object / cookie.
> + * Global clock object, actual structure is declared per-machine
> */
> struct clk;
>
> +#endif /* !CONFIG_USE_COMMON_STRUCT_CLK */
> +
> /**
> * clk_get - lookup and obtain a reference to a clock producer.
> * @dev: device for clock "consumer"
> @@ -83,12 +148,6 @@ unsigned long clk_get_rate(struct clk *clk);
> */
> void clk_put(struct clk *clk);
>
> -
> -/*
> - * The remaining APIs are optional for machine class support.
> - */
> -
> -
> /**
> * clk_round_rate - adjust a rate to the exact rate a clock can provide
> * @clk: clock source
> diff --git a/kernel/Makefile b/kernel/Makefile
> index 057472f..1ae15aa 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -105,6 +105,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
> obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
> obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
> obj-$(CONFIG_PADATA) += padata.o
> +obj-$(CONFIG_USE_COMMON_STRUCT_CLK) += clk.o
>
> ifneq ($(CONFIG_SCHED_OMIT_FRAME_POINTER),y)
> # According to Alan Modra <[email protected]>, the -fno-omit-frame-pointer is
> diff --git a/kernel/clk.c b/kernel/clk.c
> new file mode 100644
> index 0000000..cdea25f
> --- /dev/null
> +++ b/kernel/clk.c
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (C) 2010 Canonical Ltd <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Standard functionality for the common clock API.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +
> +int clk_enable(struct clk *clk)
> +{
> + int ret = 0;
> +
> + if (!clk->ops->enable)
> + return 0;
Wouldn't it be better (safer?) to check "clk" and "clk->ops" before
accessing clk->ops->enable?
For example,
if (IS_ERR_OR_NULL(clk))
return -EINVAL;
if (!clk->ops || !clk->ops->enable)
return 0;
Or, do you think it'd be better not to check and save some time?
Anyway, if we intend to check the input, the patch for the patch
including kernel/clk.c would be...
----------------------------------------------------
diff --git a/kernel/clk.c b/kernel/clk.c
index 32f25ef..c35f0ac 100644
--- a/kernel/clk.c
+++ b/kernel/clk.c
@@ -16,7 +16,10 @@ int clk_enable(struct clk *clk)
{
int ret = 0;
- if (!clk->ops->enable)
+ if (IS_ERR_OR_NULL(clk))
+ return -EINVAL;
+
+ if (!clk->ops || !clk->ops->enable)
return 0;
mutex_lock(&clk->mutex);
@@ -33,7 +36,12 @@ EXPORT_SYMBOL_GPL(clk_enable);
void clk_disable(struct clk *clk)
{
- if (!clk->ops->disable)
+ if (IS_ERR_OR_NULL(clk)) {
+ printk(KERN_ERR "clk_disable(NULL) is called.\n");
+ return;
+ }
+
+ if (!clk->ops || !clk->ops->disable)
return;
mutex_lock(&clk->mutex);
@@ -47,7 +55,9 @@ EXPORT_SYMBOL_GPL(clk_disable);
unsigned long clk_get_rate(struct clk *clk)
{
- if (clk->ops->get_rate)
+ if (IS_ERR_OR_NULL(clk))
+ return -EINVAL;
+ if (clk->ops && clk->ops->get_rate)
return clk->ops->get_rate(clk);
return 0;
}
@@ -55,7 +65,9 @@ EXPORT_SYMBOL_GPL(clk_get_rate);
int __clk_get(struct clk *clk)
{
- if (clk->ops->get)
+ if (IS_ERR_OR_NULL(clk))
+ return -EINVAL;
+ if (clk->ops && clk->ops->get)
return clk->ops->get(clk);
return 1;
}
@@ -63,14 +75,19 @@ EXPORT_SYMBOL_GPL(__clk_get);
void clk_put(struct clk *clk)
{
- if (clk->ops->put)
+ if (IS_ERR_OR_NULL(clk)) {
+ printk(KERN_ERR "clk_disable(NULL) is called.\n");
+ return;
+ }
+
+ if (clk->ops && clk->ops->put)
clk->ops->put(clk);
}
EXPORT_SYMBOL_GPL(clk_put);
long clk_round_rate(struct clk *clk, unsigned long rate)
{
- if (clk->ops->round_rate)
+ if (!IS_ERR_OR_NULL(clk) && clk->ops && clk->ops->round_rate)
return clk->ops->round_rate(clk, rate);
return -ENOSYS;
}
@@ -78,7 +95,7 @@ EXPORT_SYMBOL_GPL(clk_round_rate);
int clk_set_rate(struct clk *clk, unsigned long rate)
{
- if (clk->ops->set_rate)
+ if (!IS_ERR_OR_NULL(clk) && clk->ops && clk->ops->set_rate)
return clk->ops->set_rate(clk, rate);
return -ENOSYS;
}
@@ -86,7 +103,7 @@ EXPORT_SYMBOL_GPL(clk_set_rate);
int clk_set_parent(struct clk *clk, struct clk *parent)
{
- if (clk->ops->set_parent)
+ if (!IS_ERR_OR_NULL(clk) && clk->ops && clk->ops->set_parent)
return clk->ops->set_parent(clk, parent);
return -ENOSYS;
}
@@ -94,7 +111,7 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
struct clk *clk_get_parent(struct clk *clk)
{
- if (clk->ops->get_parent)
+ if (!IS_ERR_OR_NULL(clk) && clk->ops && clk->ops->get_parent)
return clk->ops->get_parent(clk);
return ERR_PTR(-ENOSYS);
}
> +
> + mutex_lock(&clk->mutex);
> + if (!clk->enable_count)
> + ret = clk->ops->enable(clk);
> +
> + if (!ret)
> + clk->enable_count++;
> + mutex_unlock(&clk->mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_enable);
> +
> +void clk_disable(struct clk *clk)
> +{
> + if (!clk->ops->disable)
> + return;
> +
> + mutex_lock(&clk->mutex);
> +
> + if (!--clk->enable_count)
> + clk->ops->disable(clk);
> +
> + mutex_unlock(&clk->mutex);
> +}
> +EXPORT_SYMBOL_GPL(clk_disable);
> +
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> + if (clk->ops->get_rate)
> + return clk->ops->get_rate(clk);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_rate);
> +
> +int __clk_get(struct clk *clk)
> +{
> + if (clk->ops->get)
> + return clk->ops->get(clk);
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(__clk_get);
> +
> +void clk_put(struct clk *clk)
> +{
> + if (clk->ops->put)
> + clk->ops->put(clk);
> +}
> +EXPORT_SYMBOL_GPL(clk_put);
> +
> +long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> + if (clk->ops->round_rate)
> + return clk->ops->round_rate(clk, rate);
> + return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_round_rate);
> +
> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> + if (clk->ops->set_rate)
> + return clk->ops->set_rate(clk, rate);
> + return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_rate);
> +
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> + if (clk->ops->set_parent)
> + return clk->ops->set_parent(clk, parent);
> + return -ENOSYS;
> +}
> +EXPORT_SYMBOL_GPL(clk_set_parent);
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> + if (clk->ops->get_parent)
> + return clk->ops->get_parent(clk);
> + return ERR_PTR(-ENOSYS);
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858
Hi MyungJoo,
> > +int clk_enable(struct clk *clk)
> > +{
> > + int ret = 0;
> > +
> > + if (!clk->ops->enable)
> > + return 0;
>
> Wouldn't it be better (safer?) to check "clk" and "clk->ops" before
> accessing clk->ops->enable?
> For example,
>
> if (IS_ERR_OR_NULL(clk))
> return -EINVAL;
>
> if (!clk->ops || !clk->ops->enable)
> return 0;
>
> Or, do you think it'd be better not to check and save some time?
>
> Anyway, if we intend to check the input, the patch for the patch
> including kernel/clk.c would be...
I think that we should leave it as-is; although it may be 'safer', it
may mean more subtle bugs can arise because we've been handed an invalid
clock pointer.
I'd rather the code oops on first usage so that the developer realises
that something is broken, rather than fail with an error code.
BTW - nice work on the samsung implementation, I will check it out soon.
Cheers,
Jeremy