Re: [PATCH v9 3/3] mmc: Add driver for LiteX's LiteSDCard interface

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sat, Jan 8, 2022 at 6:11 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!

Thanks for an update, my comments below.

...

> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/litex.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>

> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sd.h>

I would move this group of headers...

> +#include <linux/platform_device.h>
> +

...somewhere here to show that this driver belongs to the MMC subsystem.

...

> +#define LITEX_MMC_OCR (MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 | \
> +                      MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 | \
> +                      MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36)

Seems to me this is identical to
https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/au1xmmc.c#L72

And may be reused in
https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/vub300.c#L2168.

Perhaps it makes sense to have

#define MMC_VDD_27_36 ...

in mmc.h?

In any case, it can be postponed, just a side note for the future improvements.

...

> +       /* Ensure bus width will be set (again) upon card (re)insertion */
> +       if (ret == 0)
> +               host->is_bus_width_set = false;
> +
> +       return ret;

Please, switch to standard pattern, i.e.

  if (ret)
    return ret;
  ...
  return 0;

...

> +       u32 div;
> +
> +       div = freq ? host->ref_clk / freq : 256U;

> +       div = roundup_pow_of_two(div);
> +       div = clamp(div, 2U, 256U);

Not sure why it becomes two lines again.

...

> +       ret = devm_add_action_or_reset(dev,
> +                                      (void(*)(void *))mmc_free_host, mmc);

One line?
An actually preferable way is to define a separate wrapper function
and use it here without any casting.

> +       if (ret) {

> +               dev_err(dev, "Failed to register mmc_free_host action\n");
> +               return ret;

return dev_err_probe(...);

> +       }

...

> +       clk = devm_clk_get(dev, NULL);
> +       if (IS_ERR(clk)) {

> +               ret = dev_err_probe(dev, PTR_ERR(clk), "can't get clock\n");
> +               return ret;

    return dev_err_probe(...);

> +       }

...

> +       ret = mmc_add_host(mmc);
> +
> +       return ret;

It's now

    return mmc_add_host(...);

> +}

-- 
With Best Regards,
Andy Shevchenko