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
