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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VeEvT-_47gKFAYdz-BR9y=KLEw2uMbRxYKo1rLQSQEfyg@mail.gmail.com>
Date:   Sat, 8 Jan 2022 19:43:19 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Gabriel Somlo <gsomlo@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        Karol Gugala <kgugala@...micro.com>,
        Mateusz Holenko <mholenko@...micro.com>,
        Kamil Rakoczy <krakoczy@...micro.com>,
        mdudek@...ernships.antmicro.com,
        Paul Mackerras <paulus@...abs.org>,
        Joel Stanley <joel@....id.au>,
        Stafford Horne <shorne@...il.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        david.abdurachmanov@...ive.com,
        Florent Kermarrec <florent@...oy-digital.fr>,
        Randy Dunlap <rdunlap@...radead.org>
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@...il.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