Re: [PATCH v4 3/5] [RFC] clk: shmobile: div6: Extract cpg_div6_register()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Geert,

Thank you for the patch.

On Friday 16 October 2015 14:49:18 Geert Uytterhoeven wrote:
> Extract cpg_div6_register(), to allow registering div6 clocks from
> another clock driver.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v4:
>   - New.
> ---
>  drivers/clk/shmobile/clk-div6.c | 119 +++++++++++++++++++++++-------------
>  drivers/clk/shmobile/clk-div6.h |   3 +
>  2 files changed, 77 insertions(+), 45 deletions(-)
>  create mode 100644 drivers/clk/shmobile/clk-div6.h
> 
> diff --git a/drivers/clk/shmobile/clk-div6.c
> b/drivers/clk/shmobile/clk-div6.c index 57016ff9c585fc6e..5e8525fc60427229
> 100644
> --- a/drivers/clk/shmobile/clk-div6.c
> +++ b/drivers/clk/shmobile/clk-div6.c
> @@ -18,6 +18,8 @@
>  #include <linux/of_address.h>
>  #include <linux/slab.h>
> 
> +#include "clk-div6.h"
> +
>  #define CPG_DIV6_CKSTP		BIT(8)
>  #define CPG_DIV6_DIV(d)		((d) & 0x3f)
>  #define CPG_DIV6_DIV_MASK	0x3f
> @@ -172,60 +174,34 @@ static const struct clk_ops cpg_div6_clock_ops = {
>  	.set_rate = cpg_div6_clock_set_rate,
>  };

Non-static functions deserve a bit of documentation :-)

> -static void __init cpg_div6_clock_init(struct device_node *np)
> +struct clk * __init cpg_div6_register(const char *name,
> +				      unsigned int num_parents,
> +				      const char **parent_names,
> +				      void __iomem *reg)
>  {
> -	unsigned int num_parents, valid_parents;
> -	const char **parent_names;
> +	unsigned int valid_parents;
>  	struct clk_init_data init;
>  	struct div6_clock *clock;
> -	const char *clk_name = np->name;
>  	struct clk *clk;
>  	unsigned int i;
> 
>  	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
>  	if (!clock)
> -		return;
> -
> -	num_parents = of_clk_get_parent_count(np);
> -	if (num_parents < 1) {
> -		pr_err("%s: no parent found for %s DIV6 clock\n",
> -		       __func__, np->name);
> -		return;
> -	}
> +		return ERR_PTR(-ENOMEM);
> 
>  	clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents),
> -		GFP_KERNEL);
> -	parent_names = kmalloc_array(num_parents, sizeof(*parent_names),
> -				GFP_KERNEL);
> -	if (!parent_names)
> -		return;
> +				       GFP_KERNEL);
> +	if (!clock->parents)
> +		return ERR_PTR(-ENOMEM);
> 
> -	/* Remap the clock register and read the divisor. Disabling the
> -	 * clock overwrites the divisor, so we need to cache its value for the
> -	 * enable operation.
> -	 */
> -	clock->reg = of_iomap(np, 0);
> -	if (clock->reg = NULL) {
> -		pr_err("%s: failed to map %s DIV6 clock register\n",
> -		       __func__, np->name);
> -		goto error;
> -	}
> +	clock->reg = reg;
> 
> +	/*
> +	 * Read the divisor. Disabling the clock overwrites the divisor, so we
> +	 * need to cache its value for the enable operation.
> +	 */
>  	clock->div = (clk_readl(clock->reg) & CPG_DIV6_DIV_MASK) + 1;
> 
> -	/* Parse the DT properties. */
> -	of_property_read_string(np, "clock-output-names", &clk_name);
> -
> -	for (i = 0, valid_parents = 0; i < num_parents; i++) {
> -		const char *name = of_clk_get_parent_name(np, i);
> -
> -		if (name) {
> -			parent_names[valid_parents] = name;
> -			clock->parents[valid_parents] = i;
> -			valid_parents++;
> -		}
> -	}
> -
>  	switch (num_parents) {
>  	case 1:
>  		/* fixed parent clock */
> @@ -243,12 +219,22 @@ static void __init cpg_div6_clock_init(struct
> device_node *np) break;
>  	default:
>  		pr_err("%s: invalid number of parents for DIV6 clock %s\n",
> -		       __func__, np->name);
> +		       __func__, name);
> +		clk = ERR_PTR(-EINVAL);
>  		goto error;
>  	}
> 
> +	/* Filter out invalid parents */
> +	for (i = 0, valid_parents = 0; i < num_parents; i++) {
> +		if (parent_names[i]) {
> +			parent_names[valid_parents] = parent_names[i];
> +			clock->parents[valid_parents] = i;
> +			valid_parents++;
> +		}
> +	}
> +
>  	/* Register the clock. */
> -	init.name = clk_name;
> +	init.name = name;
>  	init.ops = &cpg_div6_clock_ops;
>  	init.flags = CLK_IS_BASIC;
>  	init.parent_names = parent_names;
> @@ -257,6 +243,50 @@ static void __init cpg_div6_clock_init(struct
> device_node *np) clock->hw.init = &init;
> 
>  	clk = clk_register(NULL, &clock->hw);
> +	if (!IS_ERR(clk))
> +		return clk;
> +
> +error:
> +	kfree(clock->parents);
> +	kfree(clock);

If you think that errors when registering the DIV6 clock are not fatal enough 
to not have to care about memory leakage, you should kfree(clock) when the 
kmalloc_array() above fails.

> +	return clk;
> +}
> +
> +static void __init cpg_div6_clock_init(struct device_node *np)
> +{
> +	unsigned int num_parents;
> +	const char **parent_names;
> +	const char *clk_name = np->name;
> +	void __iomem *reg;
> +	struct clk *clk;
> +	unsigned int i;
> +
> +	num_parents = of_clk_get_parent_count(np);
> +	if (num_parents < 1) {
> +		pr_err("%s: no parent found for %s DIV6 clock\n",
> +		       __func__, np->name);
> +		return;
> +	}
> +
> +	parent_names = kmalloc_array(num_parents, sizeof(*parent_names),
> +				GFP_KERNEL);
> +	if (!parent_names)
> +		return;
> +
> +	reg = of_iomap(np, 0);
> +	if (reg = NULL) {
> +		pr_err("%s: failed to map %s DIV6 clock register\n",
> +		       __func__, np->name);
> +		goto error;
> +	}
> +
> +	/* Parse the DT properties. */
> +	of_property_read_string(np, "clock-output-names", &clk_name);
> +
> +	for (i = 0; i < num_parents; i++)
> +		parent_names[i] = of_clk_get_parent_name(np, i);
> +
> +	clk = cpg_div6_register(clk_name, num_parents, parent_names, reg);
>  	if (IS_ERR(clk)) {
>  		pr_err("%s: failed to register %s DIV6 clock (%ld)\n",
>  		       __func__, np->name, PTR_ERR(clk));
> @@ -269,9 +299,8 @@ static void __init cpg_div6_clock_init(struct
> device_node *np) return;
> 
>  error:
> -	if (clock->reg)
> -		iounmap(clock->reg);
> +	if (reg)
> +		iounmap(reg);
>  	kfree(parent_names);
> -	kfree(clock);
>  }
>  CLK_OF_DECLARE(cpg_div6_clk, "renesas,cpg-div6-clock",
> cpg_div6_clock_init); diff --git a/drivers/clk/shmobile/clk-div6.h
> b/drivers/clk/shmobile/clk-div6.h new file mode 100644
> index 0000000000000000..d19531f42953c83f
> --- /dev/null
> +++ b/drivers/clk/shmobile/clk-div6.h
> @@ -0,0 +1,3 @@
> +
> +struct clk *cpg_div6_register(const char *name, unsigned int num_parents,
> +			      const char **parent_names, void __iomem *reg);

Could you please add #ifdef include guard macros ?

-- 
Regards,

Laurent Pinchart