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