On 2020/11/8 21:55, Markus Elfring wrote:
>> When return errors, …
>
> I would find an other wording more appropriate for this change description.
>
>
>> …, so fix this issue.
>
> I suggest to replace this information by an other imperative wording
> and the tag “Fixes”.
OK, done， I have submitted the version 2 patch
>
>
> …
>> +++ b/drivers/clk/hisilicon/clk-hi3620.c
>> @@ -463,12 +463,16 @@ static void __init hi3620_mmc_clk_init(struct device_node *node)
>> }
>>
>> clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>> - if (WARN_ON(!clk_data))
>> + if (WARN_ON(!clk_data)) {
>> + iounmap(base);
>
> Can a statement like “goto unmap_io;” make sense here?
OK， I have changed it.
>
>
>> return;
>> + }
>>
>> clk_data->clks = kcalloc(num, sizeof(*clk_data->clks), GFP_KERNEL);
>> - if (!clk_data->clks)
>> + if (!clk_data->clks) {
>
> How do you think about to add the function call “kfree(clk_data)” in this if branch?
OK, I have changed it.
>
>
> …
>> +++ b/drivers/clk/hisilicon/clk.c
> …
> if (!base) {
> pr_err("%s: failed to map clock registers\n", __func__);
> - goto err;
> + return NULL;
>
>
>> @@ -69,8 +69,10 @@ struct hisi_clock_data *hisi_clk_init(struct device_node *np,
>> }
>>
>> clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>> - if (!clk_data)
>> + if (!clk_data) {
>> + iounmap(base);
>> goto err;
>
> Please use another jump target.
OK, thanks, I have changed it.
>
>
>> @@ -82,6 +84,7 @@ struct hisi_clock_data *hisi_clk_init(struct device_node *np,
>> of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data->clk_data);
>> return clk_data;
>> err_data:
>> + iounmap(base);
>> kfree(clk_data);
>> err:
>> return NULL;
>
> I propose to apply the following code variant.
OK, have modified.
>
> return clk_data;
>
> free_clk_data:
> kfree(clk_data);
> unmap_io:
> iounmap(base);
> return NULL;
>
>
> Regards,
> Markus
> .
>
