Re: [PATCH 2/2] mmc: sdhci-of-ma35d1: Add Novoton MA35D1 SDHCI driver

[Shan-Chun Hung <shanchun1218@gmail.com>]

Dear Andy,

Thanks for you review.

On 2024/6/21 下午 07:25, Andy Shevchenko wrote:
> On Fri, Jun 21, 2024 at 10:06 AM Shan-Chun Hung<shanchun1218@gmail.com>  wrote:
>> On 2024/6/20 上午 03:09, Andy Shevchenko wrote:
>>> On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung<shanchun1218@gmail.com>   wrote:
> ...
>
>>> You are missing a lot of header inclusions, please follow IWYU principle.
>> I am not familiar with IWYU yet, but I will learn it and use it for
>> checks later on.
> "Include What You Use". But some of the headers may be omitted as they
> are guaranteed to be included by others. It's a bit hard because one
> should know and follow the kernel development, currently the headers
> in the kernel are a bit of a mess.
Absolutely, kernel development needs careful attention to many details, 
like managing header file
> ...
>
>>>> +#define BOUNDARY_OK(addr, len) \
>>>> +       ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
>>> Besides sizes.h being missed, this can be done with help of ALIGN()
>>> macro (or alike). So, kill this and use the globally defined macro
>>> inline.
>> I will add sizes.h and directly apply globally defined  ALIGN() macro
>> instead
> Also check what header should be included for that macro, IIRC it's align.h.
I will add add "#include <linux/align.h>"
> ...
>
>>>> +               for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
>>>> +                       if (restore_data[idx].width == 32)
>>> sizeof(u32) ?
>> Your idea is better, I will change it.
> You might probably want to use the same in the restore_data array initialiser.
I will modify it.
>>>> +                               val[idx] = sdhci_readl(host, restore_data[idx].reg);
>>>> +                       else if (restore_data[idx].width == 8)
>>> sizeof(u8) ?
>> I will fix it.
>>>> +                               val[idx] = sdhci_readb(host, restore_data[idx].reg);
>>>> +               }
> ...
>
>>>> +               pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>>>> +               if (IS_ERR(pltfm_host->clk)) {
>>>> +                       err = PTR_ERR(pltfm_host->clk);
>>>> +                       dev_err(&pdev->dev, "failed to get clk: %d\n", err);
>>> Use
>>>
>>>     return dev_err_probe(...);
>> I will use dev_err_probe() instead of dev_err()
>>>> +                       goto free_pltfm;
>>> This is wrong. You may not call non-devm before devm ones, otherwise
>>> it makes a room for subtle mistakes on remove-probe or unbind-bind
>>> cycles. Have you tested that?
>> I have tested it, there is no error messages during driver initial process.
>>
>> My thought is that sdhci_pltfm_init() and sdhci_pltfm_free() are used together.
>>
>> If there's any error after the successful execution of sdhci_pltfm_init(),
>> I'll use sdhci_pltfm_free().
>>
>> I am not entirely sure if this answers your question.
> Yes, they are, the problem is that freeing resources happens in
> non-reversed order (for some of the resources). This might lead to
> subtle mistakes as I said above. The rule of thumb is to avoid mixing
> devm_*() with non-devm_*() calls. If you have both, they have to be
> grouped as all devm_*() followed by all non-devm_*().
> In some cases you might need to wrap existing calls to become managed.
> This may be done with the help of devm_add_action_or_reset(). Check
> other drivers which are using that.
I will add devm_add_action_or_reset() to do sdhci_pltfm_free().
>>>> +               }
>>>> +               err = clk_prepare_enable(pltfm_host->clk);
>>>> +               if (err)
>>>> +                       goto free_pltfm;
>>> Use _enabled variant of devm_clk_get() instead.
>> I will use devm_clk_get_optional_enabled() instead.
>>>> +       }
> ...
>
>>>> +free_pltfm:
>>>> +       sdhci_pltfm_free(pdev);
>>> This should go to be correct in ordering.
>> I am not entirely sure if it is similar to the "goto free_pltfm;" issue.
> Yes. It's part of the same issue.
>>>> +       return err;
>>>> +}
>>>> +
>>>> +static int ma35_remove(struct platform_device *pdev)
>>> Use remove_new callback.
>> I will fix it.
>>>> +{
>>>> +       struct sdhci_host *host = platform_get_drvdata(pdev);
>>>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +
>>>> +       sdhci_remove_host(host, 0);
>>>> +       clk_disable_unprepare(pltfm_host->clk);
>>>> +       sdhci_pltfm_free(pdev);
>>> At least these two will go away as per probe error path.
>> I will use sdhci_pltfm_remove instead of  the ma35_remove.
> After fixing the ordering issues in ->probe() this might need more
> modifications.
Understood, I will correct these issues as soon as possible.
>>>> +       return 0;
>>>> +}
> --
> With Best Regards,
> Andy Shevchenko

Best Regards,

Shan-Chun