Hi James, thanks for the reply. On Wed, Nov 30, 2011 at 2:56 AM, James Hogan <[email protected]> wrote: > Hi, > > Thanks for the update. It's better, but there's still a few issues... > > On Wed, Nov 30, 2011 at 12:00:37AM +0530, Shashidhar Hiremath wrote: >> Support of PCI mode for the dw_mmc driver This Patch adds the support for >> the scenario where the Synopsys Designware IP is present on the PCI bus.The >> patch adds the minimal modifications necessary for the driver to work on PCI >> platform.Also added separate files for PCI and PLATFORM modes of operation > > Being picky, but full stop at end of sentences, and a space between > sentences would be nice. Wrapping to 72 characters is nice too. :) > >> >> Signed-off-by: Shashidhar Hiremath <[email protected]> >> --- >> v2: >> *As per Suggestions by Will Newton and James Hogan >> -Reduced the number of ifdefs >> v3: >> *As per Suggestions by Will Newton and James Hogan >> -Added separate files for PCI and PLATFORM Modes similar to SDHCI driver >> drivers/mmc/host/Kconfig | 23 ++++++ >> drivers/mmc/host/Makefile | 2 + >> drivers/mmc/host/dw_mmc-pci.c | 159 >> +++++++++++++++++++++++++++++++++++++++ >> drivers/mmc/host/dw_mmc-pltfm.c | 125 ++++++++++++++++++++++++++++++ >> drivers/mmc/host/dw_mmc.c | 155 >> ++++++++++++++------------------------ >> drivers/mmc/host/dw_mmc.h | 7 ++ >> include/linux/mmc/dw_mmc.h | 4 +- >> 7 files changed, 374 insertions(+), 101 deletions(-) >> create mode 100644 drivers/mmc/host/dw_mmc-pci.c >> create mode 100644 drivers/mmc/host/dw_mmc-pltfm.c >> >> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig >> index 87d5067..6735fbe 100644 >> --- a/drivers/mmc/host/Kconfig >> +++ b/drivers/mmc/host/Kconfig >> @@ -534,6 +534,29 @@ config MMC_DW_IDMAC >> Designware Mobile Storage IP block. This disables the external DMA >> interface. >> >> +config MMC_DW_PCI >> + tristate "Synopsys Designware MCI support on PCI bus" >> + depends on MMC_DW && PCI >> + help >> + This selects the PCI bus for the Synopsys Designware Mobile Storage >> IP. >> + Select this option if the IP is present on PCI platform. >> + >> + If you have a controller with this interface, say Y or M here. >> + >> + If unsure, say N. >> + >> +config MMC_DW_PLTFM >> + tristate "platform driver helper for Synopsys Designware Mobile >> Storage IP" >> + depends on MMC_DW >> + help >> + This selects the common helper functions support for Host Controller >> + Interface based platform driver.Please select this option if the IP >> + is present as a platform device. >> + >> + If you have a controller with this interface, say Y or M here. >> + >> + If unsure, say N. >> + >> config MMC_SH_MMCIF >> tristate "SuperH Internal MMCIF support" >> depends on MMC_BLOCK && (SUPERH || ARCH_SHMOBILE) >> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile >> index b4b83f3..3aa2fa3 100644 >> --- a/drivers/mmc/host/Makefile >> +++ b/drivers/mmc/host/Makefile >> @@ -38,6 +38,8 @@ obj-$(CONFIG_MMC_CB710) += cb710-mmc.o >> obj-$(CONFIG_MMC_VIA_SDMMC) += via-sdmmc.o >> obj-$(CONFIG_SDH_BFIN) += bfin_sdh.o >> obj-$(CONFIG_MMC_DW) += dw_mmc.o >> +obj-$(CONFIG_MMC_DW_PLTFM) += dw_mmc-pltfm.o >> +obj-$(CONFIG_MMC_DW_PCI) += dw_mmc-pci.o >> obj-$(CONFIG_MMC_SH_MMCIF) += sh_mmcif.o >> obj-$(CONFIG_MMC_JZ4740) += jz4740_mmc.o >> obj-$(CONFIG_MMC_VUB300) += vub300.o >> diff --git a/drivers/mmc/host/dw_mmc-pci.c b/drivers/mmc/host/dw_mmc-pci.c >> new file mode 100644 >> index 0000000..7d8fb5e >> --- /dev/null >> +++ b/drivers/mmc/host/dw_mmc-pci.c >> @@ -0,0 +1,159 @@ >> +/* >> + * Synopsys DesignWare Multimedia Card PCI Interface driver >> + * >> + * Copyright (C) 2011 Vayavya Labs Pvt. Ltd. >> + * >> + * 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, or >> + * (at your option) any later version. >> + */ >> + >> +#include <linux/interrupt.h> >> +#include <linux/irq.h> >> +#include <linux/pci.h> >> +#include <linux/mmc/host.h> >> +#include <linux/mmc/mmc.h> >> +#include <linux/mmc/dw_mmc.h> >> +#include "dw_mmc.h" >> + >> +#define PCI_BAR_NO 2 >> +#define COMPLETE_BAR 0 >> +#define DW_MCI_VENDOR_ID 0x700 >> +#define DW_MCI_DEVICE_ID 0x1107 >> +/* Defining the Capabilities */ >> +#define DW_MCI_CAPABILITIES (MMC_CAP_4_BIT_DATA | MMC_CAP_MMC_HIGHSPEED |\ >> + MMC_CAP_SD_HIGHSPEED | MMC_CAP_8_BIT_DATA |\ >> + MMC_CAP_SDIO_IRQ) >> + >> +static struct dw_mci_board pci_board_data = { >> + .num_slots = 1, >> + .caps = DW_MCI_CAPABILITIES, >> + .bus_hz = 33 * 1000 * 1000, >> + .detect_delay_ms = 200, >> + .fifo_depth = 32, >> +}; >> + >> +static int __devinit dw_mci_pci_probe(struct pci_dev *pdev, >> + const struct pci_device_id *entries) >> +{ >> + struct dw_mci *host; >> + int ret; >> + >> + ret = pci_enable_device(pdev); >> + if (ret) >> + return ret; >> + if (pci_request_regions(pdev, "dw_mmc_pci")) { >> + ret = -ENODEV; >> + goto err_freehost; > > This will kfree the as yet uninitialised host. > > Should this (and later error handlers) also undo the effects of > pci_enable_device with pci_disable_device? > Yes , Will make the change >> + } >> + >> + host = kzalloc(sizeof(struct dw_mci), GFP_KERNEL); >> + if (!host) >> + return -ENOMEM; >> + >> + host->irq = pdev->irq; >> + host->dev = pdev->dev; >> + host->pdata = &pci_board_data; >> + >> + ret = -ENOMEM; > > what's this for? > This has somehow creeped from the old code.Will clean it up. >> + host->regs = pci_iomap(pdev, PCI_BAR_NO, COMPLETE_BAR); >> + if (!host->regs) { >> + pci_release_regions(pdev); >> + ret = -EIO; >> + goto err_freehost; >> + } >> + >> + >> + pci_set_drvdata(pdev, host); >> + ret = dw_mci_probe(host); >> + if (ret) >> +err_freehost: >> + kfree(host); > > pci_iounmap? > will add this. >> + return ret; > > Since there are several things that need cleaning up on failure, it > should really have the error handling labels after the return like a lot > of other kernel drivers do (e.g. see ioapic_probe() in > drivers/pci/ioapic.c). This makes the code a lot neater and easier to > read. > >> +} >> + >> + >> + >> + >> +static void __devexit dw_mci_pci_remove(struct pci_dev *pdev) >> +{ >> + > > no need for a newline here > >> + struct dw_mci *host = pci_get_drvdata(pdev); >> + >> + mci_writel(host, RINTSTS, 0xFFFFFFFF); >> + mci_writel(host, INTMASK, 0); /* disable all mmc interrupt first */ >> + >> + pci_set_drvdata(pdev, NULL); >> + dw_mci_remove(host); >> + pci_release_regions(pdev); >> + kfree(host); > > should this also be pci_iounmap'ing, and pci_disable_device'ing? > Yes, will make the change. >> +} >> + >> +#ifdef CONFIG_PM >> +static int dw_mci_pci_suspend(struct pci_dev *pdev, pm_message_t mesg) >> +{ >> + int ret; >> + struct dw_mci *host = pci_get_drvdata(pdev); >> + >> + ret = dw_mci_suspend(host); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +/* >> + * TODO: we should probably disable the clock to the card in the suspend >> path. >> + */ >> + >> + >> +static int dw_mci_pci_resume(struct pci_dev *pdev) >> +{ >> + int ret; >> + struct dw_mci *host = pci_get_drvdata(pdev); >> + >> + ret = dw_mci_resume(host); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +#else >> +#define dw_mci_pci_suspend NULL >> +#define dw_mci_pci_resume NULL >> +#endif /* CONFIG_PM */ >> + >> +static DEFINE_PCI_DEVICE_TABLE(dw_mci_pci_id) = { >> + { PCI_DEVICE(DW_MCI_DEVICE_ID, DW_MCI_VENDOR_ID) }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(pci, dw_mci_pci_id); >> + >> +static struct pci_driver dw_mci_pci_driver = { >> + .name = "dw_mmc_pci", >> + .id_table = dw_mci_pci_id, >> + .probe = dw_mci_pci_probe, >> + .remove = dw_mci_pci_remove, >> + .suspend = dw_mci_pci_suspend, >> + .resume = dw_mci_pci_resume, >> +}; >> + >> +static int __init dw_mci_init(void) >> +{ >> + return pci_register_driver(&dw_mci_pci_driver); >> +} >> + >> +static void __exit dw_mci_exit(void) >> +{ >> + pci_unregister_driver(&dw_mci_pci_driver); >> +} >> + >> +module_init(dw_mci_init); >> +module_exit(dw_mci_exit); >> + >> +MODULE_DESCRIPTION("DW Multimedia Card PCI Interface driver"); >> +MODULE_AUTHOR("Shashidhar Hiremath <[email protected]>"); >> +MODULE_AUTHOR("VayavyaLabs Pvt. Ltd."); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c >> b/drivers/mmc/host/dw_mmc-pltfm.c >> new file mode 100644 >> index 0000000..0c6a95df >> --- /dev/null >> +++ b/drivers/mmc/host/dw_mmc-pltfm.c >> @@ -0,0 +1,125 @@ >> +/* >> + * Synopsys DesignWare Multimedia Card Interface driver >> + * >> + * Copyright (C) 2009 NXP Semiconductors >> + * Copyright (C) 2009, 2010 Imagination Technologies Ltd. >> + * >> + * 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, or >> + * (at your option) any later version. >> + */ >> + >> +#include <linux/interrupt.h> >> +#include <linux/irq.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/mmc/host.h> >> +#include <linux/mmc/mmc.h> >> +#include <linux/mmc/dw_mmc.h> >> +#include "dw_mmc.h" >> + >> +static int dw_mci_pltfm_probe(struct platform_device *pdev) >> +{ >> + struct dw_mci *host; >> + struct resource *regs; >> + int ret; > > no need for the extra space before ret. > >> + >> + host = kzalloc(sizeof(struct dw_mci), GFP_KERNEL); >> + if (!host) >> + return -ENOMEM; >> + >> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!regs) >> + return -ENXIO; >> + >> + host->irq = platform_get_irq(pdev, 0); >> + if (host->irq < 0) >> + return host->irq; >> + >> + host->dev = pdev->dev; >> + host->pdata = pdev->dev.platform_data; >> + ret = -ENOMEM; >> + host->regs = ioremap(regs->start, resource_size(regs)); >> + if (!host->regs) >> + goto err_freehost; >> + platform_set_drvdata(pdev, host); >> + ret = dw_mci_probe(host); >> + if (ret) >> +err_freehost: >> + kfree(host); >> + return ret; > > same stuff again, please fix the error handling. > will do the change. >> +} >> + >> +static int __exit dw_mci_pltfm_remove(struct platform_device *pdev) >> +{ >> + struct dw_mci *host = platform_get_drvdata(pdev); >> + >> + mci_writel(host, RINTSTS, 0xFFFFFFFF); >> + mci_writel(host, INTMASK, 0); /* disable all mmc interrupt first */ >> + >> + platform_set_drvdata(pdev, NULL); >> + dw_mci_remove(host); >> + kfree(host); > > iounmap (see below)? > >> + return 0; >> +} >> +#ifdef CONFIG_PM >> +/* >> + * TODO: we should probably disable the clock to the card in the suspend >> path. >> + */ >> +static int dw_mci_pltfm_suspend(struct platform_device *pdev, pm_message_t >> mesg) >> +{ >> + >> + int ret; >> + struct dw_mci *host = platform_get_drvdata(pdev); >> + >> + ret = dw_mci_suspend(host); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int dw_mci_pltfm_resume(struct platform_device *pdev) >> +{ >> + >> + int ret; >> + struct dw_mci *host = platform_get_drvdata(pdev); >> + >> + ret = dw_mci_resume(host); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> +#else >> +#define dw_mci_pltfm_suspend NULL >> +#define dw_mci_pltfm_resume NULL >> +#endif /* CONFIG_PM */ >> + >> +static struct platform_driver dw_mci_pltfm_driver = { >> + .remove = __exit_p(dw_mci_pltfm_remove), >> + .suspend = dw_mci_pltfm_suspend, >> + .resume = dw_mci_pltfm_resume, >> + .driver = { >> + .name = "dw_mmc", >> + }, >> +}; >> + >> +static int __init dw_mci_init(void) >> +{ >> + return platform_driver_probe(&dw_mci_pltfm_driver, dw_mci_pltfm_probe); >> +} >> + >> +static void __exit dw_mci_exit(void) >> +{ >> + platform_driver_unregister(&dw_mci_pltfm_driver); >> +} >> + >> +module_init(dw_mci_init); >> +module_exit(dw_mci_exit); >> + >> +MODULE_DESCRIPTION("DW Multimedia Card Interface driver"); >> +MODULE_AUTHOR("NXP Semiconductor VietNam"); >> +MODULE_AUTHOR("Imagination Technologies Ltd"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 3aaeb08..7d33626 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -269,7 +269,7 @@ static void dw_mci_start_command(struct dw_mci *host, >> struct mmc_command *cmd, u32 cmd_flags) >> { >> host->cmd = cmd; >> - dev_vdbg(&host->pdev->dev, >> + dev_vdbg(&host->dev, >> "start command: ARGR=0x%08x CMDR=0x%08x\n", >> cmd->arg, cmd_flags); >> >> @@ -302,7 +302,7 @@ static void dw_mci_dma_cleanup(struct dw_mci *host) >> struct mmc_data *data = host->data; >> >> if (data) >> - dma_unmap_sg(&host->pdev->dev, data->sg, data->sg_len, >> + dma_unmap_sg(&host->dev, data->sg, data->sg_len, >> ((data->flags & MMC_DATA_WRITE) >> ? DMA_TO_DEVICE : DMA_FROM_DEVICE)); >> } >> @@ -327,7 +327,7 @@ static void dw_mci_idmac_complete_dma(struct dw_mci >> *host) >> { >> struct mmc_data *data = host->data; >> >> - dev_vdbg(&host->pdev->dev, "DMA complete\n"); >> + dev_vdbg(&host->dev, "DMA complete\n"); >> >> host->dma_ops->cleanup(host); >> >> @@ -463,10 +463,10 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, >> struct mmc_data *data) >> else >> direction = DMA_TO_DEVICE; >> >> - sg_len = dma_map_sg(&host->pdev->dev, data->sg, data->sg_len, >> + sg_len = dma_map_sg(&host->dev, data->sg, data->sg_len, >> direction); >> >> - dev_vdbg(&host->pdev->dev, >> + dev_vdbg(&host->dev, >> "sd sg_cpu: %#lx sg_dma: %#lx sg_len: %d\n", >> (unsigned long)host->sg_cpu, (unsigned long)host->sg_dma, >> sg_len); >> @@ -716,6 +716,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct >> mmc_ios *ios) >> switch (ios->power_mode) { >> case MMC_POWER_UP: >> set_bit(DW_MMC_CARD_NEED_INIT, &slot->flags); >> + mci_writel(slot->host, PWREN, (0x1 << (slot->id))); > > Was this something that the original driver should have been doing? (it > might be best as a separate patch if so). > Yes, I will submit another patch on this. > Does it need to be undone somewhere? > > Won't it unintentionally power down all other slots by writing 0 to > them? > > Do you "wait for regulator/switch ramp-up time before trying to > initialize card" (quoting TRM) or is that handled by other code? > >> break; >> default: >> break; >> @@ -804,12 +805,12 @@ static void dw_mci_request_end(struct dw_mci *host, >> struct mmc_request *mrq) >> slot = list_entry(host->queue.next, >> struct dw_mci_slot, queue_node); >> list_del(&slot->queue_node); >> - dev_vdbg(&host->pdev->dev, "list not empty: %s is next\n", >> + dev_vdbg(&host->dev, "list not empty: %s is next\n", >> mmc_hostname(slot->mmc)); >> host->state = STATE_SENDING_CMD; >> dw_mci_start_request(host, slot); >> } else { >> - dev_vdbg(&host->pdev->dev, "list empty\n"); >> + dev_vdbg(&host->dev, "list empty\n"); >> host->state = STATE_IDLE; >> } >> >> @@ -941,7 +942,7 @@ static void dw_mci_tasklet_func(unsigned long priv) >> data->bytes_xfered = 0; >> data->error = -ETIMEDOUT; >> } else { >> - dev_err(&host->pdev->dev, >> + dev_err(&host->dev, >> "data FIFO error " >> "(status=%08x)\n", >> status); >> @@ -1651,7 +1652,7 @@ static int __init dw_mci_init_slot(struct dw_mci >> *host, unsigned int id) >> struct mmc_host *mmc; >> struct dw_mci_slot *slot; >> >> - mmc = mmc_alloc_host(sizeof(struct dw_mci_slot), &host->pdev->dev); >> + mmc = mmc_alloc_host(sizeof(struct dw_mci_slot), &host->dev); >> if (!mmc) >> return -ENOMEM; >> >> @@ -1757,10 +1758,10 @@ static void dw_mci_cleanup_slot(struct dw_mci_slot >> *slot, unsigned int id) >> static void dw_mci_init_dma(struct dw_mci *host) >> { >> /* Alloc memory for sg translation */ >> - host->sg_cpu = dma_alloc_coherent(&host->pdev->dev, PAGE_SIZE, >> + host->sg_cpu = dma_alloc_coherent(&host->dev, PAGE_SIZE, >> &host->sg_dma, GFP_KERNEL); >> if (!host->sg_cpu) { >> - dev_err(&host->pdev->dev, "%s: could not alloc DMA memory\n", >> + dev_err(&host->dev, "%s: could not alloc DMA memory\n", >> __func__); >> goto no_dma; >> } >> @@ -1768,7 +1769,7 @@ static void dw_mci_init_dma(struct dw_mci *host) >> /* Determine which DMA interface to use */ >> #ifdef CONFIG_MMC_DW_IDMAC >> host->dma_ops = &dw_mci_idmac_ops; >> - dev_info(&host->pdev->dev, "Using internal DMA controller.\n"); >> + dev_info(&host->dev, "Using internal DMA controller.\n"); >> #endif >> >> if (!host->dma_ops) >> @@ -1776,12 +1777,12 @@ static void dw_mci_init_dma(struct dw_mci *host) >> >> if (host->dma_ops->init) { >> if (host->dma_ops->init(host)) { >> - dev_err(&host->pdev->dev, "%s: Unable to initialize " >> + dev_err(&host->dev, "%s: Unable to initialize " >> "DMA Controller.\n", __func__); >> goto no_dma; >> } >> } else { >> - dev_err(&host->pdev->dev, "DMA initialization not found.\n"); >> + dev_err(&host->dev, "DMA initialization not found.\n"); >> goto no_dma; >> } >> >> @@ -1789,7 +1790,7 @@ static void dw_mci_init_dma(struct dw_mci *host) >> return; >> >> no_dma: >> - dev_info(&host->pdev->dev, "Using PIO mode.\n"); >> + dev_info(&host->dev, "Using PIO mode.\n"); >> host->use_dma = 0; >> return; >> } >> @@ -1815,61 +1816,37 @@ static bool mci_wait_reset(struct device *dev, >> struct dw_mci *host) >> return false; >> } >> >> -static int dw_mci_probe(struct platform_device *pdev) >> +int dw_mci_probe(struct dw_mci *host) >> { >> - struct dw_mci *host; >> - struct resource *regs; >> - struct dw_mci_board *pdata; >> - int irq, ret, i, width; >> + int width, i , ret; > > no need for a space after the i. > >> u32 fifo_size; >> >> - regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> - if (!regs) >> - return -ENXIO; >> - >> - irq = platform_get_irq(pdev, 0); >> - if (irq < 0) >> - return irq; >> - >> - host = kzalloc(sizeof(struct dw_mci), GFP_KERNEL); >> - if (!host) >> - return -ENOMEM; >> - >> - host->pdev = pdev; >> - host->pdata = pdata = pdev->dev.platform_data; >> - if (!pdata || !pdata->init) { >> - dev_err(&pdev->dev, >> + if (!host->pdata || !host->pdata->init) { >> + dev_err(&host->dev, >> "Platform data must supply init function\n"); >> - ret = -ENODEV; >> - goto err_freehost; >> + return -ENODEV; >> } >> >> - if (!pdata->select_slot && pdata->num_slots > 1) { >> - dev_err(&pdev->dev, >> + if (!host->pdata->select_slot && host->pdata->num_slots > 1) { >> + dev_err(&host->dev, >> "Platform data must supply select_slot function\n"); >> - ret = -ENODEV; >> - goto err_freehost; >> + return -ENODEV; >> } >> >> - if (!pdata->bus_hz) { >> - dev_err(&pdev->dev, >> + if (!host->pdata->bus_hz) { >> + dev_err(&host->dev, >> "Platform data must supply bus speed\n"); >> - ret = -ENODEV; >> - goto err_freehost; >> + return -ENODEV; >> } >> >> - host->bus_hz = pdata->bus_hz; >> - host->quirks = pdata->quirks; >> + host->bus_hz = host->pdata->bus_hz; >> + host->quirks = host->pdata->quirks; >> >> spin_lock_init(&host->lock); >> INIT_LIST_HEAD(&host->queue); >> >> - ret = -ENOMEM; >> - host->regs = ioremap(regs->start, resource_size(regs)); >> - if (!host->regs) >> - goto err_freehost; >> >> - host->dma_ops = pdata->dma_ops; >> + host->dma_ops = host->pdata->dma_ops; >> dw_mci_init_dma(host); >> >> /* >> @@ -1899,7 +1876,7 @@ static int dw_mci_probe(struct platform_device *pdev) >> } >> >> /* Reset all blocks */ >> - if (!mci_wait_reset(&pdev->dev, host)) { >> + if (!mci_wait_reset(&host->dev, host)) { >> ret = -ENODEV; >> goto err_dmaunmap; >> } >> @@ -1942,13 +1919,13 @@ static int dw_mci_probe(struct platform_device *pdev) >> if (!dw_mci_card_workqueue) >> goto err_dmaunmap; >> INIT_WORK(&host->card_work, dw_mci_work_routine_card); >> - >> - ret = request_irq(irq, dw_mci_interrupt, 0, "dw-mci", host); >> +#ifdef CONFIG_MMC_DW_PCI >> + ret = request_irq(host->irq, dw_mci_interrupt, IRQF_SHARED, "dw-mci", >> host); >> +#else >> + ret = request_irq(host->irq, dw_mci_interrupt, 0, "dw-mci", host); >> +#endif > > is this necessary? perhaps it could pass in the irq flags from the > caller somehow. > >> if (ret) >> goto err_workqueue; >> - >> - platform_set_drvdata(pdev, host); >> - >> if (host->pdata->num_slots) >> host->num_slots = host->pdata->num_slots; >> else >> @@ -1985,12 +1962,12 @@ static int dw_mci_probe(struct platform_device *pdev) >> DW_MCI_ERROR_FLAGS | SDMMC_INT_CD); >> mci_writel(host, CTRL, SDMMC_CTRL_INT_ENABLE); /* Enable mci interrupt >> */ >> >> - dev_info(&pdev->dev, "DW MMC controller at irq %d, " >> + dev_info(&host->dev, "DW MMC controller at irq %d, " >> "%d bit host data width, " >> "%u deep fifo\n", >> - irq, width, fifo_size); >> + host->irq, width, fifo_size); >> if (host->quirks & DW_MCI_QUIRK_IDMAC_DTO) >> - dev_info(&pdev->dev, "Internal DMAC interrupt fix enabled.\n"); >> + dev_info(&host->dev, "Internal DMAC interrupt fix enabled.\n"); >> >> return 0; >> >> @@ -2001,7 +1978,7 @@ err_init_slot: >> dw_mci_cleanup_slot(host->slot[i], i); >> i--; >> } >> - free_irq(irq, host); >> + free_irq(host->irq, host); >> >> err_workqueue: >> destroy_workqueue(dw_mci_card_workqueue); >> @@ -2009,7 +1986,7 @@ err_workqueue: >> err_dmaunmap: >> if (host->use_dma && host->dma_ops->exit) >> host->dma_ops->exit(host); >> - dma_free_coherent(&host->pdev->dev, PAGE_SIZE, >> + dma_free_coherent(&host->dev, PAGE_SIZE, >> host->sg_cpu, host->sg_dma); >> iounmap(host->regs); > > this iounmap can be moved out to caller now that regs it initialised by > caller. > >> >> @@ -2017,25 +1994,16 @@ err_dmaunmap: >> regulator_disable(host->vmmc); >> regulator_put(host->vmmc); >> } >> - >> - >> -err_freehost: >> - kfree(host); >> return ret; >> } >> +EXPORT_SYMBOL(dw_mci_probe); >> >> -static int __exit dw_mci_remove(struct platform_device *pdev) >> +void dw_mci_remove(struct dw_mci *host) >> { >> - struct dw_mci *host = platform_get_drvdata(pdev); >> int i; >> >> - mci_writel(host, RINTSTS, 0xFFFFFFFF); >> - mci_writel(host, INTMASK, 0); /* disable all mmc interrupt first */ > > This seems pretty independent of PCI/platform, and is done by both > callers anyway. Perhaps it should stay where it is. > >> - >> - platform_set_drvdata(pdev, NULL); >> - >> for (i = 0; i < host->num_slots; i++) { >> - dev_dbg(&pdev->dev, "remove slot %d\n", i); >> + dev_dbg(&host->dev, "remove slot %d\n", i); >> if (host->slot[i]) >> dw_mci_cleanup_slot(host->slot[i], i); >> } >> @@ -2044,9 +2012,9 @@ static int __exit dw_mci_remove(struct platform_device >> *pdev) >> mci_writel(host, CLKENA, 0); >> mci_writel(host, CLKSRC, 0); >> >> - free_irq(platform_get_irq(pdev, 0), host); >> + free_irq(host->irq, host); >> destroy_workqueue(dw_mci_card_workqueue); >> - dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma); >> + dma_free_coherent(&host->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma); >> >> if (host->use_dma && host->dma_ops->exit) >> host->dma_ops->exit(host); >> @@ -2057,19 +2025,18 @@ static int __exit dw_mci_remove(struct >> platform_device *pdev) >> } >> >> iounmap(host->regs); > > same here (move out to caller) > >> - >> - kfree(host); >> - return 0; >> } >> +EXPORT_SYMBOL(dw_mci_remove); >> + >> + >> >> #ifdef CONFIG_PM >> /* >> * TODO: we should probably disable the clock to the card in the suspend >> path. >> */ >> -static int dw_mci_suspend(struct platform_device *pdev, pm_message_t mesg) >> +int dw_mci_suspend(struct dw_mci *host) >> { >> int i, ret; >> - struct dw_mci *host = platform_get_drvdata(pdev); >> >> for (i = 0; i < host->num_slots; i++) { >> struct dw_mci_slot *slot = host->slot[i]; >> @@ -2091,19 +2058,18 @@ static int dw_mci_suspend(struct platform_device >> *pdev, pm_message_t mesg) >> >> return 0; >> } >> +EXPORT_SYMBOL(dw_mci_suspend); >> >> -static int dw_mci_resume(struct platform_device *pdev) >> +int dw_mci_resume(struct dw_mci *host) >> { >> int i, ret; >> - struct dw_mci *host = platform_get_drvdata(pdev); >> - >> if (host->vmmc) >> regulator_enable(host->vmmc); >> >> if (host->dma_ops->init) >> host->dma_ops->init(host); >> >> - if (!mci_wait_reset(&pdev->dev, host)) { >> + if (!mci_wait_reset(&host->dev, host)) { >> ret = -ENODEV; >> return ret; >> } >> @@ -2125,31 +2091,22 @@ static int dw_mci_resume(struct platform_device >> *pdev) >> if (ret < 0) >> return ret; >> } >> - >> return 0; >> } >> +EXPORT_SYMBOL(dw_mci_resume); >> #else >> #define dw_mci_suspend NULL >> #define dw_mci_resume NULL > > this else bit can probably go since the #defines aren't being used > any more. > >> #endif /* CONFIG_PM */ >> >> -static struct platform_driver dw_mci_driver = { >> - .remove = __exit_p(dw_mci_remove), >> - .suspend = dw_mci_suspend, >> - .resume = dw_mci_resume, >> - .driver = { >> - .name = "dw_mmc", >> - }, >> -}; >> - >> static int __init dw_mci_init(void) >> { >> - return platform_driver_probe(&dw_mci_driver, dw_mci_probe); >> + printk(KERN_INFO "Synopsys Designware Multimedia Card Interface >> Driver"); >> + return 0; >> } >> >> static void __exit dw_mci_exit(void) >> { >> - platform_driver_unregister(&dw_mci_driver); >> } >> >> module_init(dw_mci_init); > > if the init and exit functions of the main driver no longer do anything, > they can probably go too. Actually, when I looked at the SDHCI-PCI drivers, these are still present because the the plain sdhci.ko can be loaded as an independent module, after that we can insert either pltfm.ko module or pci.ko module. So I feel it would be better to keep them as it is. > >> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h >> index 72c071f..70078f6 100644 >> --- a/drivers/mmc/host/dw_mmc.h >> +++ b/drivers/mmc/host/dw_mmc.h >> @@ -175,4 +175,11 @@ >> (*(volatile u64 __force *)((dev)->regs + SDMMC_##reg) = (value)) >> #endif >> >> +extern int dw_mci_probe(struct dw_mci *host); >> +extern void dw_mci_remove(struct dw_mci *host); >> +#ifdef CONFIG_PM >> +extern int dw_mci_suspend(struct dw_mci *host); >> +extern int dw_mci_resume(struct dw_mci *host); >> +#endif >> + >> #endif /* _DW_MMC_H_ */ >> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h >> index 6dc9b80..5240446 100644 >> --- a/include/linux/mmc/dw_mmc.h >> +++ b/include/linux/mmc/dw_mmc.h >> @@ -74,7 +74,7 @@ struct mmc_data; >> * @num_slots: Number of slots available. >> * @verid: Denote Version ID. >> * @data_offset: Set the offset of DATA register according to VERID. >> - * @pdev: Platform device associated with the MMC controller. >> + * @dev: device associated with the MMC controller. > > I'm being picky here, but you could capitalise the d of device, for > consistency with the rest of the descriptions. > >> * @pdata: Platform data associated with the MMC controller. >> * @slot: Slots sharing this MMC controller. >> * @fifo_depth: depth of FIFO. >> @@ -151,7 +151,7 @@ struct dw_mci { >> u32 fifoth_val; >> u16 verid; >> u16 data_offset; >> - struct platform_device *pdev; >> + struct device *dev; >> struct dw_mci_board *pdata; >> struct dw_mci_slot *slot[MAX_MCI_SLOTS]; >> >> -- >> 1.7.6.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >> the body of a message to [email protected] >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > Thanks > James
-- regards, Shashidhar Hiremath -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
