MIME-Version: 1.0
References: <20220106174803.1773876-1-gsomlo@gmail.com> <20220106174803.1773876-4-gsomlo@gmail.com>
In-Reply-To: <20220106174803.1773876-4-gsomlo@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 6 Jan 2022 20:19:39 +0200
Message-ID: <CAHp75Ve_jWmo3+Es0G5SyMpcdC_=hWfxHoa866Difd+X3F0uxg@mail.gmail.com>
Subject: Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
To:     Gabriel Somlo <gsomlo@gmail.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        linux-mmc <linux-mmc@vger.kernel.org>,
        Karol Gugala <kgugala@antmicro.com>,
        Mateusz Holenko <mholenko@antmicro.com>, krakoczy@antmicro.com,
        mdudek@internships.antmicro.com, paulus@ozlabs.org,
        Joel Stanley <joel@jms.id.au>,
        Stafford Horne <shorne@gmail.com>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        david.abdurachmanov@sifive.com, florent@enjoy-digital.fr,
        Randy Dunlap <rdunlap@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 6, 2022 at 7:48 PM Gabriel Somlo <gsomlo@gmail.com> 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@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.

> + */

...

> +#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