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 <m...@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
