Re: [PATCH 4/5] mmc: sdhci: consolidate sdhci-of-esdhc andsdhci-esdhc-imx

[Shawn Guo]

On Tue, Apr 19, 2011 at 12:21:10PM +0200, Wolfram Sang wrote:
> >  config MMC_SDHCI_ESDHC_IMX
> > -	bool "SDHCI platform support for the Freescale eSDHC i.MX controller"
> > +	bool "SDHCI support for the Freescale eSDHC i.MX controller"
> >  	depends on ARCH_MX25 || ARCH_MX35 || ARCH_MX5
> >  	depends on MMC_SDHCI
> > -	select MMC_SDHCI_PLTFM
> > +	select MMC_SDHCI_ESDHC
> >  	select MMC_SDHCI_IO_ACCESSORS
> >  	help
> > -	  This selects the Freescale eSDHC controller support on the platform
> > -	  bus, found on platforms like mx35/51.
> > +	  This selects the Freescale eSDHC controller support on platforms
> > +	  like mx35/51.
> 
> While we are at it, mx25 could be added and mx53 (and you know better
> what else will be coming ;))
> 
It's a direct copy.  Will update it properly.

> > diff --git a/drivers/mmc/host/sdhci-esdhc.c b/drivers/mmc/host/sdhci-esdhc.c
> > new file mode 100644
> > index 0000000..b3d1bc1
> > --- /dev/null
> > +++ b/drivers/mmc/host/sdhci-esdhc.c
> > @@ -0,0 +1,413 @@
> > +/*
> > + * Freescale eSDHC controller driver for MPCxxx and i.MX.
> > + *
> > + * Copyright (c) 2007 Freescale Semiconductor, Inc.
> > + *   Author: Xiaobo Xie <X.Xie@xxxxxxxxxxxxx>
> > + *
> > + * Copyright (c) 2009 MontaVista Software, Inc.
> > + *   Author: Anton Vorontsov <avorontsov@xxxxxxxxxxxxx>
> > + *
> > + * Copyright (c) 2010 Pengutronix e.K.
> > + *   Author: Wolfram Sang <w.sang@xxxxxxxxxxxxxx>
> > + *
> > + * 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; either version 2 of the License.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/clk.h>
> > +#include <linux/mmc/host.h>
> > +#include <linux/mmc/sdhci-pltfm.h>
> > +#ifdef CONFIG_MMC_SDHCI_ESDHC_IMX
> > +#include <mach/hardware.h>
> > +#endif
> > +#include "sdhci.h"
> > +#include "sdhci-pltfm.h"
> > +
> > +/*
> > + * Ops and quirks for the Freescale eSDHC controller.
> > + */
> > +
> > +#define ESDHC_DEFAULT_QUIRKS	(SDHCI_QUIRK_FORCE_BLK_SZ_2048 | \
> > +				SDHCI_QUIRK_BROKEN_CARD_DETECTION | \
> > +				SDHCI_QUIRK_NO_BUSY_IRQ | \
> > +				SDHCI_QUIRK_NONSTANDARD_CLOCK | \
> > +				SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK | \
> > +				SDHCI_QUIRK_PIO_NEEDS_DELAY | \
> > +				SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET | \
> > +				SDHCI_QUIRK_NO_CARD_NO_RESET)
> 
> These are not the current quirks. Meanwhile BROKEN_CARD_DETECTION is
> gone as well as NO_CARD_NO_RESET (at least for imx). My additions to use
> GPIOs for card detect and write protect are also not in this version.
> 
Next version will be based on mmc-next.

> [...]
> 
> > +static struct sdhci_pltfm_data sdhci_esdhc_mpc_pdata = {
> > +	.quirks = ESDHC_DEFAULT_QUIRKS,
> > +	.ops = &sdhci_esdhc_mpc_ops,
> > +};
> > +#endif
> 
> Please mark the #endif with comments of the expression they are
> depending on, e.g.
> 
> #endif /* CONFIG_MMC_SDHCI_ESDHC_MPC */
> 
> if it is not immediately visible. That helps readability.
> 
Ok, since you like it :)

> [...]
> 
> Phew, due to all these hardware bugs, the driver will get messy, even if
> we try hard. Any chances that future revisions of the core will be
> updated? :)
> 
I doubt all these quirks are design bugs but some designs not fully
compliant to SDHC specification, which you still can say bugs :)

-- 
Regards,
Shawn

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/