On 11/07/07 21:34 +0100, Russell King wrote: > On Wed, Jul 11, 2007 at 10:02:54AM -0700, David Brownell wrote: > > On Wednesday 11 July 2007, Christoph Hellwig wrote: > > > On Wed, Jul 11, 2007 at 08:56:58AM -0700, David Brownell wrote: > > > > > Umm, this is about the fifth almost identical implementation of > > > > > the clk_ functions. Please, please put it into common code. > > > > > > > > > > And talk to the mips folks which just got a similar comment from me. > > > > > > > > You mean like a lib/clock.c core, rather than an opsvector? > > > > > > I mean an ops vector and surrounding wrappers. Every architecture > > > is reimplementing their own dispatch table which is rather annoying. > > > > ARM doesn't. :) > > > > But then, nobody expects one kernel to support more than one > > vendor's ARM chips; or usually, more than one generation of > > that vendor's chips. So any dispatch table is specific to > > a given platform, and tuned to its quirks. Not much to share > > between OMAP and AT91, for example, except in some cases maybe > > an arm926ejs block. > > And also the information stored within a 'struct clk' is very platform > dependent. In the most basic situation, 'struct clk' may not actually > be a structure, but the clock rate. All functions with the exception of > clk_get() and clk_get_rate() could well be no-ops, clk_get() just returns > the 'struct clk' representing the rate and 'clk_get_rate' returns that > as an integer. > > More complex setups might want 'struct clk' to contain the address of a > clock enable register, the bit position to enable that clock source, the > clock rate, a refcount, and so on, all of which would be utterly useless > for a platform which had fixed rate clocks. The patch that triggered this discussion is at the end. It doesn't make any assumption on struct clk, it's just a wrapper around functions from clk.h. Point of this patch was to abstract exported functions, since arch/powerpc/ can support multiple platfroms in one binary. > > > I've not seen a solid proposal for such a thing, and it's not > > clear to me how that would play with with older code (e.g. any > > of the ARM implementations). > > If people are implementing their own incompatible changes without reference > to the API they're invalid implementations as far as I'm concerned. If > they can't bothered to lift a finger to even _talk_ to me about their > requirements they just don't have any say concerning any future > developments IMO. My changes were implemented according to clk.h API. And honestly, I just wanted to rework clocks in some SPI driver and others made me do all the clk work. ;-) Domen > > IOW, talk to me and I'll talk back. Ignore me and I'll ignore them. > > -- > Russell King > Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ > maintainer of: clk interface for arch/powerpc, platforms should fill clk_functions. Signed-off-by: Domen Puncer <domen.puncer@xxxxxxxxxxx> --- arch/powerpc/kernel/Makefile | 3 - arch/powerpc/kernel/clock.c | 82 ++++++++++++++++++++++++++++++++++++ include/asm-powerpc/clk_interface.h | 20 ++++++++ 3 files changed, 104 insertions(+), 1 deletion(-) Index: work-powerpc.git/arch/powerpc/kernel/clock.c =================================================================== --- /dev/null +++ work-powerpc.git/arch/powerpc/kernel/clock.c @@ -0,0 +1,82 @@ +/* + * Dummy clk implementations for powerpc. + * These need to be overridden in platform code. + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/module.h> +#include <asm/clk_interface.h> + +struct clk_interface clk_functions; + +struct clk *clk_get(struct device *dev, const char *id) +{ + if (clk_functions.clk_get) + return clk_functions.clk_get(dev, id); + return ERR_PTR(-ENOSYS); +} +EXPORT_SYMBOL(clk_get); + +void clk_put(struct clk *clk) +{ + if (clk_functions.clk_put) + clk_functions.clk_put(clk); +} +EXPORT_SYMBOL(clk_put); + +int clk_enable(struct clk *clk) +{ + if (clk_functions.clk_enable) + return clk_functions.clk_enable(clk); + return -ENOSYS; +} +EXPORT_SYMBOL(clk_enable); + +void clk_disable(struct clk *clk) +{ + if (clk_functions.clk_disable) + clk_functions.clk_disable(clk); +} +EXPORT_SYMBOL(clk_disable); + +unsigned long clk_get_rate(struct clk *clk) +{ + if (clk_functions.clk_get_rate) + return clk_functions.clk_get_rate(clk); + return 0; +} +EXPORT_SYMBOL(clk_get_rate); + +long clk_round_rate(struct clk *clk, unsigned long rate) +{ + if (clk_functions.clk_round_rate) + return clk_functions.clk_round_rate(clk, rate); + return -ENOSYS; +} +EXPORT_SYMBOL(clk_round_rate); + +int clk_set_rate(struct clk *clk, unsigned long rate) +{ + if (clk_functions.clk_set_rate) + return clk_functions.clk_set_rate(clk, rate); + return -ENOSYS; +} +EXPORT_SYMBOL(clk_set_rate); + +struct clk *clk_get_parent(struct clk *clk) +{ + if (clk_functions.clk_get_parent) + return clk_functions.clk_get_parent(clk); + return ERR_PTR(-ENOSYS); +} +EXPORT_SYMBOL(clk_get_parent); + +int clk_set_parent(struct clk *clk, struct clk *parent) +{ + if (clk_functions.clk_set_parent) + return clk_functions.clk_set_parent(clk, parent); + return -ENOSYS; +} +EXPORT_SYMBOL(clk_set_parent); Index: work-powerpc.git/include/asm-powerpc/clk_interface.h =================================================================== --- /dev/null +++ work-powerpc.git/include/asm-powerpc/clk_interface.h @@ -0,0 +1,20 @@ +#ifndef __ASM_POWERPC_CLK_INTERFACE_H +#define __ASM_POWERPC_CLK_INTERFACE_H + +#include <linux/clk.h> + +struct clk_interface { + struct clk* (*clk_get) (struct device *dev, const char *id); + int (*clk_enable) (struct clk *clk); + void (*clk_disable) (struct clk *clk); + unsigned long (*clk_get_rate) (struct clk *clk); + void (*clk_put) (struct clk *clk); + long (*clk_round_rate) (struct clk *clk, unsigned long rate); + int (*clk_set_rate) (struct clk *clk, unsigned long rate); + int (*clk_set_parent) (struct clk *clk, struct clk *parent); + struct clk* (*clk_get_parent) (struct clk *clk); +}; + +extern struct clk_interface clk_functions; + +#endif /* __ASM_POWERPC_CLK_INTERFACE_H */ Index: work-powerpc.git/arch/powerpc/kernel/Makefile =================================================================== --- work-powerpc.git.orig/arch/powerpc/kernel/Makefile +++ work-powerpc.git/arch/powerpc/kernel/Makefile @@ -12,7 +12,8 @@ endif obj-y := semaphore.o cputable.o ptrace.o syscalls.o \ irq.o align.o signal_32.o pmc.o vdso.o \ - init_task.o process.o systbl.o idle.o + init_task.o process.o systbl.o idle.o \ + clock.o obj-y += vdso32/ obj-$(CONFIG_PPC64) += setup_64.o binfmt_elf32.o sys_ppc32.o \ signal_64.o ptrace32.o \
