Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751709AbbLQSF2 (ORCPT ); Thu, 17 Dec 2015 13:05:28 -0500 Received: from exsmtp01.microchip.com ([198.175.253.37]:54335 "EHLO email.microchip.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750881AbbLQSF0 (ORCPT ); Thu, 17 Dec 2015 13:05:26 -0500 From: To: , CC: , , , , , , , , , , , , , , , , , , , , , Subject: RE: [PATCH v2 12/14] mmc: sdhci-pic32: Add PIC32 SDHCI host controller driver Thread-Topic: [PATCH v2 12/14] mmc: sdhci-pic32: Add PIC32 SDHCI host controller driver Thread-Index: AQHRNsCt9Pq1H7UTskmnIVXyoRiPXp7N5mAAgAGV4zA= Date: Thu, 17 Dec 2015 18:05:24 +0000 Message-ID: References: <1450133093-7053-1-git-send-email-joshua.henderson@microchip.com> <1450133093-7053-13-git-send-email-joshua.henderson@microchip.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.10.76.4] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id tBHI5WUY018302 Content-Length: 6476 Lines: 191 On 12/16/2015 3:48 AM, Ulf Hansson wrote: > > [...] > > > +static int pic32_sdhci_probe(struct platform_device *pdev) { > > + struct device *dev = &pdev->dev; > > + struct sdhci_host *host; > > + struct resource *iomem; > > + struct pic32_sdhci_pdata *sdhci_pdata; > > + struct pic32_sdhci_platform_data *plat_data; > > + unsigned int clk_rate = 0; > > + int ret; > > + struct pinctrl *pinctrl; > > + > > + host = sdhci_alloc_host(dev, sizeof(*sdhci_pdata)); > > + if (IS_ERR(host)) { > > + ret = PTR_ERR(host); > > + dev_err(&pdev->dev, "cannot allocate memory for sdhci\n"); > > + goto err; > > + } > > + > > + sdhci_pdata = sdhci_priv(host); > > + sdhci_pdata->pdev = pdev; > > + platform_set_drvdata(pdev, host); > > + > > + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + host->ioaddr = devm_ioremap_resource(&pdev->dev, iomem); > > + if (IS_ERR(host->ioaddr)) { > > + ret = PTR_ERR(host->ioaddr); > > + dev_err(&pdev->dev, "unable to map iomem: %d\n", ret); > > + goto err_host; > > + } > > + > > + plat_data = pdev->dev.platform_data; > > + if (plat_data && plat_data->setup_dma) { > > + ret = plat_data->setup_dma(ADMA_FIFO_RD_THSHLD, > > + ADMA_FIFO_WR_THSHLD); > > + if (ret) > > + goto err_host; > > + } > > + > > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > > This isn't need as it's already handled by the PM core. Ack. Will remove. > > > + if (IS_ERR(pinctrl)) { > > + ret = PTR_ERR(pinctrl); > > + dev_warn(&pdev->dev, "No pinctrl provided %d\n", ret); > > + if (ret == -EPROBE_DEFER) > > + goto err_host; > > + } > > + > > + host->ops = &pic32_sdhci_ops; > > + host->irq = platform_get_irq(pdev, 0); > > + > > + sdhci_pdata->sys_clk = devm_clk_get(&pdev->dev, "sys_clk"); > > + if (IS_ERR(sdhci_pdata->sys_clk)) { > > + ret = PTR_ERR(sdhci_pdata->sys_clk); > > + dev_err(&pdev->dev, "Error getting clock\n"); > > + goto err_host; > > + } > > + > > + /* Enable clock when available! */ > > + ret = clk_prepare_enable(sdhci_pdata->sys_clk); > > + if (ret) { > > + dev_dbg(&pdev->dev, "Error enabling clock\n"); > > + goto err_host; > > + } > > + > > + /* SDH CLK enable */ > > + sdhci_pdata->base_clk = devm_clk_get(&pdev->dev, "base_clk"); > > + if (IS_ERR(sdhci_pdata->base_clk)) { > > + ret = PTR_ERR(sdhci_pdata->base_clk); > > + dev_err(&pdev->dev, "Error getting clock\n"); > > + goto err_host; > > + } > > + > > + /* Enable clock when available! */ > > + ret = clk_prepare_enable(sdhci_pdata->base_clk); > > + if (ret) { > > + dev_dbg(&pdev->dev, "Error enabling clock\n"); > > + goto err_host; > > + } > > + > > + clk_rate = clk_get_rate(sdhci_pdata->base_clk); > > + dev_dbg(&pdev->dev, "base clock at: %u\n", clk_rate); > > + clk_rate = clk_get_rate(sdhci_pdata->sys_clk); > > + dev_dbg(&pdev->dev, "sys clock at: %u\n", clk_rate); > > This looks like some leftover from a debugging task. Can you remove them? > Yes, these are not needed and will be removed. > > + > > + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > > + > > + host->quirks |= SDHCI_QUIRK_NO_HISPD_BIT; > > + > > + ret = mmc_of_parse(host->mmc); > > + if (ret) > > From this point, the error handling doesn't undo clk_prepare_enable(). > Please add that. Ack. Will do. > > > + goto err_host; > > + > > + ret = pic32_sdhci_probe_platform(pdev, sdhci_pdata); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to probe platform!\n"); > > + goto err_host; > > + } > > + > > + ret = sdhci_add_host(host); > > + if (ret) { > > + dev_dbg(&pdev->dev, "error adding host\n"); > > + goto err_host; > > + } > > + > > + dev_info(&pdev->dev, "Successfully added sdhci host\n"); > > + return 0; > > + > > +err_host: > > + sdhci_free_host(host); > > +err: > > + dev_err(&pdev->dev, "pic32-sdhci probe failed: %d\n", ret); > > + return ret; > > A general comment for the ->probe() and the below ->remove() callback, is that > you should probably be able to convert to use > sdhci_pltfm_init() and sdhci_pltfm_free() in favor of > sdhci_alloc_host() and sdhci_free_host(). > > I think that could simplify the code a bit. Ok. Will do. > > > +} > > + > > +static int pic32_sdhci_remove(struct platform_device *pdev) { > > + struct sdhci_host *host = platform_get_drvdata(pdev); > > + struct pic32_sdhci_pdata *sdhci_pdata = sdhci_priv(host); > > + int dead = 0; > > + u32 scratch; > > + > > + scratch = readl(host->ioaddr + SDHCI_INT_STATUS); > > + if (scratch == (u32)-1) > > + dead = 1; > > + > > + sdhci_remove_host(host, dead); > > + clk_disable_unprepare(sdhci_pdata->base_clk); > > + clk_disable_unprepare(sdhci_pdata->sys_clk); > > + sdhci_free_host(host); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id pic32_sdhci_id_table[] = { > > + { .compatible = "microchip,pic32mzda-sdhci" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, pic32_sdhci_id_table); > > + > > +static struct platform_driver pic32_sdhci_driver = { > > + .driver = { > > + .name = DEV_NAME, > > + .owner = THIS_MODULE, > > + .of_match_table = of_match_ptr(pic32_sdhci_id_table), > > + }, > > + .probe = pic32_sdhci_probe, > > + .remove = pic32_sdhci_remove, > > +}; > > + > > +module_platform_driver(pic32_sdhci_driver); > > + > > +MODULE_DESCRIPTION("Microchip PIC32 SDHCI driver"); > > +MODULE_AUTHOR("Pistirica Sorin Andrei & Sandeep Sheriker"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 1.7.9.5 Thanks, Paul ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?