On Thu, Jan 6, 2022 at 7:48 PM Gabriel Somlo <gsomlo@xxxxxxxxx> 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! > +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? ... > +/* > + * LiteX LiteSDCard driver > + * > + * Copyright (C) 2019-2020 Antmicro <contact@xxxxxxxxxxxx> > + * Copyright (C) 2019-2020 Kamil Rakoczy <krakoczy@xxxxxxxxxxxx> > + * Copyright (C) 2019-2020 Maciej Dudek <mdudek@xxxxxxxxxxxxxxxxxxxxxxxx> > + * Copyright (C) 2020 Paul Mackerras <paulus@xxxxxxxxxx> > + * Copyright (C) 2020-2021 Gabriel Somlo <gsomlo@xxxxxxxxx> > + * Redundant blank line. > + */ ... > +#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(). ... > + /* 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. ... > + 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() ? ... > + 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); > + /* 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; ... > + /* 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. ... > +err: > + mmc_free_host(mmc); > + return ret; This... > +} > + > +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
