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

["Gabriel L. Somlo" <gsomlo@gmail.com>]

On Sat, Jan 08, 2022 at 07:43:19PM +0200, Andy Shevchenko wrote:
> 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.

OK, lined up for v10
 
> ...
> 
> > +#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.

I'm awaiting follow-up advice from Ulf Hansson, who originally suggested
this should be dynamically configured through a dummy voltage regulator
in DTS. Since LiteSDCard doesn't a (current or planned) option to
adaptively configure voltages via software, I think hard-coding the
valid range in the driver (in the exact way as au1xmmc.c) might be
cleaner, and if we end up agreeing on that, there might be opportunity
for factoring it out in the way you describe.

> 
> ...
> 
> > +       /* 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;

OK, lined up for v10

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

Per my previous email, I have:

        div = clamp((u32)roundup_pow_of_two(div), 2U, 256U);

... lined up for v10 (pending also Geert's OK on the (u32) cast
to shut up compiler warnings) :)

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

Done and lined up for v10:

    /* wrapper for use with devm_add_action_or_reset(), below */
    static void litex_mmc_free_host_wrapper(void *ptr)
    {
        mmc_free_host((struct mmc_host *)ptr);
    }

    static int litex_mmc_probe(struct platform_device *pdev)
    {
        ...
        ret = devm_add_action_or_reset(dev, litex_mmc_free_host_wrapper, mmc);
        ...
    }

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

OK.
 
> > +       }
> 
> ...
> 
> > +       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(...);

OK.

> > +       }
> 
> ...
> 
> > +       ret = mmc_add_host(mmc);
> > +
> > +       return ret;
> 
> It's now
> 
>     return mmc_add_host(...);

OK.

I'll wait till sometime tomorrow for additional feedback on clamp()
casting and voltage range hard-coding vs. regulators, before I send
out v10 so we can continue from there.

Thanks, as always,
--Gabriel