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

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

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