Hi Ulf,

On 2017/3/15 21:11, Ulf Hansson wrote:

On 14 February 2017 at 18:01, Gregory CLEMENT
[off-list ref] wrote:

quoted

From: Hu Ziji <huziji@marvell.com>

<snip>

quoted

+config MMC_SDHCI_XENON
+       tristate "Marvell Xenon eMMC/SD/SDIO SDHCI driver"
+       depends on MMC_SDHCI_PLTFM
+       help
+         This selects Marvell Xenon eMMC/SD/SDIO SDHCI.
+         If you have a machine with integrated Marvell Xenon SDHC IP,

/s/SDHC/SDHCI

	Sorry. I don't get your point.
	Could you please give more detailed comments?

quoted

+         say Y or M here.
+         If unsure, say N.

diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index ccc9c4cba154..b0a2ab4b256e 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile

@@ -82,3 +82,6 @@ obj-$(CONFIG_MMC_SDHCI_BRCMSTB)               += sdhci-brcmstb.o
 ifeq ($(CONFIG_CB710_DEBUG),y)
        CFLAGS-cb710-mmc        += -DDEBUG
 endif
+
+obj-$(CONFIG_MMC_SDHCI_XENON)  += sdhci-xenon-driver.o
+sdhci-xenon-driver-y           += sdhci-xenon.o

Why not only this:
obj-$(CONFIG_MMC_SDHCI_XENON) += sdhci-xenon.o

	Yes. Will try.

quoted

diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
new file mode 100644

<snip>

ff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h

quoted

new file mode 100644
index 000000000000..69de711db9eb

--- /dev/null
+++ b/drivers/mmc/host/sdhci-xenon.h

You should probably put all this in the c-file instead. That is how
most other sdhci variants does it.

	Unfortunately, we have some registers like XENON_SLOT_EMMC_CTRL
	which are accessed by both sdhci-xenon.c and sdhci-xenon-phy.c

[...]

Overall this looks good to me, however I think Adrian needs to have a
quick look as well.

One additional very minor nitpick. Perhaps you can align on the
function names prefix, as those currently varies between "whatever",
"xenon_" and "sdhci_xenon_".

	Sure. Will fix them as you request.

	Thank you.

Best regards,
Hu Ziji

Kind regards
Uffe