Quoting Sebastian Hesselbarth (2012-07-08 10:15:26)
> This patch adds support for using clock gates (clk-gate) from DT based
> on Rob Herrings DT clk binding support for 3.6.
>
> It adds a helper function to clk-gate to allocate all resources required by
> a set of individual clock gates, i.e. register base address and lock. Each
> clock gate is described as a child of the clock-gating-control in DT and
> also created by the helper function.
>
Hi Sebastian,
Thanks for submitting this. I'd prefer for Rob or someone with a more vested
interest in DT to review your binding. I have some comments on the code below.
<snip>
> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> index 578465e..1e88907 100644
> --- a/drivers/clk/clk-gate.c
> +++ b/drivers/clk/clk-gate.c
> @@ -15,6 +15,9 @@
> #include <linux/io.h>
> #include <linux/err.h>
> #include <linux/string.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
>
> /**
> * DOC: basic gatable clock which can gate and ungate it's ouput
> @@ -148,3 +151,84 @@ struct clk *clk_register_gate(struct device *dev, const
> char *name,
>
> return clk;
> }
> +
> +#ifdef CONFIG_OF
> +/**
> + * of_clock_gating_control_setup() - Setup function for clock gate control
> + * This is a helper for using clk-gate from OF device tree. It allocates
> + * a common lock for a base register and creates the individual clk-gates.
> + */
> +void __init of_clock_gating_control_setup(struct device_node *np)
> +{
> + struct device_node *child;
> + const char *pclk_name;
> + void __iomem *base;
> + spinlock_t *lockp;
> + unsigned int rnum;
> + u64 addr;
> +
> + pclk_name = of_clk_get_parent_name(np, 0);
> + if (!pclk_name) {
> + pr_debug("%s: unable to get parent clock for %s\n",
> + __func__, np->full_name);
> + return;
> + }
> +
> + lockp = kzalloc(sizeof(spinlock_t), GFP_KERNEL);
> + if (!lockp) {
> + pr_debug("%s: unable to allocate spinlock for %s\n",
> + __func__, np->full_name);
> + return;
> + }
> +
> + spin_lock_init(lockp);
The spinlocks for the basic clock types have always been optional. This
code should reflect that and not assume the spinlock.
Also I wonder if the assumption is true that a single spinlock
corresponding to a device_node is always the right thing for every
platform. What about a 32-bit register that contains some gating bits
and a 3-bit wide field for a mux which we perform read-modify-write
operations on?
You'll have to pardon my DT ignorance. My concerns above may be totally
crazy with respect to DT.
> + base = of_iomap(np, 0);
> + rnum = sizeof(resource_size_t) * 8;
> + addr = of_translate_address(np, of_get_property(np, "reg", NULL));
> +
> + pr_debug("create clock gate control %s\n", np->full_name);
There are some inconsistent prints here. How about leading this trace
with a __func__ like you do below for the error messages?
> +
> + for_each_child_of_node(np, child) {
> + struct clk *cg;
> + const char *cg_name;
> + const char *cg_pclk_name;
> + u32 propval[2];
> + unsigned int rbit;
> +
> + if (of_property_read_u32_array(child, "reg", propval, 2)) {
> + pr_debug("%s: wrong #reg on %s\n",
> + __func__, child->full_name);
> + continue;
> + }
> +
> + rbit = propval[0];
> + if (rbit >= rnum) {
> + pr_debug("%s: bit position of %s exceeds resources\n",
> + __func__, child->full_name);
> + continue;
> + }
> +
> + cg_pclk_name = of_clk_get_parent_name(child, 0);
> + if (!pclk_name)
> + cg_pclk_name = pclk_name;
!pclk_name would have caused an early return above, so this conditional
will never evaluate as true. Even if it did, I'm not sure I follow the
logic. Why set cg_pclk_name to NULL if pclk_name is NULL?
> +
> + if (of_property_read_string(child, "clock-output-names",
> + &cg_name)) {
> + unsigned int nlen = 4 + 16 + strlen(child->name);
> + char *name = kzalloc(nlen+1, GFP_KERNEL);
> + if (!name)
> + continue;
> + snprintf(name, nlen, "%u@%llx.%s", rbit,
> + (unsigned long long)addr, child->name);
> + cg_name = name;
> + }
> +
> + pr_debug(" create clock gate: %s\n", cg_name);
Extra whitespace typo? Again, would be nice to lead this trace with a
__func__ string.
> +
> + cg = clk_register_gate(NULL, cg_name, cg_pclk_name, 0,
> + base, rbit, propval[1], lockp);
> + if (cg)
> + of_clk_add_provider(child, of_clk_src_simple_get, cg);
Need to check if clk_register_gate fails and do memory leak cleanup of
name and maybe lockp for the corner case where none of the clock
registration operations succeed.
Regards,
Mike
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
