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

[Shan-Chun Hung]

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@xxxxxxxxx>  wrote:
> > On 2024/6/20 上午 03:09, Andy Shevchenko wrote:
> > > On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung<shanchun1218@xxxxxxxxx>   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