linux-kernel - Re: [v2] mmc: sdhci-cadence: add HS400 enhanced strobe support

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] [day] [month] [year] [list]

Date:   Sat, 4 Mar 2017 12:37:13 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Piotr Sroka <piotrs@...ence.com>
Cc:     linux-mmc <linux-mmc@...r.kernel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [v2] mmc: sdhci-cadence: add HS400 enhanced strobe support

Hi Piotr,


2017-03-03 17:31 GMT+09:00 Piotr Sroka <piotrs@...ence.com>:
> Add support for HS400ES mode to Cadence SDHCI driver.
>
> Signed-off-by: Piotr Sroka <piotrs@...ence.com>


Thanks for your patch.  It looks almost good to me.
My comments are about some coding style issues only.
Please see below.




> +static void sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv, u32 *mode)
> +{
> +       *mode = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
> +       *mode = *mode & SDHCI_CDNS_HRS06_MODE_MASK;
> +}
> +

Why doesn't this function simply return u32 value?





>
> +static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
> +                                            struct mmc_ios *ios)
> +{
> +       struct sdhci_host *host = mmc_priv(mmc);
> +       struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> +       u32 timingMode;

Could you avoid CamelCase, please?
(perhaps does checkpatch.pl complain about this?).

I guess "mode" could be enough.



> +       priv->enhanced_strobe = ios->enhanced_strobe;
> +
> +       sdhci_cdns_get_emmc_mode(priv, &timingMode);
> +
> +       if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400 &&
> +           ios->enhanced_strobe)
> +               sdhci_cdns_set_emmc_mode(priv,
> +                                        SDHCI_CDNS_HRS06_MODE_MMC_HS400ES);
> +
> +       if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400ES &&
> +           !ios->enhanced_strobe)

You could stretch this if-conditional within 80-col
by renaming "timingMode" into "mode".





> +       host->mmc_host_ops.hs400_enhanced_strobe =
> +               sdhci_cdns_hs400_enhanced_strobe;

I should not say this if this is the only matter,
but can you move the 2nd line to the right by inserting some more tabs?



FYI:
If you have not checked the coding-style guideline yet, and you are interested,
it is a good idea to read Documentation/process/coding-style.rst


Some of my comments are based on the following statements in that document:

  - "LOCAL variable names should be short, and to the point."
  - "Descendants are always substantially shorter than the parent and
     are placed substantially to the right."

We need not too be nervous about the style,
but generally speaking, it will be nice to keep the style consistent
unless there is a specific reason.

Thanks!

-- 
Best Regards
Masahiro Yamada