Re: [PATCH 04/11] clk: ti: clkctrl: support multiple clkctrl nodes under a cm node

[Tero Kristo <t-kristo@ti.com>]

On 16/10/18 02:38, Stephen Boyd wrote:
> Quoting Tero Kristo (2018-10-15 08:58:21)
>> On 13/10/18 01:03, Stephen Boyd wrote:
>>> Quoting Rob Herring (2018-10-12 14:45:08)
>>>> On Fri, Oct 12, 2018 at 4:25 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>>
>>>>> Quoting Tero Kristo (2018-08-31 08:07:00)
>>>>>> @@ -492,19 +501,35 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>>>>>>
>>>>>>           provider->base = of_iomap(node, 0);
>>>>>>
>>>>>> -       provider->clkdm_name = kmalloc(strlen(node->parent->name) + 3,
>>>>>> -                                      GFP_KERNEL);
>>>>>> -       if (!provider->clkdm_name) {
>>>>>> -               kfree(provider);
>>>>>> -               return;
>>>>>> +       if (ti_clk_get_features()->flags & TI_CLK_CLKCTRL_COMPAT) {
>>>>>> +               provider->clkdm_name = kmalloc(strlen(node->parent->name) + 3,
>>>>>> +                                              GFP_KERNEL);
>>>>>> +               if (!provider->clkdm_name) {
>>>>>> +                       kfree(provider);
>>>>>> +                       return;
>>>>>> +               }
>>>>>> +
>>>>>> +               /*
>>>>>> +                * Create default clkdm name, replace _cm from end of parent
>>>>>> +                * node name with _clkdm
>>>>>> +                */
>>>>>> +               strcpy(provider->clkdm_name, node->parent->name);
>>>>
>>>> kasprintf can replace kmalloc, strlen and strcpy here and remove the
>>>> direct access of .name in the process.
>>>>
>>>>>> +               provider->clkdm_name[strlen(provider->clkdm_name) - 2] = 0;
>>>>>> +       } else {
>>>>>> +               provider->clkdm_name = kmalloc(strlen(node->name), GFP_KERNEL);
>>>>>> +               if (!provider->clkdm_name) {
>>>>>> +                       kfree(provider);
>>>>>> +                       return;
>>>>>> +               }
>>>>>> +
>>>>>> +               /*
>>>>>> +                * Create default clkdm name, replace _clkctrl from end of
>>>>>> +                * node name with _clkdm
>>>>>> +                */
>>>>>> +               strcpy(provider->clkdm_name, node->name);
>>>>
>>>> and here.
>>>>
>>>>>> +               provider->clkdm_name[strlen(provider->clkdm_name) - 7] = 0;
>>>>>>           }
>>>>>>
>>>>>
>>>>> This conflicts with Rob's change so I tried to fix it up. Let me know if
>>>>> something is horribly wrong with it. I suspect Rob will need to figure
>>>>> out how to make node::name usage go away again, but I put it back.
>>>>
>>>> Then I don't have to find and fix. :)
>>>>
>>>
>>> Can you send a patch? Otherwise I will try and fix this up tomorrow.
>>> You can use this git tree for reference:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-ti
>>>
>>
>> Sorry, I've also been OoO for last week, just captured this.
>>
>> Do you need help on this one? I can take a look at it tomorrow if there
>> is a specific merge conflict you want me to take a look at.
> 
> This is what I got:

Ok that looks fine except one detail below:

> 
> ----8<---
> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
> index 955f2e26ab00..78deca44789d 100644
> --- a/drivers/clk/ti/clkctrl.c
> +++ b/drivers/clk/ti/clkctrl.c
> @@ -520,8 +520,7 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>   	provider->base = of_iomap(node, 0);
>   
>   	if (ti_clk_get_features()->flags & TI_CLK_CLKCTRL_COMPAT) {
> -		provider->clkdm_name = kmalloc(strlen(node->parent->name) + 3,
> -					       GFP_KERNEL);
> +		provider->clkdm_name = kasprintf(GFP_KERNEL, "%pOFn%s", node->parent, "xxx");
>   		if (!provider->clkdm_name) {
>   			kfree(provider);
>   			return;
> @@ -531,10 +530,9 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>   		 * Create default clkdm name, replace _cm from end of parent
>   		 * node name with _clkdm
>   		 */
> -		strcpy(provider->clkdm_name, node->parent->name);
> -		provider->clkdm_name[strlen(provider->clkdm_name) - 2] = 0;
> +		provider->clkdm_name[strlen(provider->clkdm_name) - 5] = 0;
>   	} else {
> -		provider->clkdm_name = kmalloc(strlen(node->name), GFP_KERNEL);
> +		provider->clkdm_name = kasprintf(GFP_KERNEL, "%pOFn", node->name);

should be:

  +		provider->clkdm_name = kasprintf(GFP_KERNEL, "%pOFn", node);

...instead right?

-Tero


>   		if (!provider->clkdm_name) {
>   			kfree(provider);
>   			return;
> @@ -544,7 +542,6 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>   		 * Create default clkdm name, replace _clkctrl from end of
>   		 * node name with _clkdm
>   		 */
> -		strcpy(provider->clkdm_name, node->name);
>   		provider->clkdm_name[strlen(provider->clkdm_name) - 7] = 0;
>   	}
>   
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki