Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
flat view
From: Andy Shevchenko <hidden>
Date: 2022-01-06 18:20:19
Also in:
linux-mmc, lkml
On Thu, Jan 6, 2022 at 7:48 PM Gabriel Somlo [off-list ref] wrote:
LiteX (https://github.com/enjoy-digital/litex) is a SoC framework that targets FPGAs. LiteSDCard is a small footprint, configurable SDCard core commonly used in LiteX designs. The driver was first written in May 2020 and has been maintained cooperatively by the LiteX community. Thanks to all contributors!
quoted hunk ↗ jump to hunk
+config MMC_LITEX + tristate "LiteX MMC Host Controller support" + depends on OF + depends on PPC_MICROWATT || LITEX || COMPILE_TEST + help + This selects support for the MMC Host Controller found in LiteX SoCs. + + If unsure, say N.
What would be the module name if built as a module? ...
quoted hunk ↗ jump to hunk
+/* + * LiteX LiteSDCard driver + * + * Copyright (C) 2019-2020 Antmicro <contact@antmicro.com> + * Copyright (C) 2019-2020 Kamil Rakoczy <krakoczy@antmicro.com> + * Copyright (C) 2019-2020 Maciej Dudek <mdudek@internships.antmicro.com> + * Copyright (C) 2020 Paul Mackerras <paulus@ozlabs.org> + * Copyright (C) 2020-2021 Gabriel Somlo <gsomlo@gmail.com>
+ *
Redundant blank line.
+ */
...
quoted hunk ↗ jump to hunk
+#include <linux/module.h> +#include <linux/litex.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/interrupt.h> +#include <linux/iopoll.h> +#include <linux/mmc/sd.h> +#include <linux/mmc/mmc.h> +#include <linux/mmc/host.h> +#include <linux/delay.h> +#include <linux/dma-mapping.h>
Perhaps keep it sorted? It will easily show, for example, absence of bits.h. ...
+ ret = readx_poll_timeout(litex_read8, reg, evt, (evt & SD_BIT_DONE),
Too many parentheses.
+ SD_SLEEP_US, SD_TIMEOUT_US);
+ if (ret || (evt & SD_BIT_TIMEOUT))
Redundant second condition. If you want +1 iteration, increase timeout.
+ return -ETIMEDOUT;
Why shadowed error code? ...
+ pr_err("%s: unknown error evt=%x\n", __func__, evt);Use dev_err(). ...
quoted hunk ↗ jump to hunk
+ /* Wait for an interrupt if we have an interrupt and either there is + * data to be transferred, or if the card can report busy via DAT0. + */
This comment style is for the net subsystem, for others we use /* * Starting here... */ Fix it everywhere in your code. ...
quoted hunk ↗ jump to hunk
+ reg = host->sdcore + LITEX_CORE_CMDRSP; + for (i = 0; i < 4; i++) { + host->resp[i] = litex_read32(reg); + reg += sizeof(u32); + }
Isn't it memcpy_fromio()? ...
+ if (!host->app_cmd && cmd == SD_SEND_RELATIVE_ADDR) + host->rca = (host->resp[3] >> 16) & 0xffff;
Are you expecting a 32-bit value to be bigger than 2^32-1? ...
+ div = min(max(div, 2U), 256U);
clamp_t() / clamp_val() ? ...
quoted hunk ↗ jump to hunk
+ ret = platform_get_irq_optional(host->dev, 0); + if (ret == -ENXIO || ret == 0) { + dev_warn(dev, "Failed to get IRQ, using polling\n"); + goto use_polling; + } + if (ret < 0) + return ret; /* e.g., deferred probe */ + host->irq = ret;
Can it be rather written as
ret = platform_get_irq_optional(host->dev, 0);
if (ret < 0 && ret != -ENXIO)
return ret;
if (ret > 0)
host->irq = ret;
else {
dev_warn(dev, "Failed to get IRQ, using polling\n");
goto use_polling;
}
?
...
+use_polling: + host->mmc->caps |= MMC_CAP_NEEDS_POLL;
+ host->irq = 0;
Isn't it 0 by default? ...
+ mmc = mmc_alloc_host(sizeof(struct litex_mmc_host), &pdev->dev);
quoted hunk ↗ jump to hunk
+ /* NOTE: defaults to max_[req,seg]_size=PAGE_SIZE, max_blk_size=512, + * and max_blk_count accordingly set to 8; + * If for some reason we need to modify max_blk_count, we must also + * re-calculate `max_[req,seg]_size = max_blk_size * max_blk_count;` + */
Can you rather not split code by this comment. It makes sense to be above, no?
+ if (!mmc) + return -ENOMEM;
...
quoted hunk ↗ jump to hunk
+ /* set default sd_clk frequency range based on empirical observations + * of LiteSDCard gateware behavior on typical SDCard media + */
Start sentences from capital letters and keep proper style of multi-line comments. ...
quoted hunk ↗ jump to hunk
+err: + mmc_free_host(mmc); + return ret;
This...
quoted hunk ↗ jump to hunk
+} + +static int litex_mmc_remove(struct platform_device *pdev) +{ + struct litex_mmc_host *host = dev_get_drvdata(&pdev->dev); + + if (host->irq > 0) + free_irq(host->irq, host->mmc); + mmc_remove_host(host->mmc); + mmc_free_host(host->mmc);
...and this have ordering issues. You mixed devm_*() with non-devm_*() APIs in the wrong way. Also, I haven't noticed the free_irq() call in the error path of ->probe(). Isn't it missed?
+ return 0; +}
...
+ .of_match_table = of_match_ptr(litex_match),
Wrong usage of of_match_ptr(). -- With Best Regards, Andy Shevchenko