This patch set is to take sdhci device driver specific things out from sdhci-pltfm.c and make them self registered. Here are the differences it makes.
* Get the sdhci device driver follow the Linux trend that driver take the registration by its own * sdhci-pltfm.c becomes significantly simpler and only has common support functions there * All sdhci device specific stuff are going back its own driver * The dt and non-dt support share the same pair of .probe and .remove hooks.
The first one patch adds common support function into sdhci-pltfm.c when changing sdhci-esdhc-imx driver, while the last one cleans up sdhci-pltfm.c when changing sdhci-tegra driver.
Only the patch of sdhci-esdhc-imx are tested on hardware, and others are just build tested. And it is based on the tree below.
git://git.secretlab.ca/git/linux-2.6.git devicetree/test
Comments are welcomed and appreciated.
Regards, Shawn
Shawn Guo (5): mmc: sdhci: make sdhci-esdhc-imx driver self registered mmc: sdhci: make sdhci-cns3xxx driver self registered mmc: sdhci: make sdhci-dove driver self registered mmc: sdhci: make sdhci-tegra driver self registered mmc: sdhci: update Makefile/Kconfig for sdhci_pltfm change
drivers/mmc/host/Kconfig | 24 +++-- drivers/mmc/host/Makefile | 11 +- drivers/mmc/host/sdhci-cns3xxx.c | 68 +++++++++++++- drivers/mmc/host/sdhci-dove.c | 70 +++++++++++++- drivers/mmc/host/sdhci-esdhc-imx.c | 98 +++++++++++++++---- drivers/mmc/host/sdhci-pltfm.c | 165 ++----------------------------- drivers/mmc/host/sdhci-pltfm.h | 14 ++- drivers/mmc/host/sdhci-tegra.c | 189 +++++++++++++++++++++---------------
The patch turns the common stuff to in sdhci-pltfm.c into functions, and add sdhci-esdhc-imx its own .probe and .remove which in turn call into the common functions, so that sdhci-esdhc-imx driver registers itself and keep all sdhci-esdhc-imx specific things like sdhci_esdhc_imx_pdata away from sdhci-pltfm.c which is common.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/mmc/host/sdhci-esdhc-imx.c | 98 +++++++++++++++++++++++++++++------- drivers/mmc/host/sdhci-pltfm.c | 84 +++++++++++++++++++++++++++++-- drivers/mmc/host/sdhci-pltfm.h | 11 ++++- 3 files changed, 169 insertions(+), 24 deletions(-)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index 9b82910..6585620 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -100,15 +100,39 @@ static unsigned int esdhc_pltfm_get_min_clock(struct sdhci_host *host) return clk_get_rate(pltfm_host->clk) / 256 / 16; }
-static int esdhc_pltfm_init(struct sdhci_host *host, struct sdhci_pltfm_data *pdata) +static struct sdhci_ops sdhci_esdhc_ops = { + .read_w = esdhc_readw_le, + .write_w = esdhc_writew_le, + .write_b = esdhc_writeb_le, + .set_clock = esdhc_set_clock, + .get_max_clock = esdhc_pltfm_get_max_clock, + .get_min_clock = esdhc_pltfm_get_min_clock, +}; + +static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = { + .quirks = ESDHC_DEFAULT_QUIRKS | SDHCI_QUIRK_BROKEN_ADMA, + /* ADMA has issues. Might be fixable */ + .ops = &sdhci_esdhc_ops, +}; + +static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev) { - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_pltfm_host *pltfm_host; + struct sdhci_host *host; struct clk *clk; + int ret; + + host = sdhci_pltfm_init(pdev, &sdhci_esdhc_imx_pdata); + if (!host) + return -ENOMEM; + + pltfm_host = sdhci_priv(host);
clk = clk_get(mmc_dev(host->mmc), NULL); if (IS_ERR(clk)) { dev_err(mmc_dev(host->mmc), "clk err\n"); - return PTR_ERR(clk); + ret = PTR_ERR(clk); + goto err_clk_get; } clk_enable(clk); pltfm_host->clk = clk; @@ -120,30 +144,68 @@ static int esdhc_pltfm_init(struct sdhci_host *host, struct sdhci_pltfm_data *pd if (cpu_is_mx25() || cpu_is_mx35()) host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK;
+ ret = sdhci_add_host(host); + if (ret) + goto err_add_host; + return 0; + +err_add_host: + clk_disable(pltfm_host->clk); + clk_put(pltfm_host->clk); +err_clk_get: + sdhci_pltfm_free(pdev); + return ret; }
-static void esdhc_pltfm_exit(struct sdhci_host *host) +static int __devexit sdhci_esdhc_imx_remove(struct platform_device *pdev) { + struct sdhci_host *host = platform_get_drvdata(pdev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + int dead = 0; + u32 scratch; + + dead = 0; + scratch = readl(host->ioaddr + SDHCI_INT_STATUS); + if (scratch == (u32)-1) + dead = 1; + + sdhci_remove_host(host, dead);
clk_disable(pltfm_host->clk); clk_put(pltfm_host->clk); + + sdhci_pltfm_free(pdev); + + return 0; }
-static struct sdhci_ops sdhci_esdhc_ops = { - .read_w = esdhc_readw_le, - .write_w = esdhc_writew_le, - .write_b = esdhc_writeb_le, - .set_clock = esdhc_set_clock, - .get_max_clock = esdhc_pltfm_get_max_clock, - .get_min_clock = esdhc_pltfm_get_min_clock, +static struct platform_driver sdhci_esdhc_imx_driver = { + .driver = { + .name = "sdhci-esdhc-imx", + .owner = THIS_MODULE, + }, + .probe = sdhci_esdhc_imx_probe, + .remove = __devexit_p(sdhci_esdhc_imx_remove), +#ifdef CONFIG_PM + .suspend = sdhci_pltfm_suspend, + .resume = sdhci_pltfm_resume, +#endif };
-struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = { - .quirks = ESDHC_DEFAULT_QUIRKS | SDHCI_QUIRK_BROKEN_ADMA, - /* ADMA has issues. Might be fixable */ - .ops = &sdhci_esdhc_ops, - .init = esdhc_pltfm_init, - .exit = esdhc_pltfm_exit, -}; +static int __init sdhci_esdhc_imx_init(void) +{ + return platform_driver_register(&sdhci_esdhc_imx_driver); +} + +static void __exit sdhci_esdhc_imx_exit(void) +{ + platform_driver_unregister(&sdhci_esdhc_imx_driver); +} + +module_init(sdhci_esdhc_imx_init); +module_exit(sdhci_esdhc_imx_exit); + +MODULE_DESCRIPTION("SDHCI driver for i.MX eSDHC"); +MODULE_AUTHOR("Wolfram Sang w.sang@pengutronix.de"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c index ccc04ac..38b8cb4 100644 --- a/drivers/mmc/host/sdhci-pltfm.c +++ b/drivers/mmc/host/sdhci-pltfm.c @@ -44,6 +44,83 @@ static struct sdhci_ops sdhci_pltfm_ops = { };
+struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev, + struct sdhci_pltfm_data *pdata) +{ + struct sdhci_host *host; + struct sdhci_pltfm_host *pltfm_host; + struct resource *iomem; + int ret; + + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!iomem) { + ret = -ENOMEM; + goto err; + } + + if (resource_size(iomem) < 0x100) + dev_err(&pdev->dev, "Invalid iomem size!\n"); + + /* Some PCI-based MFD need the parent here */ + if (pdev->dev.parent != &platform_bus) + host = sdhci_alloc_host(pdev->dev.parent, sizeof(*pltfm_host)); + else + host = sdhci_alloc_host(&pdev->dev, sizeof(*pltfm_host)); + + if (IS_ERR(host)) { + ret = PTR_ERR(host); + goto err; + } + + pltfm_host = sdhci_priv(host); + + host->hw_name = "SDHCI"; + if (pdata && pdata->ops) + host->ops = pdata->ops; + else + host->ops = &sdhci_pltfm_ops; + if (pdata) + host->quirks = pdata->quirks; + host->irq = platform_get_irq(pdev, 0); + + if (!request_mem_region(iomem->start, resource_size(iomem), + mmc_hostname(host->mmc))) { + dev_err(&pdev->dev, "cannot request region\n"); + ret = -EBUSY; + goto err_request; + } + + host->ioaddr = ioremap(iomem->start, resource_size(iomem)); + if (!host->ioaddr) { + dev_err(&pdev->dev, "failed to remap registers\n"); + ret = -ENOMEM; + goto err_remap; + } + + platform_set_drvdata(pdev, host); + + return host; + +err_remap: + release_mem_region(iomem->start, resource_size(iomem)); +err_request: + sdhci_free_host(host); +err: + pr_err("%s failed %d\n", __func__, ret); + return NULL; +} + +void sdhci_pltfm_free(struct platform_device *pdev) +{ + struct sdhci_host *host = platform_get_drvdata(pdev); + struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + iounmap(host->ioaddr); + release_mem_region(iomem->start, resource_size(iomem)); + sdhci_free_host(host); + platform_set_drvdata(pdev, NULL); +} + /*****************************************************************************\ * * * Device probing/removal * @@ -198,9 +275,6 @@ static const struct platform_device_id sdhci_pltfm_ids[] = { #ifdef CONFIG_MMC_SDHCI_CNS3XXX { "sdhci-cns3xxx", (kernel_ulong_t)&sdhci_cns3xxx_pdata }, #endif -#ifdef CONFIG_MMC_SDHCI_ESDHC_IMX - { "sdhci-esdhc-imx", (kernel_ulong_t)&sdhci_esdhc_imx_pdata }, -#endif #ifdef CONFIG_MMC_SDHCI_DOVE { "sdhci-dove", (kernel_ulong_t)&sdhci_dove_pdata }, #endif @@ -212,14 +286,14 @@ static const struct platform_device_id sdhci_pltfm_ids[] = { MODULE_DEVICE_TABLE(platform, sdhci_pltfm_ids);
#ifdef CONFIG_PM -static int sdhci_pltfm_suspend(struct platform_device *dev, pm_message_t state) +int sdhci_pltfm_suspend(struct platform_device *dev, pm_message_t state) { struct sdhci_host *host = platform_get_drvdata(dev);
return sdhci_suspend_host(host, state); }
-static int sdhci_pltfm_resume(struct platform_device *dev) +int sdhci_pltfm_resume(struct platform_device *dev) { struct sdhci_host *host = platform_get_drvdata(dev);
diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h index c67523d..8357eba 100644 --- a/drivers/mmc/host/sdhci-pltfm.h +++ b/drivers/mmc/host/sdhci-pltfm.h @@ -13,6 +13,7 @@
#include <linux/clk.h> #include <linux/types.h> +#include <linux/platform_device.h> #include <linux/mmc/sdhci-pltfm.h>
struct sdhci_pltfm_host { @@ -21,9 +22,17 @@ struct sdhci_pltfm_host { };
extern struct sdhci_pltfm_data sdhci_cns3xxx_pdata; -extern struct sdhci_pltfm_data sdhci_esdhc_imx_pdata; extern struct sdhci_pltfm_data sdhci_dove_pdata; extern struct sdhci_pltfm_data sdhci_tegra_pdata; extern struct sdhci_pltfm_data sdhci_tegra_dt_pdata;
+struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev, + struct sdhci_pltfm_data *pdata); +void sdhci_pltfm_free(struct platform_device *pdev); + +#ifdef CONFIG_PM +int sdhci_pltfm_suspend(struct platform_device *dev, pm_message_t state); +int sdhci_pltfm_resume(struct platform_device *dev); +#endif + #endif /* _DRIVERS_MMC_SDHCI_PLTFM_H */
On Mon, Mar 21, 2011 at 04:06:59PM +0800, Shawn Guo wrote:
The patch turns the common stuff to in sdhci-pltfm.c into functions, and add sdhci-esdhc-imx its own .probe and .remove which in turn call into the common functions, so that sdhci-esdhc-imx driver registers itself and keep all sdhci-esdhc-imx specific things like sdhci_esdhc_imx_pdata away from sdhci-pltfm.c which is common.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Hi Shawn,
Thanks for all this work, I think it is the right thing to do. A few relatively minor comments below, but otherwise I like it.
g.
drivers/mmc/host/sdhci-esdhc-imx.c | 98 +++++++++++++++++++++++++++++------- drivers/mmc/host/sdhci-pltfm.c | 84 +++++++++++++++++++++++++++++-- drivers/mmc/host/sdhci-pltfm.h | 11 ++++-
I think this patch should be split in 2. One patch to refactor edhci-pltfm* to create the common utility functions, and one patch to convert the imx driver.
3 files changed, 169 insertions(+), 24 deletions(-)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index 9b82910..6585620 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -100,15 +100,39 @@ static unsigned int esdhc_pltfm_get_min_clock(struct sdhci_host *host) return clk_get_rate(pltfm_host->clk) / 256 / 16; } -static int esdhc_pltfm_init(struct sdhci_host *host, struct sdhci_pltfm_data *pdata) +static struct sdhci_ops sdhci_esdhc_ops = {
- .read_w = esdhc_readw_le,
- .write_w = esdhc_writew_le,
- .write_b = esdhc_writeb_le,
- .set_clock = esdhc_set_clock,
- .get_max_clock = esdhc_pltfm_get_max_clock,
- .get_min_clock = esdhc_pltfm_get_min_clock,
+};
+static struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
- .quirks = ESDHC_DEFAULT_QUIRKS | SDHCI_QUIRK_BROKEN_ADMA,
- /* ADMA has issues. Might be fixable */
- .ops = &sdhci_esdhc_ops,
+};
+static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev) {
- struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
- struct sdhci_pltfm_host *pltfm_host;
- struct sdhci_host *host; struct clk *clk;
- int ret;
- host = sdhci_pltfm_init(pdev, &sdhci_esdhc_imx_pdata);
Nice! I like that this adds type checking to the passing of the sdhci_pltfm_data pointer.
- if (!host)
return -ENOMEM;- pltfm_host = sdhci_priv(host);
clk = clk_get(mmc_dev(host->mmc), NULL); if (IS_ERR(clk)) { dev_err(mmc_dev(host->mmc), "clk err\n");
return PTR_ERR(clk);
ret = PTR_ERR(clk);goto err_clk_get;@@ -120,30 +144,68 @@ static int esdhc_pltfm_init(struct sdhci_host *host, struct sdhci_pltfm_data *pd if (cpu_is_mx25() || cpu_is_mx35()) host->quirks |= SDHCI_QUIRK_NO_MULTIBLOCK;
- ret = sdhci_add_host(host);
- if (ret)
goto err_add_host;- return 0;
+err_add_host:
- clk_disable(pltfm_host->clk);
- clk_put(pltfm_host->clk);
+err_clk_get:
- sdhci_pltfm_free(pdev);
- return ret;
} -static void esdhc_pltfm_exit(struct sdhci_host *host) +static int __devexit sdhci_esdhc_imx_remove(struct platform_device *pdev) {
- struct sdhci_host *host = platform_get_drvdata(pdev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
- int dead = 0;
- u32 scratch;
- dead = 0;
- scratch = readl(host->ioaddr + SDHCI_INT_STATUS);
- if (scratch == (u32)-1)
dead = 1;- sdhci_remove_host(host, dead);
clk_disable(pltfm_host->clk); clk_put(pltfm_host->clk);
- sdhci_pltfm_free(pdev);
- return 0;
} -static struct sdhci_ops sdhci_esdhc_ops = {
- .read_w = esdhc_readw_le,
- .write_w = esdhc_writew_le,
- .write_b = esdhc_writeb_le,
- .set_clock = esdhc_set_clock,
- .get_max_clock = esdhc_pltfm_get_max_clock,
- .get_min_clock = esdhc_pltfm_get_min_clock,
+static struct platform_driver sdhci_esdhc_imx_driver = {
- .driver = {
.name = "sdhci-esdhc-imx",.owner = THIS_MODULE,- },
- .probe = sdhci_esdhc_imx_probe,
- .remove = __devexit_p(sdhci_esdhc_imx_remove),
+#ifdef CONFIG_PM
- .suspend = sdhci_pltfm_suspend,
- .resume = sdhci_pltfm_resume,
+#endif }; -struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
- .quirks = ESDHC_DEFAULT_QUIRKS | SDHCI_QUIRK_BROKEN_ADMA,
- /* ADMA has issues. Might be fixable */
- .ops = &sdhci_esdhc_ops,
- .init = esdhc_pltfm_init,
- .exit = esdhc_pltfm_exit,
-}; +static int __init sdhci_esdhc_imx_init(void) +{
- return platform_driver_register(&sdhci_esdhc_imx_driver);
+}
+static void __exit sdhci_esdhc_imx_exit(void) +{
- platform_driver_unregister(&sdhci_esdhc_imx_driver);
+}
+module_init(sdhci_esdhc_imx_init); +module_exit(sdhci_esdhc_imx_exit);
Typically, I like to see module_init() and module_exit() immediately adjacent to the functions they register.
+MODULE_DESCRIPTION("SDHCI driver for i.MX eSDHC"); +MODULE_AUTHOR("Wolfram Sang w.sang@pengutronix.de"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c index ccc04ac..38b8cb4 100644 --- a/drivers/mmc/host/sdhci-pltfm.c +++ b/drivers/mmc/host/sdhci-pltfm.c @@ -44,6 +44,83 @@ static struct sdhci_ops sdhci_pltfm_ops = { }; +struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
struct sdhci_pltfm_data *pdata)
Since there is no chance of *pdata being passed via the pdev->dev.platform_data pointer (there aren't any users in mainline of that feature) then I would take the opportunity to rename this parameter and structure to eliminate any confusion. 'struct sdhci_pltfm' would be sufficient I think.
+{
- struct sdhci_host *host;
- struct sdhci_pltfm_host *pltfm_host;
- struct resource *iomem;
- int ret;
- iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!iomem) {
ret = -ENOMEM;goto err;- }
- if (resource_size(iomem) < 0x100)
dev_err(&pdev->dev, "Invalid iomem size!\n");- /* Some PCI-based MFD need the parent here */
- if (pdev->dev.parent != &platform_bus)
host = sdhci_alloc_host(pdev->dev.parent, sizeof(*pltfm_host));- else
host = sdhci_alloc_host(&pdev->dev, sizeof(*pltfm_host));- if (IS_ERR(host)) {
ret = PTR_ERR(host);goto err;- }
- pltfm_host = sdhci_priv(host);
- host->hw_name = "SDHCI";
- if (pdata && pdata->ops)
host->ops = pdata->ops;- else
host->ops = &sdhci_pltfm_ops;- if (pdata)
host->quirks = pdata->quirks;- host->irq = platform_get_irq(pdev, 0);
- if (!request_mem_region(iomem->start, resource_size(iomem),
mmc_hostname(host->mmc))) {dev_err(&pdev->dev, "cannot request region\n");ret = -EBUSY;goto err_request;- }
- host->ioaddr = ioremap(iomem->start, resource_size(iomem));
- if (!host->ioaddr) {
dev_err(&pdev->dev, "failed to remap registers\n");ret = -ENOMEM;goto err_remap;- }
- platform_set_drvdata(pdev, host);
- return host;
+err_remap:
- release_mem_region(iomem->start, resource_size(iomem));
+err_request:
- sdhci_free_host(host);
+err:
- pr_err("%s failed %d\n", __func__, ret);
- return NULL;
+}
Since the bulk of this function is a direct clone of the body of sdhci_pltfm_probe(), this patch should also modify sdhci_pltfm_probe() to use the new utility function. That will also make it clearer to readers that this new block is simply a refactor of the old.
+void sdhci_pltfm_free(struct platform_device *pdev) +{
- struct sdhci_host *host = platform_get_drvdata(pdev);
- struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- iounmap(host->ioaddr);
- release_mem_region(iomem->start, resource_size(iomem));
- sdhci_free_host(host);
- platform_set_drvdata(pdev, NULL);
+}
Ditto here for sdhci_pltfm_remove()
/*****************************************************************************\
*- Device probing/removal *
@@ -198,9 +275,6 @@ static const struct platform_device_id sdhci_pltfm_ids[] = { #ifdef CONFIG_MMC_SDHCI_CNS3XXX { "sdhci-cns3xxx", (kernel_ulong_t)&sdhci_cns3xxx_pdata }, #endif -#ifdef CONFIG_MMC_SDHCI_ESDHC_IMX
- { "sdhci-esdhc-imx", (kernel_ulong_t)&sdhci_esdhc_imx_pdata },
-#endif #ifdef CONFIG_MMC_SDHCI_DOVE { "sdhci-dove", (kernel_ulong_t)&sdhci_dove_pdata }, #endif @@ -212,14 +286,14 @@ static const struct platform_device_id sdhci_pltfm_ids[] = { MODULE_DEVICE_TABLE(platform, sdhci_pltfm_ids); #ifdef CONFIG_PM -static int sdhci_pltfm_suspend(struct platform_device *dev, pm_message_t state) +int sdhci_pltfm_suspend(struct platform_device *dev, pm_message_t state) { struct sdhci_host *host = platform_get_drvdata(dev); return sdhci_suspend_host(host, state); } -static int sdhci_pltfm_resume(struct platform_device *dev) +int sdhci_pltfm_resume(struct platform_device *dev) { struct sdhci_host *host = platform_get_drvdata(dev); diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h index c67523d..8357eba 100644 --- a/drivers/mmc/host/sdhci-pltfm.h +++ b/drivers/mmc/host/sdhci-pltfm.h @@ -13,6 +13,7 @@ #include <linux/clk.h> #include <linux/types.h> +#include <linux/platform_device.h> #include <linux/mmc/sdhci-pltfm.h> struct sdhci_pltfm_host { @@ -21,9 +22,17 @@ struct sdhci_pltfm_host { }; extern struct sdhci_pltfm_data sdhci_cns3xxx_pdata; -extern struct sdhci_pltfm_data sdhci_esdhc_imx_pdata; extern struct sdhci_pltfm_data sdhci_dove_pdata; extern struct sdhci_pltfm_data sdhci_tegra_pdata; extern struct sdhci_pltfm_data sdhci_tegra_dt_pdata; +struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
struct sdhci_pltfm_data *pdata);+void sdhci_pltfm_free(struct platform_device *pdev);
+#ifdef CONFIG_PM +int sdhci_pltfm_suspend(struct platform_device *dev, pm_message_t state); +int sdhci_pltfm_resume(struct platform_device *dev); +#endif
#endif /* _DRIVERS_MMC_SDHCI_PLTFM_H */
1.7.1
On Wed, Mar 23, 2011 at 10:28:58PM -0600, Grant Likely wrote:
On Mon, Mar 21, 2011 at 04:06:59PM +0800, Shawn Guo wrote:
The patch turns the common stuff to in sdhci-pltfm.c into functions, and add sdhci-esdhc-imx its own .probe and .remove which in turn call into the common functions, so that sdhci-esdhc-imx driver registers itself and keep all sdhci-esdhc-imx specific things like sdhci_esdhc_imx_pdata away from sdhci-pltfm.c which is common.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Hi Shawn,
Hi Grant,
Thanks for all this work, I think it is the right thing to do. A few relatively minor comments below, but otherwise I like it.
g.
drivers/mmc/host/sdhci-esdhc-imx.c | 98 +++++++++++++++++++++++++++++------- drivers/mmc/host/sdhci-pltfm.c | 84 +++++++++++++++++++++++++++++-- drivers/mmc/host/sdhci-pltfm.h | 11 ++++-
I think this patch should be split in 2. One patch to refactor edhci-pltfm* to create the common utility functions, and one patch to convert the imx driver.
I squashed this whole series into one patch in a relevant patch set I just posted out minutes ago, primarily to reduce the patch quantity and ease the bisect, since it's logically integral.
[...]
+static int __init sdhci_esdhc_imx_init(void) +{
- return platform_driver_register(&sdhci_esdhc_imx_driver);
+}
+static void __exit sdhci_esdhc_imx_exit(void) +{
- platform_driver_unregister(&sdhci_esdhc_imx_driver);
+}
+module_init(sdhci_esdhc_imx_init); +module_exit(sdhci_esdhc_imx_exit);
Typically, I like to see module_init() and module_exit() immediately adjacent to the functions they register.
Incorporated in the new patch set just posted ...
+MODULE_DESCRIPTION("SDHCI driver for i.MX eSDHC"); +MODULE_AUTHOR("Wolfram Sang w.sang@pengutronix.de"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c index ccc04ac..38b8cb4 100644 --- a/drivers/mmc/host/sdhci-pltfm.c +++ b/drivers/mmc/host/sdhci-pltfm.c @@ -44,6 +44,83 @@ static struct sdhci_ops sdhci_pltfm_ops = { }; +struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
struct sdhci_pltfm_data *pdata)Since there is no chance of *pdata being passed via the pdev->dev.platform_data pointer (there aren't any users in mainline of that feature) then I would take the opportunity to rename this parameter and structure to eliminate any confusion. 'struct sdhci_pltfm' would be sufficient I think.
The chance comes along with the new patch set (function sdhci_esdhc_probe in sdhci-esdhc.c). And I would not rename 'struct sdhci_pltfmi_data' to 'struct sdhci_pltfm' in terms of that we have another name of 'struct sdhci_pltfm_host'. But yes, pdata does confuse. Any suggestion? I can fix all of them with a follow-up patch against the new series.
+{
- struct sdhci_host *host;
- struct sdhci_pltfm_host *pltfm_host;
- struct resource *iomem;
- int ret;
- iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!iomem) {
ret = -ENOMEM;goto err;- }
- if (resource_size(iomem) < 0x100)
dev_err(&pdev->dev, "Invalid iomem size!\n");- /* Some PCI-based MFD need the parent here */
- if (pdev->dev.parent != &platform_bus)
host = sdhci_alloc_host(pdev->dev.parent, sizeof(*pltfm_host));- else
host = sdhci_alloc_host(&pdev->dev, sizeof(*pltfm_host));- if (IS_ERR(host)) {
ret = PTR_ERR(host);goto err;- }
- pltfm_host = sdhci_priv(host);
- host->hw_name = "SDHCI";
- if (pdata && pdata->ops)
host->ops = pdata->ops;- else
host->ops = &sdhci_pltfm_ops;- if (pdata)
host->quirks = pdata->quirks;- host->irq = platform_get_irq(pdev, 0);
- if (!request_mem_region(iomem->start, resource_size(iomem),
mmc_hostname(host->mmc))) {dev_err(&pdev->dev, "cannot request region\n");ret = -EBUSY;goto err_request;- }
- host->ioaddr = ioremap(iomem->start, resource_size(iomem));
- if (!host->ioaddr) {
dev_err(&pdev->dev, "failed to remap registers\n");ret = -ENOMEM;goto err_remap;- }
- platform_set_drvdata(pdev, host);
- return host;
+err_remap:
- release_mem_region(iomem->start, resource_size(iomem));
+err_request:
- sdhci_free_host(host);
+err:
- pr_err("%s failed %d\n", __func__, ret);
- return NULL;
+}
Since the bulk of this function is a direct clone of the body of sdhci_pltfm_probe(), this patch should also modify sdhci_pltfm_probe() to use the new utility function. That will also make it clearer to readers that this new block is simply a refactor of the old.
Good suggestion. Will take it next time where applies ...
The patch adds sdhci-cns3xxx its own .probe and .remove which in turn call into the common functions provided by sdhci-pltfm.c, so that sdhci-cns3xxx driver registers itself and keep all sdhci-cns3xxx specific things like sdhci_cns3xxx_pdata away from sdhci-pltfm.c which is common.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/mmc/host/sdhci-cns3xxx.c | 68 +++++++++++++++++++++++++++++++++++++- drivers/mmc/host/sdhci-pltfm.c | 3 -- drivers/mmc/host/sdhci-pltfm.h | 1 - 3 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/host/sdhci-cns3xxx.c b/drivers/mmc/host/sdhci-cns3xxx.c index 9ebd1d7..97d19cd 100644 --- a/drivers/mmc/host/sdhci-cns3xxx.c +++ b/drivers/mmc/host/sdhci-cns3xxx.c @@ -86,7 +86,7 @@ static struct sdhci_ops sdhci_cns3xxx_ops = { .set_clock = sdhci_cns3xxx_set_clock, };
-struct sdhci_pltfm_data sdhci_cns3xxx_pdata = { +static struct sdhci_pltfm_data sdhci_cns3xxx_pdata = { .ops = &sdhci_cns3xxx_ops, .quirks = SDHCI_QUIRK_BROKEN_DMA | SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK | @@ -95,3 +95,69 @@ struct sdhci_pltfm_data sdhci_cns3xxx_pdata = { SDHCI_QUIRK_BROKEN_TIMEOUT_VAL | SDHCI_QUIRK_NONSTANDARD_CLOCK, }; + +static int __devinit sdhci_cns3xxx_probe(struct platform_device *pdev) +{ + struct sdhci_host *host; + int ret; + + host = sdhci_pltfm_init(pdev, &sdhci_cns3xxx_pdata); + if (!host) + return -ENOMEM; + + ret = sdhci_add_host(host); + if (ret) + goto err_add_host; + + return 0; + +err_add_host: + sdhci_pltfm_free(pdev); + return ret; +} + +static int __devexit sdhci_cns3xxx_remove(struct platform_device *pdev) +{ + struct sdhci_host *host = platform_get_drvdata(pdev); + int dead = 0; + u32 scratch; + + scratch = readl(host->ioaddr + SDHCI_INT_STATUS); + if (scratch == (u32)-1) + dead = 1; + + sdhci_remove_host(pdev, dead); + sdhci_pltfm_free(pdev); + + return 0; +} + +static struct platform_driver sdhci_cns3xxx_driver = { + .driver = { + .name = "sdhci-cns3xxx", + .owner = THIS_MODULE, + }, + .probe = sdhci_cns3xxx_probe, + .remove = __devexit_p(sdhci_cns3xxx_remove), +#ifdef CONFIG_PM + .suspend = sdhci_pltfm_suspend, + .resume = sdhci_pltfm_resume, +#endif +}; + +static int __init sdhci_cns3xxx_init(void) +{ + return platform_driver_register(&sdhci_cns3xxx_driver); +} + +static void __exit sdhci_cns3xxx_exit(void) +{ + platform_driver_unregister(&sdhci_cns3xxx_driver); +} + +module_init(sdhci_cns3xxx_init); +module_exit(sdhci_cns3xxx_exit); + +MODULE_DESCRIPTION("SDHCI driver for CNS3xxx"); +MODULE_AUTHOR("Anton Vorontsov avorontsov@mvista.com"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c index 38b8cb4..b9a904d 100644 --- a/drivers/mmc/host/sdhci-pltfm.c +++ b/drivers/mmc/host/sdhci-pltfm.c @@ -272,9 +272,6 @@ static int __devexit sdhci_pltfm_remove(struct platform_device *pdev)
static const struct platform_device_id sdhci_pltfm_ids[] = { { "sdhci", }, -#ifdef CONFIG_MMC_SDHCI_CNS3XXX - { "sdhci-cns3xxx", (kernel_ulong_t)&sdhci_cns3xxx_pdata }, -#endif #ifdef CONFIG_MMC_SDHCI_DOVE { "sdhci-dove", (kernel_ulong_t)&sdhci_dove_pdata }, #endif diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h index 8357eba..493a8d0 100644 --- a/drivers/mmc/host/sdhci-pltfm.h +++ b/drivers/mmc/host/sdhci-pltfm.h @@ -21,7 +21,6 @@ struct sdhci_pltfm_host { u32 scratchpad; /* to handle quirks across io-accessor calls */ };
-extern struct sdhci_pltfm_data sdhci_cns3xxx_pdata; extern struct sdhci_pltfm_data sdhci_dove_pdata; extern struct sdhci_pltfm_data sdhci_tegra_pdata; extern struct sdhci_pltfm_data sdhci_tegra_dt_pdata;
The patch adds sdhci-dove its own .probe and .remove which in turn call into the common functions provided by sdhci-pltfm.c, so that sdhci-dove driver registers itself and keep all sdhci-dove specific things like sdhci_dove_pdata away from sdhci-pltfm.c which is common.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/mmc/host/sdhci-dove.c | 70 ++++++++++++++++++++++++++++++++++++++- drivers/mmc/host/sdhci-pltfm.c | 3 -- drivers/mmc/host/sdhci-pltfm.h | 1 - 3 files changed, 68 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/sdhci-dove.c b/drivers/mmc/host/sdhci-dove.c index 2aeef4f..e985338 100644 --- a/drivers/mmc/host/sdhci-dove.c +++ b/drivers/mmc/host/sdhci-dove.c @@ -3,7 +3,7 @@ * * Author: Saeed Bishara saeed@marvell.com * Mike Rapoport mike@compulab.co.il - * Based on sdhci-cns3xxx.c + * Based on sdhci-dove.c * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -61,10 +61,76 @@ static struct sdhci_ops sdhci_dove_ops = { .read_l = sdhci_dove_readl, };
-struct sdhci_pltfm_data sdhci_dove_pdata = { +static struct sdhci_pltfm_data sdhci_dove_pdata = { .ops = &sdhci_dove_ops, .quirks = SDHCI_QUIRK_NO_SIMULT_VDD_AND_POWER | SDHCI_QUIRK_NO_BUSY_IRQ | SDHCI_QUIRK_BROKEN_TIMEOUT_VAL | SDHCI_QUIRK_FORCE_DMA, }; + +static int __devinit sdhci_dove_probe(struct platform_device *pdev) +{ + struct sdhci_host *host; + int ret; + + host = sdhci_pltfm_init(pdev, &sdhci_dove_pdata); + if (!host) + return -ENOMEM; + + ret = sdhci_add_host(host); + if (ret) + goto err_add_host; + + return 0; + +err_add_host: + sdhci_pltfm_free(pdev); + return ret; +} + +static int __devexit sdhci_dove_remove(struct platform_device *pdev) +{ + struct sdhci_host *host = platform_get_drvdata(pdev); + int dead = 0; + u32 scratch; + + scratch = readl(host->ioaddr + SDHCI_INT_STATUS); + if (scratch == (u32)-1) + dead = 1; + + sdhci_remove_host(pdev, dead); + sdhci_pltfm_free(pdev); + + return 0; +} + +static struct platform_driver sdhci_dove_driver = { + .driver = { + .name = "sdhci-dove", + .owner = THIS_MODULE, + }, + .probe = sdhci_dove_probe, + .remove = __devexit_p(sdhci_dove_remove), +#ifdef CONFIG_PM + .suspend = sdhci_pltfm_suspend, + .resume = sdhci_pltfm_resume, +#endif +}; + +static int __init sdhci_dove_init(void) +{ + return platform_driver_register(&sdhci_dove_driver); +} + +static void __exit sdhci_dove_exit(void) +{ + platform_driver_unregister(&sdhci_dove_driver); +} + +module_init(sdhci_dove_init); +module_exit(sdhci_dove_exit); + +MODULE_DESCRIPTION("SDHCI driver for Dove"); +MODULE_AUTHOR("Saeed Bishara saeed@marvell.com"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c index b9a904d..c4bba33 100644 --- a/drivers/mmc/host/sdhci-pltfm.c +++ b/drivers/mmc/host/sdhci-pltfm.c @@ -272,9 +272,6 @@ static int __devexit sdhci_pltfm_remove(struct platform_device *pdev)
static const struct platform_device_id sdhci_pltfm_ids[] = { { "sdhci", }, -#ifdef CONFIG_MMC_SDHCI_DOVE - { "sdhci-dove", (kernel_ulong_t)&sdhci_dove_pdata }, -#endif #ifdef CONFIG_MMC_SDHCI_TEGRA { "sdhci-tegra", (kernel_ulong_t)&sdhci_tegra_pdata }, #endif diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h index 493a8d0..d7267f0 100644 --- a/drivers/mmc/host/sdhci-pltfm.h +++ b/drivers/mmc/host/sdhci-pltfm.h @@ -21,7 +21,6 @@ struct sdhci_pltfm_host { u32 scratchpad; /* to handle quirks across io-accessor calls */ };
-extern struct sdhci_pltfm_data sdhci_dove_pdata; extern struct sdhci_pltfm_data sdhci_tegra_pdata; extern struct sdhci_pltfm_data sdhci_tegra_dt_pdata;
The patch adds sdhci-tegra its own .probe and .remove which in turn call into the common functions provided by sdhci-pltfm.c, so that sdhci-tegra driver registers itself and keep all sdhci-tegra specific things like sdhci_tegra_pdata away from sdhci-pltfm.c which is common.
As sdhci-tegra is the last one remaining in sdhci-pltfm.c, with removing it out, the .probe and .remove in sdhci-pltfm.c together with other things like sdhci_dt_ids and sdhci_pltfm_ids can be cleaned up as well.
Also, having sdhci-tegra handle its own driver registration, the dt and non-dt support of sdhci-tegra can be consolidated into one pair of .probe and .remove hooks now.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/mmc/host/sdhci-pltfm.c | 213 +--------------------------------------- drivers/mmc/host/sdhci-pltfm.h | 3 - drivers/mmc/host/sdhci-tegra.c | 189 +++++++++++++++++++++--------------- 3 files changed, 112 insertions(+), 293 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c index c4bba33..c300234 100644 --- a/drivers/mmc/host/sdhci-pltfm.c +++ b/drivers/mmc/host/sdhci-pltfm.c @@ -22,25 +22,11 @@ * Inspired by sdhci-pci.c, by Pierre Ossman */
-#include <linux/delay.h> -#include <linux/highmem.h> -#include <linux/mod_devicetable.h> -#include <linux/platform_device.h> - -#include <linux/mmc/host.h> - -#include <linux/io.h> -#include <linux/mmc/sdhci-pltfm.h> +#include <linux/err.h>
#include "sdhci.h" #include "sdhci-pltfm.h"
-/*****************************************************************************\ - * * - * SDHCI core callbacks * - * * -*****************************************************************************/ - static struct sdhci_ops sdhci_pltfm_ops = { };
@@ -121,164 +107,6 @@ void sdhci_pltfm_free(struct platform_device *pdev) platform_set_drvdata(pdev, NULL); }
-/*****************************************************************************\ - * * - * Device probing/removal * - * * -*****************************************************************************/ -#if defined(CONFIG_OF) -#include <linux/of_device.h> -static const struct of_device_id sdhci_dt_ids[] = { - { .compatible = "nvidia,tegra250-sdhci", .data = &sdhci_tegra_dt_pdata }, - { } -}; -MODULE_DEVICE_TABLE(platform, sdhci_dt_ids); - -static const struct of_device_id *sdhci_get_of_device_id(struct platform_device *pdev) -{ - return of_match_device(sdhci_dt_ids, &pdev->dev); -} -#else -#define shdci_dt_ids NULL -static inline struct of_device_id *sdhci_get_of_device_id(struct platform_device *pdev) -{ - return NULL; -} -#endif - -static int __devinit sdhci_pltfm_probe(struct platform_device *pdev) -{ - const struct platform_device_id *platid = platform_get_device_id(pdev); - const struct of_device_id *dtid = sdhci_get_of_device_id(pdev); - struct sdhci_pltfm_data *pdata; - struct sdhci_host *host; - struct sdhci_pltfm_host *pltfm_host; - struct resource *iomem; - int ret; - - if (platid && platid->driver_data) - pdata = (void *)platid->driver_data; - else if (dtid && dtid->data) - pdata = dtid->data; - else - pdata = pdev->dev.platform_data; - - iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!iomem) { - ret = -ENOMEM; - goto err; - } - - if (resource_size(iomem) < 0x100) - dev_err(&pdev->dev, "Invalid iomem size. You may " - "experience problems.\n"); - - /* Some PCI-based MFD need the parent here */ - if (pdev->dev.parent != &platform_bus) - host = sdhci_alloc_host(pdev->dev.parent, sizeof(*pltfm_host)); - else - host = sdhci_alloc_host(&pdev->dev, sizeof(*pltfm_host)); - - if (IS_ERR(host)) { - ret = PTR_ERR(host); - goto err; - } - - pltfm_host = sdhci_priv(host); - - host->hw_name = "platform"; - if (pdata && pdata->ops) - host->ops = pdata->ops; - else - host->ops = &sdhci_pltfm_ops; - if (pdata) - host->quirks = pdata->quirks; - host->irq = platform_get_irq(pdev, 0); - - if (!request_mem_region(iomem->start, resource_size(iomem), - mmc_hostname(host->mmc))) { - dev_err(&pdev->dev, "cannot request region\n"); - ret = -EBUSY; - goto err_request; - } - - host->ioaddr = ioremap(iomem->start, resource_size(iomem)); - if (!host->ioaddr) { - dev_err(&pdev->dev, "failed to remap registers\n"); - ret = -ENOMEM; - goto err_remap; - } - - if (pdata && pdata->init) { - ret = pdata->init(host, pdata); - if (ret) - goto err_plat_init; - } - - ret = sdhci_add_host(host); - if (ret) - goto err_add_host; - - platform_set_drvdata(pdev, host); - - return 0; - -err_add_host: - if (pdata && pdata->exit) - pdata->exit(host); -err_plat_init: - iounmap(host->ioaddr); -err_remap: - release_mem_region(iomem->start, resource_size(iomem)); -err_request: - sdhci_free_host(host); -err: - printk(KERN_ERR"Probing of sdhci-pltfm failed: %d\n", ret); - return ret; -} - -static int __devexit sdhci_pltfm_remove(struct platform_device *pdev) -{ - const struct platform_device_id *platid = platform_get_device_id(pdev); - const struct of_device_id *dtid = sdhci_get_of_device_id(pdev); - struct sdhci_pltfm_data *pdata; - struct sdhci_host *host = platform_get_drvdata(pdev); - struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); - int dead; - u32 scratch; - - if (platid && platid->driver_data) - pdata = (void *)platid->driver_data; - else if (dtid && dtid->data) - pdata = dtid->data; - else - pdata = pdev->dev.platform_data; - - dead = 0; - scratch = readl(host->ioaddr + SDHCI_INT_STATUS); - if (scratch == (u32)-1) - dead = 1; - - sdhci_remove_host(host, dead); - if (pdata && pdata->exit) - pdata->exit(host); - iounmap(host->ioaddr); - release_mem_region(iomem->start, resource_size(iomem)); - sdhci_free_host(host); - platform_set_drvdata(pdev, NULL); - - return 0; -} - -static const struct platform_device_id sdhci_pltfm_ids[] = { - { "sdhci", }, -#ifdef CONFIG_MMC_SDHCI_TEGRA - { "sdhci-tegra", (kernel_ulong_t)&sdhci_tegra_pdata }, -#endif - { }, -}; -MODULE_DEVICE_TABLE(platform, sdhci_pltfm_ids); - #ifdef CONFIG_PM int sdhci_pltfm_suspend(struct platform_device *dev, pm_message_t state) { @@ -293,43 +121,4 @@ int sdhci_pltfm_resume(struct platform_device *dev)
return sdhci_resume_host(host); } -#else -#define sdhci_pltfm_suspend NULL -#define sdhci_pltfm_resume NULL #endif /* CONFIG_PM */ - -static struct platform_driver sdhci_pltfm_driver = { - .driver = { - .name = "sdhci", - .owner = THIS_MODULE, - .of_match_table = sdhci_dt_ids, - }, - .probe = sdhci_pltfm_probe, - .remove = __devexit_p(sdhci_pltfm_remove), - .id_table = sdhci_pltfm_ids, - .suspend = sdhci_pltfm_suspend, - .resume = sdhci_pltfm_resume, -}; - -/*****************************************************************************\ - * * - * Driver init/exit * - * * -*****************************************************************************/ - -static int __init sdhci_drv_init(void) -{ - return platform_driver_register(&sdhci_pltfm_driver); -} - -static void __exit sdhci_drv_exit(void) -{ - platform_driver_unregister(&sdhci_pltfm_driver); -} - -module_init(sdhci_drv_init); -module_exit(sdhci_drv_exit); - -MODULE_DESCRIPTION("Secure Digital Host Controller Interface platform driver"); -MODULE_AUTHOR("Mocean Laboratories info@mocean-labs.com"); -MODULE_LICENSE("GPL v2"); diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h index d7267f0..b53b976 100644 --- a/drivers/mmc/host/sdhci-pltfm.h +++ b/drivers/mmc/host/sdhci-pltfm.h @@ -21,9 +21,6 @@ struct sdhci_pltfm_host { u32 scratchpad; /* to handle quirks across io-accessor calls */ };
-extern struct sdhci_pltfm_data sdhci_tegra_pdata; -extern struct sdhci_pltfm_data sdhci_tegra_dt_pdata; - struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev, struct sdhci_pltfm_data *pdata); void sdhci_pltfm_free(struct platform_device *pdev); diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index c3d6f83..c376e74 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -119,19 +119,58 @@ static int tegra_sdhci_8bit(struct sdhci_host *host, int bus_width) }
-static int tegra_sdhci_pltfm_init(struct sdhci_host *host, - struct sdhci_pltfm_data *pdata) +static struct sdhci_ops tegra_sdhci_ops = { + .get_ro = tegra_sdhci_get_ro, + .read_l = tegra_sdhci_readl, + .read_w = tegra_sdhci_readw, + .write_l = tegra_sdhci_writel, + .platform_8bit_width = tegra_sdhci_8bit, +}; + +static struct sdhci_pltfm_data sdhci_tegra_pdata = { + .quirks = SDHCI_QUIRK_BROKEN_TIMEOUT_VAL | + SDHCI_QUIRK_SINGLE_POWER_WRITE | + SDHCI_QUIRK_NO_HISPD_BIT | + SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC, + .ops = &tegra_sdhci_ops, +}; + +static int __devinit sdhci_tegra_probe(struct platform_device *pdev) { - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); - struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc)); + struct sdhci_pltfm_host *pltfm_host; struct tegra_sdhci_platform_data *plat; + struct sdhci_host *host; struct clk *clk; int rc;
+ host = sdhci_pltfm_init(pdev, &sdhci_tegra_pdata); + if (!host) + return -ENOMEM; + + pltfm_host = sdhci_priv(host); + plat = pdev->dev.platform_data; + +#ifdef CONFIG_OF + plat = kzalloc(sizeof(*plat), GFP_KERNEL); + if (!plat) { + rc = -ENOMEM; + goto err_no_plat; + } + pdev->dev.platform_data = plat; + + plat->cd_gpio = of_get_gpio(pdev->dev.of_node, 0); + plat->wp_gpio = of_get_gpio(pdev->dev.of_node, 1); + plat->power_gpio = of_get_gpio(pdev->dev.of_node, 2); + + dev_info(&pdev->dev, "using gpios cd=%i, wp=%i power=%i\n", + plat->cd_gpio, plat->wp_gpio, plat->power_gpio); +#endif + if (plat == NULL) { dev_err(mmc_dev(host->mmc), "missing platform data\n"); - return -ENXIO; + rc = -ENXIO; + goto err_no_plat; }
if (gpio_is_valid(plat->power_gpio)) { @@ -139,7 +178,7 @@ static int tegra_sdhci_pltfm_init(struct sdhci_host *host, if (rc) { dev_err(mmc_dev(host->mmc), "failed to allocate power gpio\n"); - goto out; + goto err_power_req; } tegra_gpio_enable(plat->power_gpio); gpio_direction_output(plat->power_gpio, 1); @@ -150,7 +189,7 @@ static int tegra_sdhci_pltfm_init(struct sdhci_host *host, if (rc) { dev_err(mmc_dev(host->mmc), "failed to allocate cd gpio\n"); - goto out_power; + goto err_cd_req; } tegra_gpio_enable(plat->cd_gpio); gpio_direction_input(plat->cd_gpio); @@ -161,7 +200,7 @@ static int tegra_sdhci_pltfm_init(struct sdhci_host *host,
if (rc) { dev_err(mmc_dev(host->mmc), "request irq error\n"); - goto out_cd; + goto err_cd_irq_req; }
} @@ -171,7 +210,7 @@ static int tegra_sdhci_pltfm_init(struct sdhci_host *host, if (rc) { dev_err(mmc_dev(host->mmc), "failed to allocate wp gpio\n"); - goto out_cd; + goto err_wp_req; } tegra_gpio_enable(plat->wp_gpio); gpio_direction_input(plat->wp_gpio); @@ -181,7 +220,7 @@ static int tegra_sdhci_pltfm_init(struct sdhci_host *host, if (IS_ERR(clk)) { dev_err(mmc_dev(host->mmc), "clk err\n"); rc = PTR_ERR(clk); - goto out_wp; + goto err_clk_get; } clk_enable(clk); pltfm_host->clk = clk; @@ -189,62 +228,55 @@ static int tegra_sdhci_pltfm_init(struct sdhci_host *host, if (plat->is_8bit) host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+ rc = sdhci_add_host(host); + if (rc) + goto err_add_host; + return 0;
-out_wp: +err_add_host: + clk_disable(pltfm_host->clk); + clk_put(pltfm_host->clk); +err_clk_get: if (gpio_is_valid(plat->wp_gpio)) { tegra_gpio_disable(plat->wp_gpio); gpio_free(plat->wp_gpio); } - -out_cd: +err_wp_req: + if (gpio_is_valid(plat->cd_gpio)) { + free_irq(gpio_to_irq(plat->cd_gpio), host); + } +err_cd_irq_req: if (gpio_is_valid(plat->cd_gpio)) { tegra_gpio_disable(plat->cd_gpio); gpio_free(plat->cd_gpio); } - -out_power: +err_cd_req: if (gpio_is_valid(plat->power_gpio)) { tegra_gpio_disable(plat->power_gpio); gpio_free(plat->power_gpio); } - -out: +err_power_req: +#ifdef CONFIG_OF + kfree(plat); +#endif +err_no_plat: + sdhci_pltfm_free(pdev); return rc; }
-static int tegra_sdhci_pltfm_dt_init(struct sdhci_host *host, - struct sdhci_pltfm_data *pdata) -{ - struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc)); - struct tegra_sdhci_platform_data *plat; - - if (pdev->dev.platform_data) { - dev_err(&pdev->dev, "%s: platform_data not NULL; aborting\n", - __func__); - return -ENODEV; - } - - plat = kzalloc(sizeof(*plat), GFP_KERNEL); - if (!plat) - return -ENOMEM; - pdev->dev.platform_data = plat; - - plat->cd_gpio = of_get_gpio(pdev->dev.of_node, 0); - plat->wp_gpio = of_get_gpio(pdev->dev.of_node, 1); - plat->power_gpio = of_get_gpio(pdev->dev.of_node, 2); - - dev_info(&pdev->dev, "using gpios cd=%i, wp=%i power=%i\n", - plat->cd_gpio, plat->wp_gpio, plat->power_gpio); - - return tegra_sdhci_pltfm_init(host, pdata); -} - -static void tegra_sdhci_pltfm_exit(struct sdhci_host *host) +static int __devexit sdhci_tegra_remove(struct platform_device *pdev) { + struct sdhci_host *host = platform_get_drvdata(pdev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); - struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc)); struct tegra_sdhci_platform_data *plat; + int dead = 0; + u32 scratch; + + scratch = readl(host->ioaddr + SDHCI_INT_STATUS); + if (scratch == (u32)-1) + dead = 1; + sdhci_remove_host(host, dead);
plat = pdev->dev.platform_data;
@@ -263,44 +295,45 @@ static void tegra_sdhci_pltfm_exit(struct sdhci_host *host) gpio_free(plat->power_gpio); }
+#ifdef CONFIG_OF + kfree(pdev->dev.platform_data); + pdev->dev.platform_data = NULL; +#endif + clk_disable(pltfm_host->clk); clk_put(pltfm_host->clk); -} - -static void tegra_sdhci_pltfm_dt_exit(struct sdhci_host *host) -{ - struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
- tegra_sdhci_pltfm_exit(host); + sdhci_pltfm_free(pdev);
- kfree(pdev->dev.platform_data); - pdev->dev.platform_data = NULL; + return 0; }
-static struct sdhci_ops tegra_sdhci_ops = { - .get_ro = tegra_sdhci_get_ro, - .read_l = tegra_sdhci_readl, - .read_w = tegra_sdhci_readw, - .write_l = tegra_sdhci_writel, - .platform_8bit_width = tegra_sdhci_8bit, +static struct platform_driver sdhci_tegra_driver = { + .driver = { + .name = "sdhci-tegra", + .owner = THIS_MODULE, + }, + .probe = sdhci_tegra_probe, + .remove = __devexit_p(sdhci_tegra_remove), +#ifdef CONFIG_PM + .suspend = sdhci_pltfm_suspend, + .resume = sdhci_pltfm_resume, +#endif };
-struct sdhci_pltfm_data sdhci_tegra_pdata = { - .quirks = SDHCI_QUIRK_BROKEN_TIMEOUT_VAL | - SDHCI_QUIRK_SINGLE_POWER_WRITE | - SDHCI_QUIRK_NO_HISPD_BIT | - SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC, - .ops = &tegra_sdhci_ops, - .init = tegra_sdhci_pltfm_init, - .exit = tegra_sdhci_pltfm_exit, -}; +static int __init sdhci_tegra_init(void) +{ + return platform_driver_register(&sdhci_tegra_driver); +}
-struct sdhci_pltfm_data sdhci_tegra_dt_pdata = { - .quirks = SDHCI_QUIRK_BROKEN_TIMEOUT_VAL | - SDHCI_QUIRK_SINGLE_POWER_WRITE | - SDHCI_QUIRK_NO_HISPD_BIT | - SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC, - .ops = &tegra_sdhci_ops, - .init = tegra_sdhci_pltfm_dt_init, - .exit = tegra_sdhci_pltfm_dt_exit, -}; +static void __exit sdhci_tegra_exit(void) +{ + platform_driver_unregister(&sdhci_tegra_driver); +} + +module_init(sdhci_tegra_init); +module_exit(sdhci_tegra_exit); + +MODULE_DESCRIPTION("SDHCI driver for Tegra"); +MODULE_AUTHOR(" Google, Inc."); +MODULE_LICENSE("GPL v2");
The sdhci_pltfm.c becomes a file only having common support functions than a driver with its registeration before. The patch is to update Makefile and Kconfig to let MMC_SDHCI_PLTFM be selected by specific SDHCI device drivers.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/mmc/host/Kconfig | 24 +++++++++++++----------- drivers/mmc/host/Makefile | 11 +++++------ 2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index afe8c6f..1db9347 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -113,20 +113,17 @@ config MMC_SDHCI_OF_HLWD If unsure, say N.
config MMC_SDHCI_PLTFM - tristate "SDHCI support on the platform specific bus" + bool depends on MMC_SDHCI help - This selects the platform specific bus support for Secure Digital Host - Controller Interface. - - If you have a controller with this interface, say Y or M here. - - If unsure, say N. + This selects the platform common function support for Secure Digital + Host Controller Interface.
config MMC_SDHCI_CNS3XXX bool "SDHCI support on the Cavium Networks CNS3xxx SoC" depends on ARCH_CNS3XXX - depends on MMC_SDHCI_PLTFM + depends on MMC_SDHCI + select MMC_SDHCI_PLTFM help This selects the SDHCI support for CNS3xxx System-on-Chip devices.
@@ -134,7 +131,9 @@ config MMC_SDHCI_CNS3XXX
config MMC_SDHCI_ESDHC_IMX bool "SDHCI platform support for the Freescale eSDHC i.MX controller" - depends on MMC_SDHCI_PLTFM && (ARCH_MX25 || ARCH_MX35 || ARCH_MX5) + depends on ARCH_MX25 || ARCH_MX35 || ARCH_MX5 + depends on MMC_SDHCI + select MMC_SDHCI_PLTFM select MMC_SDHCI_IO_ACCESSORS help This selects the Freescale eSDHC controller support on the platform @@ -145,7 +144,8 @@ config MMC_SDHCI_ESDHC_IMX config MMC_SDHCI_DOVE bool "SDHCI support on Marvell's Dove SoC" depends on ARCH_DOVE - depends on MMC_SDHCI_PLTFM + depends on MMC_SDHCI + select MMC_SDHCI_PLTFM select MMC_SDHCI_IO_ACCESSORS help This selects the Secure Digital Host Controller Interface in @@ -155,7 +155,9 @@ config MMC_SDHCI_DOVE
config MMC_SDHCI_TEGRA tristate "SDHCI platform support for the Tegra SD/MMC Controller" - depends on MMC_SDHCI_PLTFM && ARCH_TEGRA + depends on ARCH_TEGRA + depends on MMC_SDHCI + select MMC_SDHCI_PLTFM select MMC_SDHCI_IO_ACCESSORS help This selects the Tegra SD/MMC controller. If you have a Tegra diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile index e834fb2..1d8e43d 100644 --- a/drivers/mmc/host/Makefile +++ b/drivers/mmc/host/Makefile @@ -36,12 +36,11 @@ obj-$(CONFIG_MMC_SH_MMCIF) += sh_mmcif.o obj-$(CONFIG_MMC_JZ4740) += jz4740_mmc.o obj-$(CONFIG_MMC_USHC) += ushc.o
-obj-$(CONFIG_MMC_SDHCI_PLTFM) += sdhci-platform.o -sdhci-platform-y := sdhci-pltfm.o -sdhci-platform-$(CONFIG_MMC_SDHCI_CNS3XXX) += sdhci-cns3xxx.o -sdhci-platform-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o -sdhci-platform-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o -sdhci-platform-$(CONFIG_MMC_SDHCI_TEGRA) += sdhci-tegra.o +obj-$(CONFIG_MMC_SDHCI_PLTFM) += sdhci-pltfm.o +obj-$(CONFIG_MMC_SDHCI_CNS3XXX) += sdhci-cns3xxx.o +obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o +obj-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o +obj-$(CONFIG_MMC_SDHCI_TEGRA) += sdhci-tegra.o
obj-$(CONFIG_MMC_SDHCI_OF) += sdhci-of.o sdhci-of-y := sdhci-of-core.o
On Monday 21 March 2011, Shawn Guo wrote:
This patch set is to take sdhci device driver specific things out from sdhci-pltfm.c and make them self registered. Here are the differences it makes.
- Get the sdhci device driver follow the Linux trend that driver take the registration by its own
- sdhci-pltfm.c becomes significantly simpler and only has common support functions there
- All sdhci device specific stuff are going back its own driver
- The dt and non-dt support share the same pair of .probe and .remove hooks.
The first one patch adds common support function into sdhci-pltfm.c when changing sdhci-esdhc-imx driver, while the last one cleans up sdhci-pltfm.c when changing sdhci-tegra driver.
Only the patch of sdhci-esdhc-imx are tested on hardware, and others are just build tested. And it is based on the tree below.
git://git.secretlab.ca/git/linux-2.6.git devicetree/test
Comments are welcomed and appreciated.
Hi Shawn,
The changes you make look very nice, great work there!
I think the split of the series into five patches is not ideal because you have more interdependencies than necessary, and it's not clear that the series can be safely bisected, especially with the way you do all the Kconfig/Makefile updates at the end.
A nicer way to partition the series would be
1. Add sdhci_pltfm_init helper 2-5. Convert each of the users, including the respective Kconfig/Makefile changes 6. remove obsolete code from sdhci-pltfm.c
As I said, that's just a detail and the end result is the same. If you don't get any other feedback and Chris is happy with the changes, I wouldn't expect you to reshuffle all the patches just for this.
Acked-by: Arnd Bergmann arnd@arndb.de
On Mon, Mar 21, 2011 at 01:39:45PM +0100, Arnd Bergmann wrote: [...]
Hi Shawn,
Hi Arnd,
The changes you make look very nice, great work there!
I think the split of the series into five patches is not ideal because you have more interdependencies than necessary, and it's not clear that the series can be safely bisected, especially with the way you do all the Kconfig/Makefile updates at the end.
A nicer way to partition the series would be
- Add sdhci_pltfm_init helper
2-5. Convert each of the users, including the respective Kconfig/Makefile changes 6. remove obsolete code from sdhci-pltfm.c
As I said, that's just a detail and the end result is the same. If you don't get any other feedback and Chris is happy with the changes, I wouldn't expect you to reshuffle all the patches just for this.
Acked-by: Arnd Bergmann arnd@arndb.de
Actually, I knew there should be better partition of the patch set, and it just did not come into my mind :) You suggestion does look better. I would take it in the next spin.
Thanks for the suggestion.
If you don't get any other feedback and Chris is happy with the
I would like to have a look at it, please give me a few days.
-
Arnd Bergmann -
Grant Likely -
Shawn Guo -
Shawn Guo -
Wolfram Sang