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

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

[Masahiro Yamada]

Masahiro Yamada (7):
  mmc: sdhci-pltfm: bail out if register resource is too small
  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 | 46 +++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

-- 
1.9.1

[PATCH 1/7] mmc: sdhci-pltfm: bail out if register resource is too small

[Masahiro Yamada]

This code checks the resource size and displays an error message if
it is too small, but still moves forward.  Let is fail in that case.

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

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

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

Re: [PATCH 1/7] mmc: sdhci-pltfm: bail out if register resource is too small

[Adrian Hunter]

On 14/04/16 05:29, Masahiro Yamada wrote:
> This code checks the resource size and displays an error message if
> it is too small, but still moves forward.  Let is fail in that case.

is -> it

How do you know there aren't drivers that actually have a smaller size?

One that might not is sdhci-dove which appears to sidestep some registers in
sdhci_dove_readw().


> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  drivers/mmc/host/sdhci-pltfm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
> index 072bb27..7d12203 100644
> --- a/drivers/mmc/host/sdhci-pltfm.c
> +++ b/drivers/mmc/host/sdhci-pltfm.c
> @@ -127,8 +127,11 @@ struct sdhci_host *sdhci_pltfm_init(struct platform_device *pdev,
>  		goto err;
>  	}
>  
> -	if (resource_size(iomem) < 0x100)
> +	if (resource_size(iomem) < 0x100) {
>  		dev_err(&pdev->dev, "Invalid iomem size!\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
>  
>  	host = sdhci_alloc_host(&pdev->dev,
>  		sizeof(struct sdhci_pltfm_host) + priv_size);
> 

Re: [PATCH 1/7] mmc: sdhci-pltfm: bail out if register resource is too small

[Masahiro Yamada]

Hi Adrian,


2016-04-19 20:32 GMT+09:00 Adrian Hunter <adrian.hunter@intel.com>:
> On 14/04/16 05:29, Masahiro Yamada wrote:
>> This code checks the resource size and displays an error message if
>> it is too small, but still moves forward.  Let is fail in that case.
>
> is -> it
>
> How do you know there aren't drivers that actually have a smaller size?

No, I do not know.

This is an error message, so I think it should fail in that case.


Maybe, can we drop this check completely?




-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/7] mmc: sdhci-pltfm: bail out if register resource is too small

[Adrian Hunter]

On 19/04/16 14:46, Masahiro Yamada wrote:
> Hi Adrian,
> 
> 
> 2016-04-19 20:32 GMT+09:00 Adrian Hunter <adrian.hunter@intel.com>:
>> On 14/04/16 05:29, Masahiro Yamada wrote:
>>> This code checks the resource size and displays an error message if
>>> it is too small, but still moves forward.  Let is fail in that case.
>>
>> is -> it
>>
>> How do you know there aren't drivers that actually have a smaller size?
> 
> No, I do not know.
> 
> This is an error message, so I think it should fail in that case.
> 
> 
> Maybe, can we drop this check completely?

Yes, I think so.

[PATCH 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 error.  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>
---

 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 7d12203..3354a53 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -152,6 +152,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");
+		ret = host->irq;
+		goto err_request;
+	}
 
 	if (!request_mem_region(iomem->start, resource_size(iomem),
 		mmc_hostname(host->mmc))) {
-- 
1.9.1

[PATCH 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>
---

 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 3354a53..e4da44a 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -158,8 +158,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;
@@ -169,7 +170,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;
 	}
 
 	/*
@@ -183,8 +184,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:
@@ -196,10 +195,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 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>
---

 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 e4da44a..c81a72d 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -166,7 +166,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;
@@ -196,7 +197,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 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>
---

 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 c81a72d..94955e1 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -158,19 +158,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 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>
---

 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 94955e1..870c3d7 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;
 	}
 
@@ -141,6 +143,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;
@@ -158,12 +161,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 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 this
probe method.  So, we can drop the sdhci_free_host() call from the
failure path.

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

 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 870c3d7..94bdca8 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);
@@ -135,6 +135,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");
+		ret = irq;
+		goto err;
+	}
+
 	host = sdhci_alloc_host(&pdev->dev,
 		sizeof(struct sdhci_pltfm_host) + priv_size);
 
@@ -144,6 +151,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;
@@ -154,13 +162,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");
-		ret = host->irq;
-		goto err_request;
-	}
-
 	/*
 	 * Some platforms need to probe the controller to be able to
 	 * determine which caps should be used.
@@ -171,9 +172,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