Messages in this thread | | | From | Andy Shevchenko <> | Date | Sat, 8 Jan 2022 19:43:19 +0200 | Subject | Re: [PATCH v9 3/3] mmc: Add driver for LiteX's LiteSDCard interface |
| |
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
| |