[PATCH v2 0/7] mmc: sdhci-pltfm: fix and tidy up sdhci_pltfm_init()

[PATCH v2 0/7] mmc: sdhci-pltfm: fix and tidy up sdhci_pltfm_init()

[Masahiro Yamada]

Changes in v2:
  - Remove resource size checking rather than fix it.
  - Add \n to the tail of the error message

Masahiro Yamada (7):
  mmc: sdhci-pltfm: drop error message for too small MMIO resource size
  mmc: sdhci-pltfm: check return value of platform_get_irq()
  mmc: sdhci-pltfm: use devm_request_mem_region()
  mmc: sdhci-pltfm: use devm_ioremap()
  mmc: sdhci-pltfm: use devm_ioremap_resource()
  mmc: sdhci-pltfm: move devm_ioremap_resource() up
  mmc: sdhci-pltfm: call platform_get_irq() before sdhci_alloc_host()

 drivers/mmc/host/sdhci-pltfm.c | 42 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

-- 
1.9.1

[PATCH v2 1/7] mmc: sdhci-pltfm: drop error message for too small MMIO resource size

[Masahiro Yamada]

The requirement resource_size >= 0x100 may not necessarily be
reasonable; for example, sdhci-dove appears to sidestep some
registers in sdhci_dove_readw().

Moreover, current code displays an error message for too small
resource size, but still moves forward.

Every DT should be responsible for describing its properties
correctly, so lets's remove this error message from the common
framework.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Remove resource size checking rather than fix it.

 drivers/mmc/host/sdhci-pltfm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 072bb27..03dbdeb 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -127,9 +127,6 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 		goto err;
 	}
 
-	if (resource_size(iomem) < 0x100)
-		dev_err(&pdev->dev, "Invalid iomem size!\n");
-
 	host = sdhci_alloc_host(&pdev->dev,
 		sizeof(struct sdhci_pltfm_host) + priv_size);
 
-- 
1.9.1

[PATCH v2 2/7] mmc: sdhci-pltfm: check return value of platform_get_irq()

[Masahiro Yamada]

The function platform_get_irq() can fail; it returns a negative error
code on failure.  A negative IRQ number will make sdhci_add_host() fail
to request IRQ anyway, but it makes sense to let it fail earlier here.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Add \n to the tail of the error message

 drivers/mmc/host/sdhci-pltfm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 03dbdeb..24377a3 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -146,6 +146,11 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 	}
 
 	host->irq = platform_get_irq(pdev, 0);
+	if (host->irq < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ number\n");
+		ret = host->irq;
+		goto err_request;
+	}
 
 	if (!request_mem_region(iomem->start, resource_size(iomem),
 		mmc_hostname(host->mmc))) {
-- 
1.9.1

[PATCH v2 3/7] mmc: sdhci-pltfm: use devm_request_mem_region()

[Masahiro Yamada]

Use the managed variant of request_mem_region().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci-pltfm.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 24377a3..8527a7c 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -152,8 +152,9 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 		goto err_request;
 	}
 
-	if (!request_mem_region(iomem->start, resource_size(iomem),
-		mmc_hostname(host->mmc))) {
+	if (!devm_request_mem_region(&pdev->dev, iomem->start,
+				     resource_size(iomem),
+				     mmc_hostname(host->mmc))) {
 		dev_err(&pdev->dev, "cannot request region\n");
 		ret = -EBUSY;
 		goto err_request;
@@ -163,7 +164,7 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 	if (!host->ioaddr) {
 		dev_err(&pdev->dev, "failed to remap registers\n");
 		ret = -ENOMEM;
-		goto err_remap;
+		goto err_request;
 	}
 
 	/*
@@ -177,8 +178,6 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 
 	return host;
 
-err_remap:
-	release_mem_region(iomem->start, resource_size(iomem));
 err_request:
 	sdhci_free_host(host);
 err:
@@ -190,10 +189,8 @@ EXPORT_SYMBOL_GPL(sdhci_pltfm_init);
 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);
 }
 EXPORT_SYMBOL_GPL(sdhci_pltfm_free);
-- 
1.9.1

[PATCH v2 4/7] mmc: sdhci-pltfm: use devm_ioremap()

[Masahiro Yamada]

Use the managed variant of ioremap().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci-pltfm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 8527a7c..5213287 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -160,7 +160,8 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 		goto err_request;
 	}
 
-	host->ioaddr = ioremap(iomem->start, resource_size(iomem));
+	host->ioaddr = devm_ioremap(&pdev->dev, iomem->start,
+				    resource_size(iomem));
 	if (!host->ioaddr) {
 		dev_err(&pdev->dev, "failed to remap registers\n");
 		ret = -ENOMEM;
@@ -190,7 +191,6 @@ void sdhci_pltfm_free(struct platform_device *pdev)
 {
 	struct sdhci_host *host = platform_get_drvdata(pdev);
 
-	iounmap(host->ioaddr);
 	sdhci_free_host(host);
 }
 EXPORT_SYMBOL_GPL(sdhci_pltfm_free);
-- 
1.9.1

[PATCH v2 5/7] mmc: sdhci-pltfm: use devm_ioremap_resource()

[Masahiro Yamada]

The chain of devm_request_mem_region() and devm_ioremap() can be
replaced with devm_ioremap_resource().  Also, we can drop the error
messages because devm_ioremap_resource() displays similar messages
on error.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci-pltfm.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 5213287..caa05d7 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -152,19 +152,9 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 		goto err_request;
 	}
 
-	if (!devm_request_mem_region(&pdev->dev, iomem->start,
-				     resource_size(iomem),
-				     mmc_hostname(host->mmc))) {
-		dev_err(&pdev->dev, "cannot request region\n");
-		ret = -EBUSY;
-		goto err_request;
-	}
-
-	host->ioaddr = devm_ioremap(&pdev->dev, iomem->start,
-				    resource_size(iomem));
-	if (!host->ioaddr) {
-		dev_err(&pdev->dev, "failed to remap registers\n");
-		ret = -ENOMEM;
+	host->ioaddr = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(host->ioaddr)) {
+		ret = PTR_ERR(host->ioaddr);
 		goto err_request;
 	}
 
-- 
1.9.1

[PATCH v2 6/7] mmc: sdhci-pltfm: move devm_ioremap_resource() up

[Masahiro Yamada]

Call devm_ioremap_resource() right after platform_get_resource().
This saves the error check of platform_get_resource() because
devm_ioremap_resource() checks if the given resource is NULL.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci-pltfm.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index caa05d7..1d74db8 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -119,11 +119,13 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 {
 	struct sdhci_host *host;
 	struct resource *iomem;
+	void __iomem *ioaddr;
 	int ret;
 
 	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!iomem) {
-		ret = -ENOMEM;
+	ioaddr = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(ioaddr)) {
+		ret = PTR_ERR(ioaddr);
 		goto err;
 	}
 
@@ -135,6 +137,7 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 		goto err;
 	}
 
+	host->ioaddr = ioaddr;
 	host->hw_name = dev_name(&pdev->dev);
 	if (pdata && pdata->ops)
 		host->ops = pdata->ops;
@@ -152,12 +155,6 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 		goto err_request;
 	}
 
-	host->ioaddr = devm_ioremap_resource(&pdev->dev, iomem);
-	if (IS_ERR(host->ioaddr)) {
-		ret = PTR_ERR(host->ioaddr);
-		goto err_request;
-	}
-
 	/*
 	 * Some platforms need to probe the controller to be able to
 	 * determine which caps should be used.
-- 
1.9.1

[PATCH v2 7/7] mmc: sdhci-pltfm: call platform_get_irq() before sdhci_alloc_host()

[Masahiro Yamada]

Swap the call order of sdhci_alloc_host() and platform_get_irq().
It makes sdhci_alloc_host() the last function that can fail in the
sdhci_pltfm_init().  So, we can drop the sdhci_free_host() call from
the failure path.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci-pltfm.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 1d74db8..64f287a 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -120,7 +120,7 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 	struct sdhci_host *host;
 	struct resource *iomem;
 	void __iomem *ioaddr;
-	int ret;
+	int irq, ret;
 
 	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ioaddr = devm_ioremap_resource(&pdev->dev, iomem);
@@ -129,6 +129,13 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 		goto err;
 	}
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ number\n");
+		ret = irq;
+		goto err;
+	}
+
 	host = sdhci_alloc_host(&pdev->dev,
 		sizeof(struct sdhci_pltfm_host) + priv_size);
 
@@ -138,6 +145,7 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 	}
 
 	host->ioaddr = ioaddr;
+	host->irq = irq;
 	host->hw_name = dev_name(&pdev->dev);
 	if (pdata && pdata->ops)
 		host->ops = pdata->ops;
@@ -148,13 +156,6 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 		host->quirks2 = pdata->quirks2;
 	}
 
-	host->irq = platform_get_irq(pdev, 0);
-	if (host->irq < 0) {
-		dev_err(&pdev->dev, "failed to get IRQ number\n");
-		ret = host->irq;
-		goto err_request;
-	}
-
 	/*
 	 * Some platforms need to probe the controller to be able to
 	 * determine which caps should be used.
@@ -165,9 +166,6 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
 	platform_set_drvdata(pdev, host);
 
 	return host;
-
-err_request:
-	sdhci_free_host(host);
 err:
 	dev_err(&pdev->dev, "%s failed %d\n", __func__, ret);
 	return ERR_PTR(ret);
-- 
1.9.1

Re: [PATCH v2 0/7] mmc: sdhci-pltfm: fix and tidy up sdhci_pltfm_init()

[Adrian Hunter]

On 20/04/16 05:16, Masahiro Yamada wrote:
> 
> 
> Changes in v2:
>   - Remove resource size checking rather than fix it.
>   - Add \n to the tail of the error message
> 
> Masahiro Yamada (7):
>   mmc: sdhci-pltfm: drop error message for too small MMIO resource size
>   mmc: sdhci-pltfm: check return value of platform_get_irq()
>   mmc: sdhci-pltfm: use devm_request_mem_region()
>   mmc: sdhci-pltfm: use devm_ioremap()
>   mmc: sdhci-pltfm: use devm_ioremap_resource()
>   mmc: sdhci-pltfm: move devm_ioremap_resource() up
>   mmc: sdhci-pltfm: call platform_get_irq() before sdhci_alloc_host()
> 
>  drivers/mmc/host/sdhci-pltfm.c | 42 +++++++++++++-----------------------------
>  1 file changed, 13 insertions(+), 29 deletions(-)
> 

Thank you!

For all 7 patches:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Re: [PATCH v2 0/7] mmc: sdhci-pltfm: fix and tidy up sdhci_pltfm_init()

[Ulf Hansson]

On 20 April 2016 at 04:16, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
>
> Changes in v2:
>   - Remove resource size checking rather than fix it.
>   - Add \n to the tail of the error message
>
> Masahiro Yamada (7):
>   mmc: sdhci-pltfm: drop error message for too small MMIO resource size
>   mmc: sdhci-pltfm: check return value of platform_get_irq()
>   mmc: sdhci-pltfm: use devm_request_mem_region()
>   mmc: sdhci-pltfm: use devm_ioremap()
>   mmc: sdhci-pltfm: use devm_ioremap_resource()
>   mmc: sdhci-pltfm: move devm_ioremap_resource() up
>   mmc: sdhci-pltfm: call platform_get_irq() before sdhci_alloc_host()
>
>  drivers/mmc/host/sdhci-pltfm.c | 42 +++++++++++++-----------------------------
>  1 file changed, 13 insertions(+), 29 deletions(-)
>
> --
> 1.9.1
>

Thanks, applied for next!

Kind regards
Uffe