On Sat, May 12, 2012 at 10:44:42AM -0400, Chris Ball wrote: > Hi Shawn, just minor language changes, > > On Mon, May 07 2012, Shawn Guo wrote: > > It adds device tree probe support for mxs-mmc driver. > > > > Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx> > > --- > > Documentation/devicetree/bindings/mmc/mxs-mmc.txt | 26 +++++++++++ > > drivers/mmc/host/mxs-mmc.c | 48 ++++++++++++++++++-- > > 2 files changed, 69 insertions(+), 5 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/mmc/mxs-mmc.txt > > > > diff --git a/Documentation/devicetree/bindings/mmc/mxs-mmc.txt b/Documentation/devicetree/bindings/mmc/mxs-mmc.txt > > new file mode 100644 > > index 0000000..d7c2a40 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mmc/mxs-mmc.txt > > @@ -0,0 +1,26 @@ > > +* Freescale MXS MMC controller > > + > > +The Freescale MXS Synchronous Serial Ports (SSP) can act as a MMC controller > > +to support MMC, SD, and SDIO types of memory cards. > > + > > +Required properties: > > +- compatible: Should be "fsl,<chip>-mmc". The supported chips include > > + imx23 and imx28. > > +- reg: Should contain registers location and length > > +- interrupts: Should contain ERROR and DMA interrupts > > +- fsl,ssp-dma-channel: APBH DMA channel for the SSP > > +- fsl,bus-width: Number of data lines, can be <1>, <4>, or <8> > > Please don't use a prefix on "bus-width" -- see Arnd's proposed bindings: > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/091993.html > Yes, I've been keeping my eyes on the discussion. Before that generic bindings get landed, the vendor prefix can be treated as a marker that this is something should use generic binding. > > + > > +Optional properties: > > +- wp-gpios: Specify GPIOs for write protection > > + > > +Examples: > > + > > +ssp0: ssp@80010000 { > > + compatible = "fsl,imx28-mmc"; > > + reg = <0x80010000 2000>; > > + interrupts = <96 82>; > > + fsl,ssp-dma-channel = <0>; > > + fsl,bus-width = <8>; > > + status = "okay"; > > +}; > > You list a "status" property but it isn't mentioned in the required or > optional properties section, or used in the code. What's it for? > This is something part of DT core, and does not really belong to mxs-mmc bindings, so remove it from the example. > > diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c > > index 66f792a..c6b56b4 100644 > > --- a/drivers/mmc/host/mxs-mmc.c > > +++ b/drivers/mmc/host/mxs-mmc.c > > @@ -23,6 +23,9 @@ > > #include <linux/kernel.h> > > #include <linux/init.h> > > #include <linux/ioport.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/of_gpio.h> > > #include <linux/platform_device.h> > > #include <linux/delay.h> > > #include <linux/interrupt.h> > > @@ -685,8 +688,18 @@ static struct platform_device_id mxs_mmc_ids[] = { > > }; > > MODULE_DEVICE_TABLE(platform, mxs_mmc_ids); > > > > +static const struct of_device_id mxs_mmc_dt_ids[] = { > > + { .compatible = "fsl,imx23-mmc", .data = (void *) IMX23_MMC, }, > > + { .compatible = "fsl,imx28-mmc", .data = (void *) IMX28_MMC, }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, mxs_mmc_dt_ids); > > + > > static int mxs_mmc_probe(struct platform_device *pdev) > > { > > + const struct of_device_id *of_id = > > + of_match_device(mxs_mmc_dt_ids, &pdev->dev); > > + struct device_node *np = pdev->dev.of_node; > > struct mxs_mmc_host *host; > > struct mmc_host *mmc; > > struct resource *iores, *dmares; > > @@ -699,7 +712,7 @@ static int mxs_mmc_probe(struct platform_device *pdev) > > dmares = platform_get_resource(pdev, IORESOURCE_DMA, 0); > > irq_err = platform_get_irq(pdev, 0); > > irq_dma = platform_get_irq(pdev, 1); > > - if (!iores || !dmares || irq_err < 0 || irq_dma < 0) > > + if (!iores || irq_err < 0 || irq_dma < 0) > > return -EINVAL; > > > > mmc = mmc_alloc_host(sizeof(struct mxs_mmc_host), &pdev->dev); > > @@ -713,15 +726,31 @@ static int mxs_mmc_probe(struct platform_device *pdev) > > goto out_mmc_free; > > } > > > > - host->devid = pdev->id_entry->driver_data; > > + if (np) { > > + host->devid = (enum mxs_mmc_id) of_id->data; > > + /* > > + * TODO: This is a temporary solution and should be changed > > + * to use generic DMA binding later when the helplers get in. > > "helpers" > Fixed. Thanks for the catch. > > + */ > > + ret = of_property_read_u32(np, "fsl,ssp-dma-channel", > > + &host->dma_channel); > > + if (ret) { > > + dev_err(mmc_dev(host->mmc), > > + "failed to get dma channel\n"); > > + goto out_mmc_free; > > + } > > + } else { > > + host->devid = pdev->id_entry->driver_data; > > + host->dma_channel = dmares->start; > > + } > > + > > host->mmc = mmc; > > - host->dma_channel = dmares->start; > > host->sdio_irq_en = 0; > > > > pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > > if (IS_ERR(pinctrl)) { > > ret = PTR_ERR(pinctrl); > > - goto out_iounmap; > > + goto out_mmc_free; > > } > > > > host->clk = clk_get(&pdev->dev, NULL); > > @@ -749,7 +778,15 @@ static int mxs_mmc_probe(struct platform_device *pdev) > > MMC_CAP_SDIO_IRQ | MMC_CAP_NEEDS_POLL; > > > > pdata = mmc_dev(host->mmc)->platform_data; > > - if (pdata) { > > + if (!pdata) { > > + u32 bus_width = 0; > > + of_property_read_u32(np, "fsl,bus-width", &bus_width); > > + if (bus_width == 4) > > + mmc->caps |= MMC_CAP_4_BIT_DATA; > > + else if (bus_width == 8) > > + mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA; > > + host->wp_gpio = of_get_named_gpio(np, "wp-gpios", 0); > > + } else { > > if (pdata->flags & SLOTF_8_BIT_CAPABLE) > > mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA; > > if (pdata->flags & SLOTF_4_BIT_CAPABLE) > > @@ -857,6 +894,7 @@ static struct platform_driver mxs_mmc_driver = { > > .owner = THIS_MODULE, > > #ifdef CONFIG_PM > > .pm = &mxs_mmc_pm_ops, > > + .of_match_table = mxs_mmc_dt_ids, > > #endif > > }, > > }; > > Rest looks fine, thanks. > Thanks, Chris. -- Regards, Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
