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

[Masahiro Yamada]

Hi Piotr,


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


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