lkml.org 
[lkml]   [2017]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v6 08/14] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
    From
    Date
    Hi Ulf,

    On 2017/3/15 21:11, Ulf Hansson wrote:
    > On 14 February 2017 at 18:01, Gregory CLEMENT
    > <gregory.clement@free-electrons.com> wrote:
    >> From: Hu Ziji <huziji@marvell.com>
    <snip>
    >> +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?

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

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

    \
     
     \ /
      Last update: 2017-03-15 15:01    [W:2.204 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site