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

[Marcin Wojtas]

Hi Stephen,

2016-08-25 2:16 GMT+02:00 Stephen Boyd <sboyd@xxxxxxxxxxxxxx>:
> 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.

Ok.

>
>> Signed-off-by: Marcin Wojtas <mw@xxxxxxxxxxxx>
>> ---
>>  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?

Will change to 'if (!cp110clks)'

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

Ok.

>
>> +                                   GFP_KERNEL);
>> +     if (IS_ERR(cp110_clk_data))
>
> Doesn't that return NULL on error?

Same as above, will change.

>
>> +             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().

Will try it. Shouldn't such change be placed in separate commit?

>
>>       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?

No, why? Just below there is a loop using it. Before it was taken from
global variable, which I got rid of.

Best regards,
Marcin