Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753465AbdCONLx (ORCPT ); Wed, 15 Mar 2017 09:11:53 -0400 Received: from mail-ua0-f172.google.com ([209.85.217.172]:34902 "EHLO mail-ua0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751100AbdCONLd (ORCPT ); Wed, 15 Mar 2017 09:11:33 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Ulf Hansson Date: Wed, 15 Mar 2017 14:11:31 +0100 Message-ID: Subject: Re: [PATCH v6 08/14] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality To: Gregory CLEMENT Cc: Adrian Hunter , "linux-mmc@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Mike Turquette , Stephen Boyd , linux-clk , "linux-kernel@vger.kernel.org" , Rob Herring , "devicetree@vger.kernel.org" , Ziji Hu , Jimmy Xu , Jisheng Zhang , Nadav Haklai , Ryan Gao , Doug Jones , Victor Gu , "Wei(SOCP) Liu" , Wilson Ding , Yehuda Yitschak , Marcin Wojtas , Hanna Hawa , Kostya Porotchkin Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4156 Lines: 126 On 14 February 2017 at 18:01, Gregory CLEMENT wrote: > From: Hu Ziji > > Add Xenon eMMC/SD/SDIO host controller core functionality. > Add Xenon specific initialization process. > Add Xenon specific mmc_host_ops APIs. > Add Xenon specific register definitions. > > Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig. > > Marvell Xenon SDHC conforms to SD Physical Layer Specification > Version 3.01 and is designed according to the guidelines provided > in the SD Host Controller Standard Specification Version 3.00. > > Signed-off-by: Hu Ziji > Signed-off-by: Gregory CLEMENT > --- > drivers/mmc/host/Kconfig | 9 +- > drivers/mmc/host/Makefile | 3 +- > drivers/mmc/host/sdhci-xenon.c | 602 ++++++++++++++++++++++++++++++++++- > drivers/mmc/host/sdhci-xenon.h | 70 ++++- > 4 files changed, 684 insertions(+) > create mode 100644 drivers/mmc/host/sdhci-xenon.c > create mode 100644 drivers/mmc/host/sdhci-xenon.h > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index 2eb97014dc3f..1c1a88bfbdad 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -819,3 +819,12 @@ config MMC_SDHCI_BRCMSTB > Broadcom STB SoCs. > > If unsure, say Y. > + > +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 > + 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 > diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c > new file mode 100644 > index 000000000000..e633f803907a > --- /dev/null > +++ b/drivers/mmc/host/sdhci-xenon.c > @@ -0,0 +1,602 @@ > +/* > + * Driver for Marvell Xenon SDHC as a platform device > + * > + * Copyright (C) 2016 Marvell, All Rights Reserved. > + * > + * Author: Hu Ziji > + * Date: 2016-8-24 > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * Inspired by Jisheng Zhang > + * Special thanks to Video BG4 project team. > + */ > + > +#include > +#include > +#include > + > +#include "sdhci-pltfm.h" > +#include "sdhci-xenon.h" > + > +static int enable_xenon_internal_clk(struct sdhci_host *host) [...] > +/* Set SDCLK-off-while-idle */ > +static void xenon_set_sdclk_off_idle(struct sdhci_host *host, > + unsigned char sdhc_id, bool enable) [...] > +/* Recover the Register Setting cleared during SOFTWARE_RESET_ALL */ > +static void sdhci_xenon_reset_exit(struct sdhci_host *host, > + unsigned char sdhc_id, u8 mask) [...] 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. [...] 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_". Kind regards Uffe