clk: Initial feedback for off-SoC slow bus clocks
Subsystems (4)
cover clk: Initial feedback for off-SoC slow bus clocks
I've just been having a go at implementing generic clk API support for an off-SoC device on a slow bus, the clocking module in the wm831x/2x series of PMICs. Unfortunately as I don't currently have access to a platform that has been converted to use the API I've not actually been able to test the code but I'm reasonably optimistic that the code will Just Work(tm) as the API seems fairly straightforward.
The biggest issue I ran into was that as the clocks are all registered by name with the API if you've got two instances of the same off-SoC device in the system you'll not be able to disambiguate between the clocks it provides. I've added a simple solution for this in the form of a patch which takes a struct device as an argument and adds that to the name of the registered clock. This isn't ideal but should give stable names which systems can use together with clkdev to match clocks up with their users. I believe that when we have device tree bindings for clocks we should be able to use the device to access the bindings and avoid this mangling - Grant, is that correct?
Otherwise everything seems to work pretty well from the driver side for these devices, I've added some minor updates which came up naturally while writing the driver code and seemed fairly obvious. One of these was a change to provide a user visible Kconfig option for building the clock drivers, the idea being to make it easier to do build tests. I did also wonder if it's worth adding a patch to enable the API on x86 (which doesn't currently have a clock API) for coverage.
These patches (which include the two I posted yesterday) are against the series you posted in May with the exception of the wm831x patch which also has an additional dependency on "mfd: Add WM831x clock control register definitions" which is due for merge in the next merge window. If this stuff is all OK for you it would be good if you could include the wm831x driver in your tree for this, especially given that there's nothing in -next.
Mark Brown (6):
clk: Prototype and document clk_register()
clk: Provide a dummy clk_unregister()
clk: Constify struct clk_hw_ops
clk: Add Kconfig option to build all generic clk drivers
clk: Support multiple instances of the same clock provider
clk: Add initial WM831x clock driver
MAINTAINERS | 1 + drivers/clk/Kconfig | 16 ++- drivers/clk/Makefile | 1 + drivers/clk/clk-wm831x.c | 389 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/clk/clk.c | 38 ++++- include/linux/clk.h | 33 ++++ 6 files changed, 472 insertions(+), 6 deletions(-) create mode 100644 drivers/clk/clk-wm831x.c
PATCH 1/6 clk: Prototype and document clk_register()
This allows the compiler to ensure drivers are using the correct prototype.
--- include/linux/clk.h | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
include/linux/clk.h +12 -0
@@ -136,6 +136,18 @@ extern struct clk_hw_ops clk_gate_ops;#endif /* CONFIG_GENERIC_CLK_GATE */ +/** +* clk_register - register and initialize a new clock +* +* @ops: ops for the new clock +* @hw: struct clk_hw to be passed to the ops of the new clock +* @name: name to use for the new clock +* +* Register a new clock with the clk subsytem. Returns either a +* struct clk for the new clock or a NULL pointer. +*/ +struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, +const char *name);#else /* !CONFIG_GENERIC_CLK */
Signature · 1 line
1.7.5.4
PATCH 2/6 clk: Provide a dummy clk_unregister()
Even though unregistration is not actually supported by the clk API it is still useful to provide a clk_unregister() so that drivers can implement their unregistration code. This saves having to go back later and update them once unregistration is possible.
--- include/linux/clk.h | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
include/linux/clk.h +15 -0
@@ -149,6 +149,21 @@ extern struct clk_hw_ops clk_gate_ops;struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw,const char *name); +/** +* clk_unregister - remove a clock +* +* @clk: clock to unregister +* +* Remove a clock from the clk subsystem. This is currently not +* implemented but is provided to allow unregistration code to be +* written in drivers ready for use when an implementation is +* provided. +*/ +static inline int clk_unregister (struct clk *clk) +{ +return -ENOTSUPP ; +} +#else /* !CONFIG_GENERIC_CLK */ /*
Signature · 1 line
1.7.5.4
PATCH 3/6 clk: Constify struct clk_hw_ops
In reply to Mark Brown--- drivers/clk/clk.c | 4 ++-- include/linux/clk.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/clk/clk.c +2 -2
@@ -16,7 +16,7 @@struct clk {const char *name; -struct clk_hw_ops *ops; +const struct clk_hw_ops *ops;struct clk_hw *hw;unsigned int enable_count;unsigned int prepare_count;
@@ -252,7 +252,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent) }EXPORT_SYMBOL_GPL (clk_set_parent); -struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, +struct clk *clk_register (const struct clk_hw_ops *ops,struct clk_hw *hw,const char *name) {struct clk *clk;
include/linux/clk.h +1 -1
@@ -146,7 +146,7 @@ extern struct clk_hw_ops clk_gate_ops;* Register a new clock with the clk subsytem. Returns either a * struct clk for the new clock or a NULL pointer. */ -struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, +struct clk *clk_register (const struct clk_hw_ops *ops,struct clk_hw *hw,const char *name);/**
Signature · 1 line
1.7.5.4
PATCH 4/6 clk: Add Kconfig option to build all generic clk drivers
In reply to Mark BrownCurrently drivers for the generic clk subsystem must be selected by platforms using them in order to enable build. When doing development on the API or generic build time testing it is useful to be able to build unused drivers in order to improve coverage so supply a Kconfig option which allows this.
--- drivers/clk/Kconfig | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-)
drivers/clk/Kconfig +10 -1
@@ -1,4 +1,3 @@ - config CLKDEV_LOOKUP bool select HAVE_CLK
@@ -6,6 +5,16 @@ config CLKDEV_LOOKUP config GENERIC_CLK bool +config GENERIC_CLK_BUILD_TEST + bool "Build all generic clock drivers" + depends on EXPERIMENTAL && GENERIC_CLK + select GENERIC_CLK_FIXED + select GENERIC_CLK_GATE + help + Enable all possible generic clock drivers. This is only + useful for improving build coverage, it is not useful for + production kernel builds. + config GENERIC_CLK_FIXED bool depends on GENERIC_CLK
Signature · 1 line
1.7.5.4
PATCH 5/6 clk: Support multiple instances of the same clock provider
In reply to Mark BrownCurrently the generic clk API identifies clocks internally using the name of the clock. This is OK for on-SoC clocks where we have enough control to disambiguate but doesn't work well for clocks provided on external chips where a system design may include more than one instance of the same chip (the Wolfson Speyside system is an example of this) or may have namespace collisions.
Address this by allowing the clock provider to supply a struct device for the clock for use in disambiguation. As a first pass if it is provided we prefix the clock name with the dev_name() of the device. With a device tree binding for clocks it should be possible to remove this mangling and instead use the struct device to provide access to the binding information.
In order to avoid needless noise in names and memory usage it is strongly recommended that on-SoC clocks do not provide a struct device until the implementation is improved.
--- drivers/clk/clk.c | 36 ++++++++++++++++++++++++++++++++---- include/linux/clk.h | 14 ++++++++++---- 2 files changed, 42 insertions(+), 8 deletions(-)
drivers/clk/clk.c +32 -4
@@ -13,6 +13,7 @@#include <linux/mutex.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/device.h> struct clk {const char *name;
@@ -252,20 +253,44 @@ int clk_set_parent(struct clk *clk, struct clk *parent) }EXPORT_SYMBOL_GPL (clk_set_parent); -struct clk *clk_register (const struct clk_hw_ops *ops,struct clk_hw *hw, -const char *name) +struct clk *clk_register (struct device *dev,const struct clk_hw_ops *ops, +struct clk_hw *hw,const char *name) {struct clk *clk; +char *new_name; +size_t name_len; clk =kzalloc (sizeof (*clk),GFP_KERNEL );if (!clk)return NULL ; - clk->name = name; clk->ops = ops; clk->hw = hw; hw->clk = clk; +/* Since we currently match clock providers on a purely string +* based method add a prefix based on the device name if a +* device is provided. When we have support for device tree +* based clock matching it should be possible to avoid this +* mangling and instead use the struct device to hook into +* the bindings. +* +* As we don't currently support unregistering clocks we don't +* need to worry about cleanup as yet. +*/ +if (dev) { + name_len =strlen (name) +strlen (dev_name (dev)) +2 ; + new_name =kzalloc (name_len,GFP_KERNEL ); +if (!new_name) + goto err; + +snprintf (new_name, name_len,"%s-%s" ,dev_name (dev), name); + + clk->name = new_name; + }else { + clk->name = name; + } +/* Query the hardware for parent and initial rate. We may alter * the clock topology, making this clock available from the parent's * children list. So, we need to protect against concurrent
@@ -285,7 +310,10 @@ struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,mutex_unlock (&prepare_lock); -return clk; + +err: +kfree (clk); +return NULL ; }EXPORT_SYMBOL_GPL (clk_register);
include/linux/clk.h +10 -4
@@ -139,15 +139,21 @@ extern struct clk_hw_ops clk_gate_ops;/** * clk_register - register and initialize a new clock * +* @dev: device providing the clock or NULL * @ops: ops for the new clock * @hw: struct clk_hw to be passed to the ops of the new clock * @name: name to use for the new clock * -* Register a new clock with the clk subsytem. Returns either a -* struct clk for the new clock or a NULL pointer. +* Register a new clock with the clk subsytem. If dev is provided +* then it will be used to disambiguate between multiple instances of +* the same device in the system, typically this should only be done +* for devices that are not part of the core SoC unless device tree is +* in use. +* +* Returns either a struct clk for the new clock or a NULL pointer. */ -struct clk *clk_register (const struct clk_hw_ops *ops,struct clk_hw *hw, -const char *name); +struct clk *clk_register (struct device *dev,const struct clk_hw_ops *ops, +struct clk_hw *hw,const char *name);/** * clk_unregister - remove a clock
Signature · 1 line
1.7.5.4
Re: [PATCH 5/6] clk: Support multiple instances of the same clock provider
Quoted 69 lines · Mark Brown
Currently the generic clk API identifies clocks internally using the name of the clock. This is OK for on-SoC clocks where we have enough control to disambiguate but doesn't work well for clocks provided on external chips where a system design may include more than one instance of the same chip (the Wolfson Speyside system is an example of this) or may have namespace collisions.
Address this by allowing the clock provider to supply a struct device for the clock for use in disambiguation. As a first pass if it is provided we prefix the clock name with the
dev_name()of the device. With a device tree binding for clocks it should be possible to remove this mangling and instead use the struct device to provide access to the binding information.In order to avoid needless noise in names and memory usage it is strongly recommended that on-SoC clocks do not provide a struct device until the implementation is improved.
Signed-off-by Mark Brown <[email protected]>--- drivers/clk/clk.c | 36 ++++++++++++++++++++++++++++++++---- include/linux/clk.h | 14 ++++++++++---- 2 files changed, 42 insertions(+), 8 deletions(-)drivers/clk/clk.c +17
-3@@ -13,6 +13,7 @@#include <linux/mutex.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/device.h> struct clk {const char *name;@@ -252,20 +253,44 @@ int clk_set_parent(struct clk *clk, struct clk *parent) }EXPORT_SYMBOL_GPL (clk_set_parent); -struct clk *clk_register (const struct clk_hw_ops *ops,struct clk_hw *hw, -const char *name) +struct clk *clk_register (struct device *dev,const struct clk_hw_ops *ops, +struct clk_hw *hw,const char *name) {struct clk *clk; +char *new_name; +size_t name_len; clk =kzalloc (sizeof (*clk),GFP_KERNEL );if (!clk)return NULL ; - clk->name = name; clk->ops = ops; clk->hw = hw; hw->clk = clk; +/* Since we currently match clock providers on a purely string +* based method add a prefix based on the device name if a +* device is provided. When we have support for device tree +* based clock matching it should be possible to avoid this +* mangling and instead use the struct device to hook into +* the bindings. +* +* As we don't currently support unregistering clocks we don't +* need to worry about cleanup as yet. +*/ +if (dev) { + name_len =strlen (name) +strlen (dev_name (dev)) +2 ;
drivers/clk/clk.c int clk_set_parent(struct clk *clk, struct clk *parent) · line 292
Quoted 10 lines
drivers/clk/clk.c +10
-0+ new_name =kzalloc (name_len,GFP_KERNEL ); +if (!new_name) + goto err; + +snprintf (new_name, name_len,"%s-%s" ,dev_name (dev), name); + + clk->name = new_name; + }else { + clk->name = name; + }
This "clk consolidation" is really idiotic. The clk matching mechanism should have _nothing_ to do with the rest of the clk API, especially the consolidation effort.
It should not matter whether clkdev is used, or an alternative method to specify this stuff via DT. Keep the clk_get()/clk_put() _separate_ from the consolidation of the rest.
Re: [PATCH 5/6] clk: Support multiple instances of the same clock provider
Quoted 9 lines · Russell King - ARM Linux
drivers/clk/clk.c int clk_set_parent(struct clk *clk, struct clk *parent) · line 273
Quoted 3 lines · Mark Brown
drivers/clk/clk.c +3
-0+/* Since we currently match clock providers on a purely string +* based method add a prefix based on the device name if a +* device is provided. When we have support for device tree This "clk consolidation" is really idiotic. The clk matching mechanism should have _nothing_ to do with the rest of the clk API, especially the consolidation effort.
It's not touching clkdev, the comment is somewhat misleading and is mostly based on me thinking about how we'd deploy off-SoC clocks. There's also the diagnostic issues Sacha mentioned, if we don't keep some source information handy it's hard to tell what clock logging is talking about.
Quoted 3 lines
It should not matter whether clkdev is used, or an alternative method to specify this stuff via DT. Keep the clk_get()/clk_put() _separate_ from the consolidation of the rest.
We do need some way to have some idea which clocks we're talking about, and for off-SoC stuff passing around struct clk pointers is rather painful. At some point some bit of code is going to have to get hold of the actual struct clk and then map it onto the devices using it.
For device tree we should be able to do that fairly painlessly with just the struct devices, without device tree you either have to have the structs handy or use names. At the minute the infrastructure is somewhat lacking here.
Re: [PATCH 5/6] clk: Support multiple instances of the same clock provider
Quoted 4 lines · Mark Brown
We do need some way to have some idea which clocks we're talking about, and for off-SoC stuff passing around struct clk pointers is rather painful. At some point some bit of code is going to have to get hold of the actual struct clk and then map it onto the devices using it.
As I haven't seen any of this "passing around struct clk pointers" crap recently, I can only guess at what you're saying. I thought all that had been solved with _proper_ use of clkdev.
clkdev can provide you with a struct clk for _any_ device in the system where its name is known at build time.
For devices where the name is not known at build time, a new cl_lookup entry can be created at runtime with clkdev_alloc() or clk_add_alias(), and dropped with clkdev_drop().
The problem comes when you aren't able to hook into a subsystem which creates an unstable device name (eg, USB). I suspect that's also a problem for DT too because there has to be some way to go from a struct device to something stable.
Re: [PATCH 5/6] clk: Support multiple instances of the same clock provider
Quoted 9 lines · Russell King - ARM Linux
Quoted 4 lines · Mark Brown
We do need some way to have some idea which clocks we're talking about, and for off-SoC stuff passing around struct clk pointers is rather painful. At some point some bit of code is going to have to get hold of the actual struct clk and then map it onto the devices using it.
As I haven't seen any of this "passing around struct clk pointers" crap recently, I can only guess at what you're saying. I thought all that had been solved with _proper_ use of clkdev.
The problem is getting the data into clkdev in the first place for the off-SoC devices.
Quoted 2 lines
clkdev can provide you with a struct clk for _any_ device in the system where its name is known at build time.
Right, on the consumer side it's all sorted and works really well. The trick is on the provider side where you need to have the clock available to be able to add the mappings for it.
Perhaps I'm just missing something about how this should all work - actually, the regulator solution probably works fine here. Pass an array of mappings into either the driver or the clock subsystem and have them add the mappings when the provider registers. Whichever way around should just be some hooks in the new subsystem.
PATCH 6/6 clk: Add initial WM831x clock driver
In reply to Mark BrownThe WM831x and WM832x series of PMICs contain a flexible clocking subsystem intended to provide always on and system core clocks. It features:
- A 32.768kHz crystal oscillator which can optionally be used to pass through an externally generated clock.
- A FLL which can be clocked from either the 32.768kHz oscillator or the CLKIN pin.
- A CLKOUT pin which can bring out either the oscillator or the FLL output.
- The 32.768kHz clock can also optionally be brought out on the GPIO pins of the device.
This driver fully supports the 32.768kHz oscillator and CLKOUT. The FLL is supported only in AUTO mode, the full flexibility of the FLL cannot currently be used. The use of clock references other than the internal oscillator is not currently supported, and since clk_set_parent() is not implemented in the generic clock API the clock tree configuration cannot be changed at runtime.
Due to a lack of access to systems where the core SoC has been converted to use the generic clock API this driver has been compile tested only.
--- MAINTAINERS | 1 + drivers/clk/Kconfig | 5 + drivers/clk/Makefile | 1 + drivers/clk/clk-wm831x.c | 389 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 396 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-wm831x.c
MAINTAINERS +1 -0
@@ -6969,6 +6969,7 @@ T: git git://opensource.wolfsonmicro.com/linux-2.6-audioplus W: http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-devices S: Supported F: Documentation/hwmon/wm83?? +F: drivers/clk/clk-wm83*.c F: drivers/leds/leds-wm83*.c F: drivers/mfd/wm8*.c F: drivers/power/wm83*.c
drivers/clk/Kconfig +5 -0
@@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST depends on EXPERIMENTAL && GENERIC_CLK select GENERIC_CLK_FIXED select GENERIC_CLK_GATE + select GENERIC_CLK_WM831X if MFD_WM831X=y help Enable all possible generic clock drivers. This is only useful for improving build coverage, it is not useful for
@@ -22,3 +23,7 @@ config GENERIC_CLK_FIXED config GENERIC_CLK_GATE bool depends on GENERIC_CLK + +config GENERIC_CLK_WM831X + tristate + depends on GENERIC_CLK && MFD_WM831X
drivers/clk/Makefile +1 -0
@@ -3,3 +3,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o +obj-$(CONFIG_GENERIC_CLK_WM831X) += clk-wm831x.o
drivers/clk/clk-wm831x.c new file +389 -0
389 lines not on this page · raw message
Signature · 1 line
1.7.5.4
Re: [PATCH 6/6] clk: Add initial WM831x clock driver
Quoted 20 lines · Mark Brown
The WM831x and WM832x series of PMICs contain a flexible clocking subsystem intended to provide always on and system core clocks. It features:
- A 32.768kHz crystal oscillator which can optionally be used to pass through an externally generated clock.
- A FLL which can be clocked from either the 32.768kHz oscillator or the CLKIN pin.
- A CLKOUT pin which can bring out either the oscillator or the FLL output.
- The 32.768kHz clock can also optionally be brought out on the GPIO pins of the device.This driver fully supports the 32.768kHz oscillator and CLKOUT. The FLL is supported only in AUTO mode, the full flexibility of the FLL cannot currently be used. The use of clock references other than the internal oscillator is not currently supported, and since
clk_set_parent()is not implemented in the generic clock API the clock tree configuration cannot be changed at runtime.
Quoted 2 lines
Due to a lack of access to systems where the core SoC has been converted to use the generic clock API this driver has been compile tested only.
Generally seems okay. Minor comments below.
g.
Quoted 27 lines
Signed-off-by Mark Brown <[email protected]>--- MAINTAINERS | 1 + drivers/clk/Kconfig | 5 + drivers/clk/Makefile | 1 + drivers/clk/clk-wm831x.c | 389 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 396 insertions(+), 0 deletions(-) create mode 100644 drivers/clk/clk-wm831x.cMAINTAINERS +1
-0@@ -6969,6 +6969,7 @@ T: git git://opensource.wolfsonmicro.com/linux-2.6-audioplus W: http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-devices S: Supported F: Documentation/hwmon/wm83?? +F: drivers/clk/clk-wm83*.c F: drivers/leds/leds-wm83*.c F: drivers/mfd/wm8*.c F: drivers/power/wm83*.cdrivers/clk/Kconfig +0
-0
drivers/clk/Kconfig config GENERIC_CLK_BUILD_TEST · line 13
Quoted 4 lines
drivers/clk/Kconfig +1
-0@@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST depends on EXPERIMENTAL && GENERIC_CLK select GENERIC_CLK_FIXED select GENERIC_CLK_GATE + select GENERIC_CLK_WM831X if MFD_WM831X=y
Hmmm, this could get unwieldy in a hurry.
Quoted 48 lines
drivers/clk/Kconfig +4
-0help Enable all possible generic clock drivers. This is only useful for improving build coverage, it is not useful for@@ -22,3 +23,7 @@ config GENERIC_CLK_FIXED config GENERIC_CLK_GATE bool depends on GENERIC_CLK + +config GENERIC_CLK_WM831X + tristate + depends on GENERIC_CLK && MFD_WM831Xdrivers/clk/Makefile +1
-0@@ -3,3 +3,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP) += clkdev.o obj-$(CONFIG_GENERIC_CLK) += clk.o obj-$(CONFIG_GENERIC_CLK_FIXED) += clk-fixed.o obj-$(CONFIG_GENERIC_CLK_GATE) += clk-gate.o +obj-$(CONFIG_GENERIC_CLK_WM831X) += clk-wm831x.odrivers/clk/clk-wm831x.c new file +22
-0@@ -0,0 +1,389 @@ +/* +* WM831x clock control +* +* Copyright 2011 Wolfson Microelectronics PLC. +* +* Author: Mark Brown <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License as published by the +* Free Software Foundation; either version 2 of the License, or (at your +* option) any later version. +* +*/ + +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/platform_device.h> +#include <linux/mfd/wm831x/core.h> + +struct wm831x_clk { +struct wm831x *wm831x;
drivers/clk/clk-wm831x.c line 32
Quoted 10 lines
drivers/clk/clk-wm831x.c new file +10
-0+struct clk_hw xtal_hw; +struct clk_hw fll_hw; +struct clk_hw clkout_hw; +bool xtal_ena; +}; + +static int wm831x_xtal_enable (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + xtal_hw);
This container of is used 10 times. A static inline would be reasonable.
drivers/clk/clk-wm831x.c line 38
Quoted 6 lines
drivers/clk/clk-wm831x.c new file +6
-0+ +if (clkdata->xtal_ena) +return 0 ; +else +return -EPERM ; +}
Nit: return clkdata->xtal_ena ? 0 : -EPERM;
Just makes for more concise code.
Quoted 39 lines
drivers/clk/clk-wm831x.c new file +39
-0+ +static unsigned long wm831x_xtal_recalc_rate (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + xtal_hw); + +if (clkdata->xtal_ena) +return 32768 ; +else +return 0 ; +} + +static long wm831x_xtal_round_rate (struct clk_hw *hw,unsigned long rate) +{ +return wm831x_xtal_recalc_rate (hw); +} + +static const struct clk_hw_ops wm831x_xtal_ops = { + .enable = wm831x_xtal_enable, + .recalc_rate = wm831x_xtal_recalc_rate, + .round_rate = wm831x_xtal_round_rate, +}; + +static const unsigned long wm831x_fll_auto_rates[] = { +2048000 , +11289600 , +12000000 , +12288000 , +19200000 , +22579600 , +24000000 , +24576000 , +}; + +static bool wm831x_fll_enabled (struct wm831x *wm831x) +{ +int ret; + + ret =wm831x_reg_read (wm831x,WM831X_FLL_CONTROL_1 );
drivers/clk/clk-wm831x.c line 87
Quoted 10 lines
drivers/clk/clk-wm831x.c new file +10
-0+if (ret <0 ) { +dev_err (wm831x->dev,"Unable to read FLL_CONTROL_1: %d\n" , + ret); +return true; + } + +if (ret &WM831X_FLL_ENA ) +return true; +else +return false;
similarly, return (ret & WM831X_FLL_ENA) != 0;
Quoted 234 lines
drivers/clk/clk-wm831x.c new file +234
-0+} + +static int wm831x_fll_prepare (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); +struct wm831x *wm831x = clkdata->wm831x; +int ret; + + ret =wm831x_set_bits (wm831x,WM831X_FLL_CONTROL_2 , +WM831X_FLL_ENA ,WM831X_FLL_ENA ); +if (ret !=0 ) +dev_crit (wm831x->dev,"Failed to enable FLL: %d\n" , ret); + +return ret; +} + +static void wm831x_fll_unprepare (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); +struct wm831x *wm831x = clkdata->wm831x; +int ret; + + ret =wm831x_set_bits (wm831x,WM831X_FLL_CONTROL_2 ,WM831X_FLL_ENA ,0 ); +if (ret !=0 ) +dev_crit (wm831x->dev,"Failed to disaable FLL: %d\n" , ret); +} + +static unsigned long wm831x_fll_recalc_rate (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); +struct wm831x *wm831x = clkdata->wm831x; +int ret; + + ret =wm831x_reg_read (wm831x,WM831X_CLOCK_CONTROL_2 ); +if (ret <0 ) { +dev_err (wm831x->dev,"Unable to read CLOCK_CONTROL_2: %d\n" , + ret); +return 0 ; + } + +if (ret &WM831X_FLL_AUTO ) +return wm831x_fll_auto_rates[ret &WM831X_FLL_AUTO_FREQ_MASK ]; + +dev_err (wm831x->dev,"FLL only supported in AUTO mode\n" ); +return 0 ; +} + +static int wm831x_fll_set_rate (struct clk_hw *hw,unsigned long rate, +unsigned long *parent_rate) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); +struct wm831x *wm831x = clkdata->wm831x; +int i; + +for (i =0 ; i <ARRAY_SIZE (wm831x_fll_auto_rates); i++) +if (wm831x_fll_auto_rates[i] == rate) +break ; +if (i ==ARRAY_SIZE (wm831x_fll_auto_rates)) +return -EINVAL ; + +if (wm831x_fll_enabled (wm831x)) +return -EPERM ; + +return wm831x_set_bits (wm831x,WM831X_CLOCK_CONTROL_2 , +WM831X_FLL_AUTO_FREQ_MASK , i); +} + +static struct clk *wm831x_fll_get_parent (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + fll_hw); +struct wm831x *wm831x = clkdata->wm831x; +int ret; + +/* AUTO mode is always clocked from the crystal */ + ret =wm831x_reg_read (wm831x,WM831X_CLOCK_CONTROL_2 ); +if (ret <0 ) { +dev_err (wm831x->dev,"Unable to read CLOCK_CONTROL_2: %d\n" , + ret); +return NULL ; + } + +if (ret &WM831X_FLL_AUTO ) +return clkdata->xtal_hw.clk; + + ret =wm831x_reg_read (wm831x,WM831X_FLL_CONTROL_5 ); +if (ret <0 ) { +dev_err (wm831x->dev,"Unable to read FLL_CONTROL_5: %d\n" , + ret); +return NULL ; + } + +switch (ret &WM831X_FLL_CLK_SRC_MASK ) { +case 0 : +return clkdata->xtal_hw.clk; +case 1 : +dev_warn (wm831x->dev, +"FLL clocked from CLKIN not yet supported\n" ); +return NULL ; +default : +dev_err (wm831x->dev,"Unsupported FLL clock source %d\n" , + ret &WM831X_FLL_CLK_SRC_MASK ); +return NULL ; + } +} + +static const struct clk_hw_ops wm831x_fll_ops = { + .prepare = wm831x_fll_prepare, + .unprepare = wm831x_fll_unprepare, + .recalc_rate = wm831x_fll_recalc_rate, + .set_rate = wm831x_fll_set_rate, + .get_parent = wm831x_fll_get_parent, +}; + +static int wm831x_clkout_prepare (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + clkout_hw); +struct wm831x *wm831x = clkdata->wm831x; +int ret; + + ret =wm831x_reg_unlock (wm831x); +if (ret !=0 ) { +dev_crit (wm831x->dev,"Failed to lock registers: %d\n" , ret); +return ret; + } + + ret =wm831x_set_bits (wm831x,WM831X_CLOCK_CONTROL_1 , +WM831X_CLKOUT_ENA ,WM831X_CLKOUT_ENA ); +if (ret !=0 ) +dev_crit (wm831x->dev,"Failed to enable CLKOUT: %d\n" , ret); + +wm831x_reg_lock (wm831x); + +return ret; +} + +static void wm831x_clkout_unprepare (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + clkout_hw); +struct wm831x *wm831x = clkdata->wm831x; +int ret; + + ret =wm831x_reg_unlock (wm831x); +if (ret !=0 ) { +dev_crit (wm831x->dev,"Failed to lock registers: %d\n" , ret); +return ; + } + + ret =wm831x_set_bits (wm831x,WM831X_CLOCK_CONTROL_1 , +WM831X_CLKOUT_ENA ,0 ); +if (ret !=0 ) +dev_crit (wm831x->dev,"Failed to disable CLKOUT: %d\n" , ret); + +wm831x_reg_lock (wm831x); +} + +static unsigned long wm831x_clkout_recalc_rate (struct clk_hw *hw) +{ +return clk_get_rate (clk_get_parent (hw->clk)); +} + +static long wm831x_clkout_round_rate (struct clk_hw *hw,unsigned long rate) +{ +return clk_round_rate (clk_get_parent (hw->clk), rate); +} + +static int wm831x_clkout_set_rate (struct clk_hw *hw,unsigned long rate, +unsigned long *parent_rate) +{ + *parent_rate = rate; +return CLK_SET_RATE_PROPAGATE ; +} + +static struct clk *wm831x_clkout_get_parent (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + clkout_hw); +struct wm831x *wm831x = clkdata->wm831x; +int ret; + + ret =wm831x_reg_read (wm831x,WM831X_CLOCK_CONTROL_1 ); +if (ret <0 ) { +dev_err (wm831x->dev,"Unable to read CLOCK_CONTROL_1: %d\n" , + ret); +return NULL ; + } + +if (ret &WM831X_CLKOUT_SRC ) +return clkdata->xtal_hw.clk; +else +return clkdata->fll_hw.clk; +} + +static const struct clk_hw_ops wm831x_clkout_ops = { + .prepare = wm831x_clkout_prepare, + .unprepare = wm831x_clkout_unprepare, + .recalc_rate = wm831x_clkout_recalc_rate, + .round_rate = wm831x_clkout_round_rate, + .set_rate = wm831x_clkout_set_rate, + .get_parent = wm831x_clkout_get_parent, +}; + +static __devinit int wm831x_clk_probe(struct platform_device *pdev) +{ + struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent); + struct wm831x_clk *clkdata; + int ret; + + clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL); + if (!clkdata) + return -ENOMEM; + + /* XTAL_ENA can only be set via OTP/InstantConfig so just read once */ + ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2); + if (ret < 0) { + dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n", + ret); + goto err_alloc; + } + clkdata->xtal_ena = ret & WM831X_XTAL_ENA; + + if (!clk_register(wm831x->dev, &wm831x_xtal_ops, &clkdata->xtal_hw, + "xtal")) { + ret = -EINVAL; + goto err_alloc; + } + + if (!clk_register(wm831x->dev, &wm831x_fll_ops, &clkdata->fll_hw,
drivers/clk/clk-wm831x.c line 331
Quoted 10 lines
drivers/clk/clk-wm831x.c new file +10
-0+ "fll")) { + ret = -EINVAL ; + goto err_xtal; + } + +if (!clk_register (wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw, +"clkout" )) { + ret = -EINVAL ; + goto err_fll; + }
How common will this pattern be? Is there need for a clk_register_many() variant?
Quoted 35 lines
drivers/clk/clk-wm831x.c new file +35
-0+ +dev_set_drvdata (&pdev->dev, clkdata); + +return 0 ; + +err_fll: +clk_unregister (clkdata->fll_hw.clk); +err_xtal: +clk_unregister (clkdata->xtal_hw.clk); +err_alloc: +kfree (clkdata); +return ret; +} + +static __devexit int wm831x_clk_remove(struct platform_device *pdev) +{ +struct wm831x_clk *clkdata =dev_get_drvdata (&pdev->dev); + +clk_unregister (clkdata->clkout_hw.clk); +clk_unregister (clkdata->fll_hw.clk); +clk_unregister (clkdata->xtal_hw.clk); +kfree (clkdata); + +return 0 ; +} + +static struct platform_driver wm831x_clk_driver = { + .probe = wm831x_clk_probe, + .remove =__devexit_p (wm831x_clk_remove), + .driver = { + .name ="wm831x-clk" , + .owner =THIS_MODULE , + }, +}; +
drivers/clk/clk-wm831x.c line 376
Quoted 10 lines
drivers/clk/clk-wm831x.c new file +10
-0+static int __init wm831x_clk_init (void ) +{ +int ret; + + ret =platform_driver_register (&wm831x_clk_driver); +if (ret !=0 ) +pr_err ("Failed to register WM831x clock driver: %d\n" , ret); + +return ret; +}
Driver registration is very well implemented and debugged. Just do "return platform_driver_register();"
Quoted 16 lines
drivers/clk/clk-wm831x.c new file +13
-0+module_init (wm831x_clk_init); + +static void __exit wm831x_clk_exit(void) +{ +platform_driver_unregister (&wm831x_clk_driver); +} +module_exit (wm831x_clk_exit); + +/* Module information */ +MODULE_AUTHOR ("Mark Brown <[email protected]>" ); +MODULE_DESCRIPTION ("WM831x clock driver" ); +MODULE_LICENSE ("GPL" ); +MODULE_ALIAS ("platform:wm831x-clk" );Signature · 1 line
1.7.5.4
Re: [PATCH 6/6] clk: Add initial WM831x clock driver
Quoted 9 lines · Grant Likely
drivers/clk/Kconfig config GENERIC_CLK_BUILD_TEST · line 13
Quoted 5 lines · Mark Brown
drivers/clk/Kconfig +1
-0@@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST depends on EXPERIMENTAL && GENERIC_CLK select GENERIC_CLK_FIXED select GENERIC_CLK_GATE + select GENERIC_CLK_WM831X if MFD_WM831X=yHmmm, this could get unwieldy in a hurry.
It's not really any hassle, we've got a one of these in ASoC. The list gets long but if you keep it sorted it's not an issue for merges and otherwise it's just long not complicated. The ability to get build coverage is *really* useful.
Quoted 7 lines
drivers/clk/clk-wm831x.c line 32
Quoted 4 lines
drivers/clk/clk-wm831x.c new file +4
-0+static int wm831x_xtal_enable (struct clk_hw *hw) +{ + struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk, + xtal_hw);This container of is used 10 times. A static inline would be reasonable.
Not quite - it's used in three different variants (one for each of the clocks).
Quoted 9 lines
drivers/clk/clk-wm831x.c line 38
Quoted 5 lines
drivers/clk/clk-wm831x.c new file +5
-0+if (clkdata->xtal_ena) +return 0 ; +else +return -EPERM ; +}Nit: return
clkdata->xtal_ena? 0 :-EPERM;Just makes for more concise code.
I have an extremely strong dislike of the ternery operator, I find it does nothing for legibility.
Quoted 8 lines
drivers/clk/clk-wm831x.c line 331
Quoted 5 lines
drivers/clk/clk-wm831x.c new file +5
-0+if (!clk_register (wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw, +"clkout" )) { + ret = -EINVAL ; + goto err_fll; + }How common will this pattern be? Is there need for a
clk_register_many()variant?
I dunno, I think a lot of the SoCs may be doing one clk per device. But equally well it's not like it'd be hard if someone starts working on the API again.
Re: [PATCH 6/6] clk: Add initial WM831x clock driver
Quoted 9 lines · Mark Brown
Quoted 8 lines · Grant Likely
drivers/clk/clk-wm831x.c line 38
Quoted 5 lines · Mark Brown
drivers/clk/clk-wm831x.c new file +5
-0+if (clkdata->xtal_ena) +return 0 ; +else +return -EPERM ; +}Nit: return
clkdata->xtal_ena? 0 :-EPERM;
Just makes for more concise code.
Quoted 3 lines
I have an extremely strong dislike of the ternery operator, I find it does nothing for legibility.
I prefer this:
if (!clkdata->xtal_ena)
return -EPERM;
return 0;
}Which makes the error check clear and makes the success case visible right at the bottom of the function.
~Ryan
Re: [PATCH 1/6] clk: Prototype and document clk_register()
In reply to Mark BrownQuoted 3 lines · Mark Brown
This allows the compiler to ensure drivers are using the correct prototype.
Signed-off-by Mark Brown <[email protected]>
Quoted 30 lines
--- include/linux/clk.h | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)include/linux/clk.h +12
-0@@ -136,6 +136,18 @@ extern struct clk_hw_ops clk_gate_ops;#endif /* CONFIG_GENERIC_CLK_GATE */ +/** +* clk_register - register and initialize a new clock +* +* @ops: ops for the new clock +* @hw: struct clk_hw to be passed to the ops of the new clock +* @name: name to use for the new clock +* +* Register a new clock with the clk subsytem. Returns either a +* struct clk for the new clock or a NULL pointer. +*/ +struct clk *clk_register (struct clk_hw_ops *ops,struct clk_hw *hw, +const char *name);#else /* !CONFIG_GENERIC_CLK */ Signature · 1 line
1.7.5.4
On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
Linus, CCing you in as apparently you're taking over the clock API work. Do you need me to forward all the patches to you?
Quoted 50 lines
I've just been having a go at implementing generic clk API support for an off-SoC device on a slow bus, the clocking module in the wm831x/2x series of PMICs. Unfortunately as I don't currently have access to a platform that has been converted to use the API I've not actually been able to test the code but I'm reasonably optimistic that the code will Just Work(tm) as the API seems fairly straightforward.
The biggest issue I ran into was that as the clocks are all registered by name with the API if you've got two instances of the same off-SoC device in the system you'll not be able to disambiguate between the clocks it provides. I've added a simple solution for this in the form of a patch which takes a struct device as an argument and adds that to the name of the registered clock. This isn't ideal but should give stable names which systems can use together with clkdev to match clocks up with their users. I believe that when we have device tree bindings for clocks we should be able to use the device to access the bindings and avoid this mangling - Grant, is that correct?
Otherwise everything seems to work pretty well from the driver side for these devices, I've added some minor updates which came up naturally while writing the driver code and seemed fairly obvious. One of these was a change to provide a user visible
Kconfigoption for building the clock drivers, the idea being to make it easier to do build tests. I did also wonder if it's worth adding a patch to enable the API on x86 (which doesn't currently have a clock API) for coverage.These patches (which include the two I posted yesterday) are against the series you posted in May with the exception of the wm831x patch which also has an additional dependency on "mfd: Add WM831x clock control register definitions" which is due for merge in the next merge window. If this stuff is all OK for you it would be good if you could include the wm831x driver in your tree for this, especially given that there's nothing in -next.
Mark Brown (6):
clk: Prototype and documentclk_register()
clk: Provide a dummyclk_unregister()
clk: Constify structclk_hw_ops
clk: AddKconfigoption to build all generic clk drivers
clk: Support multiple instances of the same clock provider
clk: Add initial WM831x clock driverMAINTAINERS | 1 + drivers/clk/Kconfig | 16 ++- drivers/clk/Makefile | 1 + drivers/clk/clk-wm831x.c | 389 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/clk/clk.c | 38 ++++- include/linux/clk.h | 33 ++++ 6 files changed, 472 insertions(+), 6 deletions(-) create mode 100644 drivers/clk/clk-wm831x.c
Quoted 4 lines · On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
Linus, CCing you in as apparently you're taking over the clock API work. Do you need me to forward all the patches to you?
along these lines, i dont think the new people on the cc noticed my earlier e-
mail:
for future series, could you cc uclinux-dist-devel at blackfin.uclinux.org ? we
dont do clock management on Blackfin parts atm, but it's something we would
like to start doing. our hardware can easily benefit from this.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110711/69b913bf/attachment.sig>
2011/7/11 Mike Frysinger <[email protected]>:
Quoted 11 lines · On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
Quoted 4 lines · Mark Brown
Linus, CCing you in as apparently you're taking over the clock API work. Do you need me to forward all the patches to you?
along these lines, i dont think the new people on the cc noticed my earlier e- mail:
for future series, could you cc uclinux-dist-devel at blackfin.uclinux.org ? ?we dont do clock management on Blackfin parts atm, but it's something we would like to start doing. ?our hardware can easily benefit from this.
Except that, AFAIK, arch/blackfin/mach-xxx is much similar with arch/arm/mach-xxx with a lot of platform, i2c, spi board information. it is probably we can also benefit from device tree as what ARM is doing.
but not sure whether it is really worthwhile for the current situation and long term plan of blackfin. Anyway, there are not so many blackfin SoC and boards as ARM.
Quoted 8 lines
-mike
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 12 lines · Barry Song
2011/7/11 Mike Frysinger:
Quoted 10 lines · On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
Quoted 3 lines · Mark Brown
Linus, CCing you in as apparently you're taking over the clock API work. Do you need me to forward all the patches to you?
along these lines, i dont think the new people on the cc noticed my earlier e- mail:
for future series, could you cc uclinux-dist-devel at blackfin.uclinux.org ? ?we dont do clock management on Blackfin parts atm, but it's something we would like to start doing. ?our hardware can easily benefit from this.
Quoted 4 lines
Except that, AFAIK,
arch/blackfin/mach-xxxis much similar witharch/arm/mach-xxxwith a lot of platform, i2c, spi board information. it is probably we can also benefit from device tree as what ARM is doing.
i dont think device trees are a requirement for the clock api
Quoted 3 lines
but not sure whether it is really worthwhile for the current situation and long term plan of blackfin. Anyway, there are not so many blackfin SoC and boards as ARM.
there are not plans for device tree support. no customers have asked for it, and we arent in the arm situation where we have a distro (like Ubuntu) riding us to have a single build boot on all the different platforms.
-mike
Quoted 4 lines · Mark Brown
The biggest issue I ran into was that as the clocks are all registered by name with the API if you've got two instances of the same off-SoC device in the system you'll not be able to disambiguate between the clocks it provides.
Sigh. That sounds like yet more trash. Obviously whoever thought up that doesn't actually understand clks.
Quoted 8 lines · Russell King - ARM Linux
Quoted 4 lines · Mark Brown
The biggest issue I ran into was that as the clocks are all registered by name with the API if you've got two instances of the same off-SoC device in the system you'll not be able to disambiguate between the clocks it provides.
Sigh. That sounds like yet more trash. Obviously whoever thought up that doesn't actually understand clks.
Nope. In the patches Jeremy posted clocks have a name, but this name is not meant to be used with clk_get. clk_get is still implemented in clkdev, so the matching between clocks and devices is independent of the clock name.
In earlier versions of Jeremys patches the clock name was only present when debugfs was compiled in and I think it can be changed back to this.
That said the debugfs support (which is not present in Jeremys latest series) would break if two clocks with the same name have the same parent, because the clock core would try to create to debugfs entries with the same name.
Sascha
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Quoted 15 lines · Sascha Hauer
Quoted 8 lines · Russell King - ARM Linux
Quoted 4 lines · Mark Brown
The biggest issue I ran into was that as the clocks are all registered by name with the API if you've got two instances of the same off-SoC device in the system you'll not be able to disambiguate between the clocks it provides.
Sigh. That sounds like yet more trash. Obviously whoever thought up that doesn't actually understand clks.
Nope. In the patches Jeremy posted clocks have a name, but this name is not meant to be used with
clk_get.clk_getis still implemented in clkdev, so the matching between clocks and devices is independent of the clock name.
Quoted 7 lines
In earlier versions of Jeremys patches the clock name was only present when debugfs was compiled in and I think it can be changed back to this.
That said the debugfs support (which is not present in Jeremys latest series) would break if two clocks with the same name have the same parent, because the clock core would try to create to debugfs entries with the same name.
If that's all its for, then can't some other solution for debugfs names (lets stop calling them clock names to avoid confusion) be found - such as
sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);
Or how about using something like this:
$debugfs/clkdev/device/connection -> ../../clk/<name-of-clk> $debugfs/clk/<name-of-clk>
where <name-of-clk> could just be the address of the struct clk (which eliminates the need to pass any names in) or some prefix plus an incrementing identifier, or just an incrementing number.
I did think about introducing such a scheme along with clkdev (and did have some code but that's long gone), but without the $debugfs/clk/ bit being standardized, it wouldn't have worked.
Quoted 24 lines · Russell King - ARM Linux
Quoted 15 lines · Sascha Hauer
Quoted 8 lines · Russell King - ARM Linux
Quoted 4 lines · Mark Brown
The biggest issue I ran into was that as the clocks are all registered by name with the API if you've got two instances of the same off-SoC device in the system you'll not be able to disambiguate between the clocks it provides.
Sigh. That sounds like yet more trash. Obviously whoever thought up that doesn't actually understand clks.
Nope. In the patches Jeremy posted clocks have a name, but this name is not meant to be used with
clk_get.clk_getis still implemented in clkdev, so the matching between clocks and devices is independent of the clock name.Quoted 7 lines
In earlier versions of Jeremys patches the clock name was only present when debugfs was compiled in and I think it can be changed back to this.
That said the debugfs support (which is not present in Jeremys latest series) would break if two clocks with the same name have the same parent, because the clock core would try to create to debugfs entries with the same name.
Quoted 6 lines
If that's all its for, then can't some other solution for debugfs names (lets stop calling them clock names to avoid confusion) be found - such as
sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);
Sounds good. During my tests on i.MX I found it very convenient to have speaking debugfs names, so I vote for this suggestion instead the following ones.
Quoted 18 lines
Or how about using something like this:
$debugfs/clkdev/device/connection -> ../../clk/<name-of-clk> $debugfs/clk/<name-of-clk>
where <name-of-clk> could just be the address of the struct clk (which eliminates the need to pass any names in) or some prefix plus an incrementing identifier, or just an incrementing number.
I did think about introducing such a scheme along with clkdev (and did have some code but that's long gone), but without the $debugfs/clk/ bit being standardized, it wouldn't have worked.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Quoted 11 lines · Sascha Hauer
Quoted 5 lines · Russell King - ARM Linux
If that's all its for, then can't some other solution for debugfs names (lets stop calling them clock names to avoid confusion) be found - such as
sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);Sounds good. During my tests on i.MX I found it very convenient to have speaking debugfs names, so I vote for this suggestion instead the following ones.
FWIW that's pretty much exactly what I added except I used a - instead of @.