Re: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller

[Stephen Boyd <sboyd@codeaurora.org>]

On 08/23, Marcin Wojtas wrote:
> Original commit, which added support for Armada CP110 system controller
> used global variables for storing all clock information. It worked
> fine for Armada 7k SoC, with single CP110 block. After dual-CP110 Armada 8k
> was introduced, the data got overwritten and corrupted.
> 
> This patch fixes the issue by allocating resources dynamically in the
> driver probe and storing it as platform drvdata.
> 
> Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")
> 

Please drop the space between fixes tag and the signoff.

> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  drivers/clk/mvebu/cp110-system-controller.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
> index 0835e1d..2bd87d2 100644
> --- a/drivers/clk/mvebu/cp110-system-controller.c
> +++ b/drivers/clk/mvebu/cp110-system-controller.c
> @@ -81,13 +81,6 @@ enum {
>  #define CP110_GATE_EIP150		25
>  #define CP110_GATE_EIP197		26
>  
> -static struct clk *cp110_clks[CP110_CLK_NUM];
> -
> -static struct clk_onecell_data cp110_clk_data = {
> -	.clks = cp110_clks,
> -	.clk_num = CP110_CLK_NUM,
> -};
> -
>  struct cp110_gate_clk {
>  	struct clk_hw hw;
>  	struct regmap *regmap;
> @@ -195,7 +188,8 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>  	struct regmap *regmap;
>  	struct device_node *np = pdev->dev.of_node;
>  	const char *ppv2_name, *apll_name, *core_name, *eip_name, *nand_name;
> -	struct clk *clk;
> +	struct clk_onecell_data *cp110_clk_data;
> +	struct clk *clk, **cp110_clks;
>  	u32 nand_clk_ctrl;
>  	int i, ret;
>  
> @@ -208,6 +202,20 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	cp110_clks = devm_kcalloc(&pdev->dev, sizeof(struct clk *),
> +				  CP110_CLK_NUM, GFP_KERNEL);
> +	if (IS_ERR(cp110_clks))

Doesn't that return NULL on error?

> +		return PTR_ERR(cp110_clks);
> +
> +	cp110_clk_data = devm_kzalloc(&pdev->dev,
> +				      sizeof(struct clk_onecell_data),

sizeof(*cp110_clk_data) please

> +				      GFP_KERNEL);
> +	if (IS_ERR(cp110_clk_data))

Doesn't that return NULL on error?

> +		return PTR_ERR(cp110_clk_data);
> +
> +	cp110_clk_data->clks = cp110_clks;
> +	cp110_clk_data->clk_num = CP110_CLK_NUM;
> +
>  	/* Register the APLL which is the root of the clk tree */
>  	of_property_read_string_index(np, "core-clock-output-names",
>  				      CP110_CORE_APLL, &apll_name);
> @@ -335,10 +343,12 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>  		cp110_clks[CP110_MAX_CORE_CLOCKS + i] = clk;
>  	}
>  
> -	ret = of_clk_add_provider(np, cp110_of_clk_get, &cp110_clk_data);
> +	ret = of_clk_add_provider(np, cp110_of_clk_get, cp110_clk_data);

It would be nice if this could be converted to
of_clk_add_hw_provider().

>  	if (ret)
>  		goto fail_clk_add;
>  
> +	platform_set_drvdata(pdev, cp110_clks);
> +
>  	return 0;
>  
>  fail_clk_add:
> @@ -365,6 +375,7 @@ fail0:
>  
>  static int cp110_syscon_clk_remove(struct platform_device *pdev)
>  {
> +	struct clk **cp110_clks = platform_get_drvdata(pdev);

Is this variable unused now?

>  	int i;
>  
>  	of_clk_del_provider(pdev->dev.of_node);
> -- 
> 1.8.3.1
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project