cleanups for sh-mmcif driver

cleanups for sh-mmcif driver

[Ben Dooks]

Some cleanups and updates for the sh mmcif driver.

cleanups for sh-mmcif driver

[Ben Dooks]

Some cleanups and updates for the sh mmcif driver.

[PATCH 1/6] mmc: sh-mmcif: print bus clock rate on probe

[Ben Dooks]

Add a print to show the host-bus clock rate for mmcif on probe to allow
easy check on what clock rate the bus clock is at.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index e8713d7..c48df98 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1477,7 +1477,8 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 
 	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
 
-	dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
+	dev_info(&pdev->dev, "driver version %s, clock rate %ldMHz\n",
+		 DRIVER_VERSION, clk_get_rate(host->hclk) / 1000000);
 	dev_dbg(&pdev->dev, "chip ver H'%04x\n",
 		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
 	return ret;
-- 
1.9.0

[PATCH 1/6] mmc: sh-mmcif: print bus clock rate on probe

[Ben Dooks]

Add a print to show the host-bus clock rate for mmcif on probe to allow
easy check on what clock rate the bus clock is at.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index e8713d7..c48df98 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1477,7 +1477,8 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 
 	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
 
-	dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
+	dev_info(&pdev->dev, "driver version %s, clock rate %ldMHz\n",
+		 DRIVER_VERSION, clk_get_rate(host->hclk) / 1000000);
 	dev_dbg(&pdev->dev, "chip ver H'%04x\n",
 		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
 	return ret;
-- 
1.9.0

Re: [PATCH 1/6] mmc: sh-mmcif: print bus clock rate on probe

[Geert Uytterhoeven]

On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> -       dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
> +       dev_info(&pdev->dev, "driver version %s, clock rate %ldMHz\n",
> +                DRIVER_VERSION, clk_get_rate(host->hclk) / 1000000);

%lu (soon the clocks won't fit in unsigned 32-bit anymore, though...)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] mmc: sh-mmcif: print bus clock rate on probe

[Geert Uytterhoeven]

On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> -       dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
> +       dev_info(&pdev->dev, "driver version %s, clock rate %ldMHz\n",
> +                DRIVER_VERSION, clk_get_rate(host->hclk) / 1000000);

%lu (soon the clocks won't fit in unsigned 32-bit anymore, though...)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] mmc: sh-mmcif: print bus clock rate on probe

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Tuesday 01 April 2014 11:25:49 Ben Dooks wrote:
> Add a print to show the host-bus clock rate for mmcif on probe to allow
> easy check on what clock rate the bus clock is at.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/mmc/host/sh_mmcif.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index e8713d7..c48df98 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1477,7 +1477,8 @@ static int sh_mmcif_probe(struct platform_device
> *pdev)
> 
>  	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
> 
> -	dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
> +	dev_info(&pdev->dev, "driver version %s, clock rate %ldMHz\n",
> +		 DRIVER_VERSION, clk_get_rate(host->hclk) / 1000000);
>  	dev_dbg(&pdev->dev, "chip ver H'%04x\n",
>  		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
>  	return ret;

Given that DRIVER_VERSION is defined as "2010-04-28", I'd take this as an 
opportunity to remove it completely. You could combine the dev_info and 
dev_dbg message into something similar to

	dev_info(&pdev->dev, "Chip version 0x%04x found, clock rate %luMHz\n",
		 sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0xffff,
 		 clk_get_rate(host->hclk) / 1000000);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] mmc: sh-mmcif: print bus clock rate on probe

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Tuesday 01 April 2014 11:25:49 Ben Dooks wrote:
> Add a print to show the host-bus clock rate for mmcif on probe to allow
> easy check on what clock rate the bus clock is at.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/mmc/host/sh_mmcif.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index e8713d7..c48df98 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1477,7 +1477,8 @@ static int sh_mmcif_probe(struct platform_device
> *pdev)
> 
>  	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
> 
> -	dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
> +	dev_info(&pdev->dev, "driver version %s, clock rate %ldMHz\n",
> +		 DRIVER_VERSION, clk_get_rate(host->hclk) / 1000000);
>  	dev_dbg(&pdev->dev, "chip ver H'%04x\n",
>  		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
>  	return ret;

Given that DRIVER_VERSION is defined as "2010-04-28", I'd take this as an 
opportunity to remove it completely. You could combine the dev_info and 
dev_dbg message into something similar to

	dev_info(&pdev->dev, "Chip version 0x%04x found, clock rate %luMHz\n",
		 sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0xffff,
 		 clk_get_rate(host->hclk) / 1000000);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] mmc: sh-mmcif: print bus clock rate on probe

[Ben Dooks]

On 01/04/14 12:08, Laurent Pinchart wrote:
> Hi Ben,
>
> Thank you for the patch.
>
> On Tuesday 01 April 2014 11:25:49 Ben Dooks wrote:
>> Add a print to show the host-bus clock rate for mmcif on probe to allow
>> easy check on what clock rate the bus clock is at.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/mmc/host/sh_mmcif.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
>> index e8713d7..c48df98 100644
>> --- a/drivers/mmc/host/sh_mmcif.c
>> +++ b/drivers/mmc/host/sh_mmcif.c
>> @@ -1477,7 +1477,8 @@ static int sh_mmcif_probe(struct platform_device
>> *pdev)
>>
>>   	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
>>
>> -	dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
>> +	dev_info(&pdev->dev, "driver version %s, clock rate %ldMHz\n",
>> +		 DRIVER_VERSION, clk_get_rate(host->hclk) / 1000000);
>>   	dev_dbg(&pdev->dev, "chip ver H'%04x\n",
>>   		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
>>   	return ret;
>
> Given that DRIVER_VERSION is defined as "2010-04-28", I'd take this as an
> opportunity to remove it completely. You could combine the dev_info and
> dev_dbg message into something similar to

Ok, changed to use this.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH 1/6] mmc: sh-mmcif: print bus clock rate on probe

[Ben Dooks]

On 01/04/14 12:08, Laurent Pinchart wrote:
> Hi Ben,
>
> Thank you for the patch.
>
> On Tuesday 01 April 2014 11:25:49 Ben Dooks wrote:
>> Add a print to show the host-bus clock rate for mmcif on probe to allow
>> easy check on what clock rate the bus clock is at.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/mmc/host/sh_mmcif.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
>> index e8713d7..c48df98 100644
>> --- a/drivers/mmc/host/sh_mmcif.c
>> +++ b/drivers/mmc/host/sh_mmcif.c
>> @@ -1477,7 +1477,8 @@ static int sh_mmcif_probe(struct platform_device
>> *pdev)
>>
>>   	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
>>
>> -	dev_info(&pdev->dev, "driver version %s\n", DRIVER_VERSION);
>> +	dev_info(&pdev->dev, "driver version %s, clock rate %ldMHz\n",
>> +		 DRIVER_VERSION, clk_get_rate(host->hclk) / 1000000);
>>   	dev_dbg(&pdev->dev, "chip ver H'%04x\n",
>>   		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
>>   	return ret;
>
> Given that DRIVER_VERSION is defined as "2010-04-28", I'd take this as an
> opportunity to remove it completely. You could combine the dev_info and
> dev_dbg message into something similar to

Ok, changed to use this.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Ben Dooks]

Start tidying the probe/release code by using devm_ioremap_resource() to
map the IO registers.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index c48df98..be6be2b 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1379,22 +1379,17 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Get irq error\n");
 		return -ENXIO;
 	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "platform_get_resource error.\n");
-		return -ENXIO;
-	}
-	reg = ioremap(res->start, resource_size(res));
-	if (!reg) {
+	reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(reg)) {
 		dev_err(&pdev->dev, "ioremap error.\n");
 		return -ENOMEM;
 	}
 
 	mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
-	if (!mmc) {
-		ret = -ENOMEM;
-		goto ealloch;
-	}
+	if (!mmc)
+		return -ENOMEM;
 
 	ret = mmc_of_parse(mmc);
 	if (ret < 0)
@@ -1499,8 +1494,6 @@ eclkget:
 	pm_runtime_disable(&pdev->dev);
 eofparse:
 	mmc_free_host(mmc);
-ealloch:
-	iounmap(reg);
 	return ret;
 }
 
@@ -1525,9 +1518,6 @@ static int sh_mmcif_remove(struct platform_device *pdev)
 	 */
 	cancel_delayed_work_sync(&host->timeout_work);
 
-	if (host->addr)
-		iounmap(host->addr);
-
 	irq[0] = platform_get_irq(pdev, 0);
 	irq[1] = platform_get_irq(pdev, 1);
 
-- 
1.9.0

[PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Ben Dooks]

Start tidying the probe/release code by using devm_ioremap_resource() to
map the IO registers.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index c48df98..be6be2b 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1379,22 +1379,17 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Get irq error\n");
 		return -ENXIO;
 	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "platform_get_resource error.\n");
-		return -ENXIO;
-	}
-	reg = ioremap(res->start, resource_size(res));
-	if (!reg) {
+	reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(reg)) {
 		dev_err(&pdev->dev, "ioremap error.\n");
 		return -ENOMEM;
 	}
 
 	mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
-	if (!mmc) {
-		ret = -ENOMEM;
-		goto ealloch;
-	}
+	if (!mmc)
+		return -ENOMEM;
 
 	ret = mmc_of_parse(mmc);
 	if (ret < 0)
@@ -1499,8 +1494,6 @@ eclkget:
 	pm_runtime_disable(&pdev->dev);
 eofparse:
 	mmc_free_host(mmc);
-ealloch:
-	iounmap(reg);
 	return ret;
 }
 
@@ -1525,9 +1518,6 @@ static int sh_mmcif_remove(struct platform_device *pdev)
 	 */
 	cancel_delayed_work_sync(&host->timeout_work);
 
-	if (host->addr)
-		iounmap(host->addr);
-
 	irq[0] = platform_get_irq(pdev, 0);
 	irq[1] = platform_get_irq(pdev, 1);
 
-- 
1.9.0

Re: [PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Geert Uytterhoeven]

On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> +       reg = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(reg)) {
>                 dev_err(&pdev->dev, "ioremap error.\n");

devm_ioremap_resource() already calls dev_err() for the various error
cases, so you can drop this line.

>                 return -ENOMEM;

return PTR_ERR(reg);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Geert Uytterhoeven]

On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> +       reg = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(reg)) {
>                 dev_err(&pdev->dev, "ioremap error.\n");

devm_ioremap_resource() already calls dev_err() for the various error
cases, so you can drop this line.

>                 return -ENOMEM;

return PTR_ERR(reg);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Ben Dooks]

On 01/04/14 11:55, Geert Uytterhoeven wrote:
> On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> +       reg = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(reg)) {
>>                  dev_err(&pdev->dev, "ioremap error.\n");
>
> devm_ioremap_resource() already calls dev_err() for the various error
> cases, so you can drop this line.
>
>>                  return -ENOMEM;

Oops. Will fix.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Ben Dooks]

On 01/04/14 11:55, Geert Uytterhoeven wrote:
> On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> +       reg = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(reg)) {
>>                  dev_err(&pdev->dev, "ioremap error.\n");
>
> devm_ioremap_resource() already calls dev_err() for the various error
> cases, so you can drop this line.
>
>>                  return -ENOMEM;

Oops. Will fix.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Tuesday 01 April 2014 11:25:50 Ben Dooks wrote:
> Start tidying the probe/release code by using devm_ioremap_resource() to
> map the IO registers.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/mmc/host/sh_mmcif.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index c48df98..be6be2b 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1379,22 +1379,17 @@ static int sh_mmcif_probe(struct platform_device
> *pdev) dev_err(&pdev->dev, "Get irq error\n");
>  		return -ENXIO;
>  	}
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "platform_get_resource error.\n");
> -		return -ENXIO;
> -	}
> -	reg = ioremap(res->start, resource_size(res));
> -	if (!reg) {
> +	reg = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(reg)) {
>  		dev_err(&pdev->dev, "ioremap error.\n");

devm_ioremap_resource already prints an error message on failure, you can this 
remove this one.

>  		return -ENOMEM;

What about returning PTR_ERR(reg) instead ?

>  	}
> 
>  	mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
> -	if (!mmc) {
> -		ret = -ENOMEM;
> -		goto ealloch;
> -	}
> +	if (!mmc)
> +		return -ENOMEM;
> 
>  	ret = mmc_of_parse(mmc);
>  	if (ret < 0)
> @@ -1499,8 +1494,6 @@ eclkget:
>  	pm_runtime_disable(&pdev->dev);
>  eofparse:
>  	mmc_free_host(mmc);
> -ealloch:
> -	iounmap(reg);
>  	return ret;
>  }
> 
> @@ -1525,9 +1518,6 @@ static int sh_mmcif_remove(struct platform_device
> *pdev) */
>  	cancel_delayed_work_sync(&host->timeout_work);
> 
> -	if (host->addr)
> -		iounmap(host->addr);
> -
>  	irq[0] = platform_get_irq(pdev, 0);
>  	irq[1] = platform_get_irq(pdev, 1);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Laurent Pinchart]

Hi Ben,

Thank you for the patch.

On Tuesday 01 April 2014 11:25:50 Ben Dooks wrote:
> Start tidying the probe/release code by using devm_ioremap_resource() to
> map the IO registers.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/mmc/host/sh_mmcif.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index c48df98..be6be2b 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1379,22 +1379,17 @@ static int sh_mmcif_probe(struct platform_device
> *pdev) dev_err(&pdev->dev, "Get irq error\n");
>  		return -ENXIO;
>  	}
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "platform_get_resource error.\n");
> -		return -ENXIO;
> -	}
> -	reg = ioremap(res->start, resource_size(res));
> -	if (!reg) {
> +	reg = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(reg)) {
>  		dev_err(&pdev->dev, "ioremap error.\n");

devm_ioremap_resource already prints an error message on failure, you can this 
remove this one.

>  		return -ENOMEM;

What about returning PTR_ERR(reg) instead ?

>  	}
> 
>  	mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
> -	if (!mmc) {
> -		ret = -ENOMEM;
> -		goto ealloch;
> -	}
> +	if (!mmc)
> +		return -ENOMEM;
> 
>  	ret = mmc_of_parse(mmc);
>  	if (ret < 0)
> @@ -1499,8 +1494,6 @@ eclkget:
>  	pm_runtime_disable(&pdev->dev);
>  eofparse:
>  	mmc_free_host(mmc);
> -ealloch:
> -	iounmap(reg);
>  	return ret;
>  }
> 
> @@ -1525,9 +1518,6 @@ static int sh_mmcif_remove(struct platform_device
> *pdev) */
>  	cancel_delayed_work_sync(&host->timeout_work);
> 
> -	if (host->addr)
> -		iounmap(host->addr);
> -
>  	irq[0] = platform_get_irq(pdev, 0);
>  	irq[1] = platform_get_irq(pdev, 1);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Sergei Shtylyov]

Hello.

On 01-04-2014 14:25, Ben Dooks wrote:

> Start tidying the probe/release code by using devm_ioremap_resource() to
> map the IO registers.

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>   drivers/mmc/host/sh_mmcif.c | 20 +++++---------------
>   1 file changed, 5 insertions(+), 15 deletions(-)

> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index c48df98..be6be2b 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1379,22 +1379,17 @@ static int sh_mmcif_probe(struct platform_device *pdev)
>   		dev_err(&pdev->dev, "Get irq error\n");
>   		return -ENXIO;
>   	}
> +
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "platform_get_resource error.\n");
> -		return -ENXIO;
> -	}
> -	reg = ioremap(res->start, resource_size(res));
> -	if (!reg) {
> +	reg = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(reg)) {
>   		dev_err(&pdev->dev, "ioremap error.\n");
>   		return -ENOMEM;

    Should propagate devm_ioremap_resource() error instead.

[...]

WBR, Sergei

Re: [PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Sergei Shtylyov]

Hello.

On 01-04-2014 14:25, Ben Dooks wrote:

> Start tidying the probe/release code by using devm_ioremap_resource() to
> map the IO registers.

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>   drivers/mmc/host/sh_mmcif.c | 20 +++++---------------
>   1 file changed, 5 insertions(+), 15 deletions(-)

> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index c48df98..be6be2b 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1379,22 +1379,17 @@ static int sh_mmcif_probe(struct platform_device *pdev)
>   		dev_err(&pdev->dev, "Get irq error\n");
>   		return -ENXIO;
>   	}
> +
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "platform_get_resource error.\n");
> -		return -ENXIO;
> -	}
> -	reg = ioremap(res->start, resource_size(res));
> -	if (!reg) {
> +	reg = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(reg)) {
>   		dev_err(&pdev->dev, "ioremap error.\n");
>   		return -ENOMEM;

    Should propagate devm_ioremap_resource() error instead.

[...]

WBR, Sergei

[PATCH 3/6] mmc: sh-mmcif: use devm_ for clock management

[Ben Dooks]

---
 drivers/mmc/host/sh_mmcif.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index be6be2b..c006310 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1393,7 +1393,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 
 	ret = mmc_of_parse(mmc);
 	if (ret < 0)
-		goto eofparse;
+		goto err_host;
 
 	host		= mmc_priv(mmc);
 	host->mmc	= mmc;
@@ -1423,19 +1423,19 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	host->power = false;
 
-	host->hclk = clk_get(&pdev->dev, NULL);
+	host->hclk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(host->hclk)) {
 		ret = PTR_ERR(host->hclk);
 		dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
-		goto eclkget;
+		goto err_pm;
 	}
 	ret = sh_mmcif_clk_update(host);
 	if (ret < 0)
-		goto eclkupdate;
+		goto err_clk;
 
 	ret = pm_runtime_resume(&pdev->dev);
 	if (ret < 0)
-		goto eresume;
+		goto err_clk;
 
 	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
 
@@ -1486,13 +1486,11 @@ ereqirq1:
 	free_irq(irq[0], host);
 ereqirq0:
 	pm_runtime_suspend(&pdev->dev);
-eresume:
+err_clk:
 	clk_disable_unprepare(host->hclk);
-eclkupdate:
-	clk_put(host->hclk);
-eclkget:
+err_pm:
 	pm_runtime_disable(&pdev->dev);
-eofparse:
+err_host:
 	mmc_free_host(mmc);
 	return ret;
 }
-- 
1.9.0

[PATCH 3/6] mmc: sh-mmcif: use devm_ for clock management

[Ben Dooks]

---
 drivers/mmc/host/sh_mmcif.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index be6be2b..c006310 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1393,7 +1393,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 
 	ret = mmc_of_parse(mmc);
 	if (ret < 0)
-		goto eofparse;
+		goto err_host;
 
 	host		= mmc_priv(mmc);
 	host->mmc	= mmc;
@@ -1423,19 +1423,19 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	host->power = false;
 
-	host->hclk = clk_get(&pdev->dev, NULL);
+	host->hclk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(host->hclk)) {
 		ret = PTR_ERR(host->hclk);
 		dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
-		goto eclkget;
+		goto err_pm;
 	}
 	ret = sh_mmcif_clk_update(host);
 	if (ret < 0)
-		goto eclkupdate;
+		goto err_clk;
 
 	ret = pm_runtime_resume(&pdev->dev);
 	if (ret < 0)
-		goto eresume;
+		goto err_clk;
 
 	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
 
@@ -1486,13 +1486,11 @@ ereqirq1:
 	free_irq(irq[0], host);
 ereqirq0:
 	pm_runtime_suspend(&pdev->dev);
-eresume:
+err_clk:
 	clk_disable_unprepare(host->hclk);
-eclkupdate:
-	clk_put(host->hclk);
-eclkget:
+err_pm:
 	pm_runtime_disable(&pdev->dev);
-eofparse:
+err_host:
 	mmc_free_host(mmc);
 	return ret;
 }
-- 
1.9.0

Re: [PATCH 3/6] mmc: sh-mmcif: use devm_ for clock management

[Geert Uytterhoeven]

On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> -       host->hclk = clk_get(&pdev->dev, NULL);
> +       host->hclk = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(host->hclk)) {
>                 ret = PTR_ERR(host->hclk);
>                 dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
> -               goto eclkget;
> +               goto err_pm;
>         }
>         ret = sh_mmcif_clk_update(host);
>         if (ret < 0)
> -               goto eclkupdate;
> +               goto err_clk;

This goto doesn't look correct to me. If sh_mmcif_clk_update() failed,
the clock was not enabled nor prepared. "goto err_pm"?

>         ret = pm_runtime_resume(&pdev->dev);
>         if (ret < 0)
> -               goto eresume;
> +               goto err_clk;
>
>         INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
>
> @@ -1486,13 +1486,11 @@ ereqirq1:
>         free_irq(irq[0], host);
>  ereqirq0:
>         pm_runtime_suspend(&pdev->dev);
> -eresume:
> +err_clk:
>         clk_disable_unprepare(host->hclk);
> -eclkupdate:
> -       clk_put(host->hclk);
> -eclkget:
> +err_pm:
>         pm_runtime_disable(&pdev->dev);
> -eofparse:
> +err_host:
>         mmc_free_host(mmc);
>         return ret;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/6] mmc: sh-mmcif: use devm_ for clock management

[Geert Uytterhoeven]

On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> -       host->hclk = clk_get(&pdev->dev, NULL);
> +       host->hclk = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(host->hclk)) {
>                 ret = PTR_ERR(host->hclk);
>                 dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
> -               goto eclkget;
> +               goto err_pm;
>         }
>         ret = sh_mmcif_clk_update(host);
>         if (ret < 0)
> -               goto eclkupdate;
> +               goto err_clk;

This goto doesn't look correct to me. If sh_mmcif_clk_update() failed,
the clock was not enabled nor prepared. "goto err_pm"?

>         ret = pm_runtime_resume(&pdev->dev);
>         if (ret < 0)
> -               goto eresume;
> +               goto err_clk;
>
>         INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
>
> @@ -1486,13 +1486,11 @@ ereqirq1:
>         free_irq(irq[0], host);
>  ereqirq0:
>         pm_runtime_suspend(&pdev->dev);
> -eresume:
> +err_clk:
>         clk_disable_unprepare(host->hclk);
> -eclkupdate:
> -       clk_put(host->hclk);
> -eclkget:
> +err_pm:
>         pm_runtime_disable(&pdev->dev);
> -eofparse:
> +err_host:
>         mmc_free_host(mmc);
>         return ret;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/6] mmc: sh-mmcif: use devm_ for clock management

[Ben Dooks]

On 01/04/14 11:59, Geert Uytterhoeven wrote:
> On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> -       host->hclk = clk_get(&pdev->dev, NULL);
>> +       host->hclk = devm_clk_get(&pdev->dev, NULL);
>>          if (IS_ERR(host->hclk)) {
>>                  ret = PTR_ERR(host->hclk);
>>                  dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
>> -               goto eclkget;
>> +               goto err_pm;
>>          }
>>          ret = sh_mmcif_clk_update(host);
>>          if (ret < 0)
>> -               goto eclkupdate;
>> +               goto err_clk;
>
> This goto doesn't look correct to me. If sh_mmcif_clk_update() failed,
> the clock was not enabled nor prepared. "goto err_pm"?

Yes, I think the clk_prepare_enable() call there is the only failure
mode.

As a note, we should probably check all systems that use it and see if
we can remove the clock management code in here as the pm_runtime will
also be doing this.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH 3/6] mmc: sh-mmcif: use devm_ for clock management

[Ben Dooks]

On 01/04/14 11:59, Geert Uytterhoeven wrote:
> On Tue, Apr 1, 2014 at 12:25 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> -       host->hclk = clk_get(&pdev->dev, NULL);
>> +       host->hclk = devm_clk_get(&pdev->dev, NULL);
>>          if (IS_ERR(host->hclk)) {
>>                  ret = PTR_ERR(host->hclk);
>>                  dev_err(&pdev->dev, "cannot get clock: %d\n", ret);
>> -               goto eclkget;
>> +               goto err_pm;
>>          }
>>          ret = sh_mmcif_clk_update(host);
>>          if (ret < 0)
>> -               goto eclkupdate;
>> +               goto err_clk;
>
> This goto doesn't look correct to me. If sh_mmcif_clk_update() failed,
> the clock was not enabled nor prepared. "goto err_pm"?

Yes, I think the clk_prepare_enable() call there is the only failure
mode.

As a note, we should probably check all systems that use it and see if
we can remove the clock management code in here as the pm_runtime will
also be doing this.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 4/6] mmc: sh-mmcif: use devm_ for irq management

[Ben Dooks]

Use devm_request_threaded_irq() for the host interrupt handlers so we
do not have to worry about freeing them on exit or error. Tidies up the
exit path code for the driver.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index c006310..485db7b 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1443,17 +1443,19 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
 
 	name = irq[1] < 0 ? dev_name(&pdev->dev) : "sh_mmc:error";
-	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, name, host);
+	ret = devm_request_threaded_irq(&pdev->dev, irq[0], sh_mmcif_intr,
+					sh_mmcif_irqt, 0, name, host);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq error (%s)\n", name);
-		goto ereqirq0;
+		goto err_irq;
 	}
 	if (irq[1] >= 0) {
-		ret = request_threaded_irq(irq[1], sh_mmcif_intr, sh_mmcif_irqt,
-					   0, "sh_mmc:int", host);
+		ret = devm_request_threaded_irq(&pdev->dev, irq[1],
+						sh_mmcif_intr, sh_mmcif_irqt,
+						0, "sh_mmc:int", host);
 		if (ret) {
 			dev_err(&pdev->dev, "request_irq error (sh_mmc:int)\n");
-			goto ereqirq1;
+			goto err_irq;
 		}
 	}
 
@@ -1480,11 +1482,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 
 emmcaddh:
 erqcd:
-	if (irq[1] >= 0)
-		free_irq(irq[1], host);
-ereqirq1:
-	free_irq(irq[0], host);
-ereqirq0:
+err_irq:
 	pm_runtime_suspend(&pdev->dev);
 err_clk:
 	clk_disable_unprepare(host->hclk);
@@ -1498,7 +1496,6 @@ err_host:
 static int sh_mmcif_remove(struct platform_device *pdev)
 {
 	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
-	int irq[2];
 
 	host->dying = true;
 	clk_prepare_enable(host->hclk);
@@ -1516,13 +1513,6 @@ static int sh_mmcif_remove(struct platform_device *pdev)
 	 */
 	cancel_delayed_work_sync(&host->timeout_work);
 
-	irq[0] = platform_get_irq(pdev, 0);
-	irq[1] = platform_get_irq(pdev, 1);
-
-	free_irq(irq[0], host);
-	if (irq[1] >= 0)
-		free_irq(irq[1], host);
-
 	clk_disable_unprepare(host->hclk);
 	mmc_free_host(host->mmc);
 	pm_runtime_put_sync(&pdev->dev);
-- 
1.9.0

[PATCH 4/6] mmc: sh-mmcif: use devm_ for irq management

[Ben Dooks]

Use devm_request_threaded_irq() for the host interrupt handlers so we
do not have to worry about freeing them on exit or error. Tidies up the
exit path code for the driver.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index c006310..485db7b 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1443,17 +1443,19 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
 
 	name = irq[1] < 0 ? dev_name(&pdev->dev) : "sh_mmc:error";
-	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, name, host);
+	ret = devm_request_threaded_irq(&pdev->dev, irq[0], sh_mmcif_intr,
+					sh_mmcif_irqt, 0, name, host);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq error (%s)\n", name);
-		goto ereqirq0;
+		goto err_irq;
 	}
 	if (irq[1] >= 0) {
-		ret = request_threaded_irq(irq[1], sh_mmcif_intr, sh_mmcif_irqt,
-					   0, "sh_mmc:int", host);
+		ret = devm_request_threaded_irq(&pdev->dev, irq[1],
+						sh_mmcif_intr, sh_mmcif_irqt,
+						0, "sh_mmc:int", host);
 		if (ret) {
 			dev_err(&pdev->dev, "request_irq error (sh_mmc:int)\n");
-			goto ereqirq1;
+			goto err_irq;
 		}
 	}
 
@@ -1480,11 +1482,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 
 emmcaddh:
 erqcd:
-	if (irq[1] >= 0)
-		free_irq(irq[1], host);
-ereqirq1:
-	free_irq(irq[0], host);
-ereqirq0:
+err_irq:
 	pm_runtime_suspend(&pdev->dev);
 err_clk:
 	clk_disable_unprepare(host->hclk);
@@ -1498,7 +1496,6 @@ err_host:
 static int sh_mmcif_remove(struct platform_device *pdev)
 {
 	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
-	int irq[2];
 
 	host->dying = true;
 	clk_prepare_enable(host->hclk);
@@ -1516,13 +1513,6 @@ static int sh_mmcif_remove(struct platform_device *pdev)
 	 */
 	cancel_delayed_work_sync(&host->timeout_work);
 
-	irq[0] = platform_get_irq(pdev, 0);
-	irq[1] = platform_get_irq(pdev, 1);
-
-	free_irq(irq[0], host);
-	if (irq[1] >= 0)
-		free_irq(irq[1], host);
-
 	clk_disable_unprepare(host->hclk);
 	mmc_free_host(host->mmc);
 	pm_runtime_put_sync(&pdev->dev);
-- 
1.9.0

[PATCH 5/6] mmc: sh-mmcif: no need to call pm_runtime_suspend on error

[Ben Dooks]

The pm_runtime call should implicitly disable the device once the
probe is over if there is no explicit reference gained. There is no
need to call pm_runtime_suspend() before the pm_runtime_disable()
call.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 485db7b..c951760 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1447,7 +1447,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 					sh_mmcif_irqt, 0, name, host);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq error (%s)\n", name);
-		goto err_irq;
+		goto err_clk;
 	}
 	if (irq[1] >= 0) {
 		ret = devm_request_threaded_irq(&pdev->dev, irq[1],
@@ -1455,7 +1455,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 						0, "sh_mmc:int", host);
 		if (ret) {
 			dev_err(&pdev->dev, "request_irq error (sh_mmc:int)\n");
-			goto err_irq;
+			goto err_clk;
 		}
 	}
 
@@ -1482,8 +1482,6 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 
 emmcaddh:
 erqcd:
-err_irq:
-	pm_runtime_suspend(&pdev->dev);
 err_clk:
 	clk_disable_unprepare(host->hclk);
 err_pm:
-- 
1.9.0

[PATCH 5/6] mmc: sh-mmcif: no need to call pm_runtime_suspend on error

[Ben Dooks]

The pm_runtime call should implicitly disable the device once the
probe is over if there is no explicit reference gained. There is no
need to call pm_runtime_suspend() before the pm_runtime_disable()
call.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 485db7b..c951760 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1447,7 +1447,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 					sh_mmcif_irqt, 0, name, host);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq error (%s)\n", name);
-		goto err_irq;
+		goto err_clk;
 	}
 	if (irq[1] >= 0) {
 		ret = devm_request_threaded_irq(&pdev->dev, irq[1],
@@ -1455,7 +1455,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 						0, "sh_mmc:int", host);
 		if (ret) {
 			dev_err(&pdev->dev, "request_irq error (sh_mmc:int)\n");
-			goto err_irq;
+			goto err_clk;
 		}
 	}
 
@@ -1482,8 +1482,6 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 
 emmcaddh:
 erqcd:
-err_irq:
-	pm_runtime_suspend(&pdev->dev);
 err_clk:
 	clk_disable_unprepare(host->hclk);
 err_pm:
-- 
1.9.0

[PATCH 6/6] mmc: sh-mmcif: final error path cleanup

[Ben Dooks]

Remove the error path items that are no longer needed. The mmc card-detect
code cleans up after itself (and registers with devm) and the host error
is the same as the clock disable.

This should also fix a double call to clk_disable_unprepare() if the call
to mmc_add_host() fails.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index c951760..d63cfa7 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1462,7 +1462,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 	if (pd && pd->use_cd_gpio) {
 		ret = mmc_gpio_request_cd(mmc, pd->cd_gpio, 0);
 		if (ret < 0)
-			goto erqcd;
+			goto err_clk;
 	}
 
 	mutex_init(&host->thread_lock);
@@ -1470,7 +1470,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 	clk_disable_unprepare(host->hclk);
 	ret = mmc_add_host(mmc);
 	if (ret < 0)
-		goto emmcaddh;
+		goto err_pm;
 
 	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
 
@@ -1480,8 +1480,6 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
 	return ret;
 
-emmcaddh:
-erqcd:
 err_clk:
 	clk_disable_unprepare(host->hclk);
 err_pm:
-- 
1.9.0

[PATCH 6/6] mmc: sh-mmcif: final error path cleanup

[Ben Dooks]

Remove the error path items that are no longer needed. The mmc card-detect
code cleans up after itself (and registers with devm) and the host error
is the same as the clock disable.

This should also fix a double call to clk_disable_unprepare() if the call
to mmc_add_host() fails.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/mmc/host/sh_mmcif.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index c951760..d63cfa7 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1462,7 +1462,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 	if (pd && pd->use_cd_gpio) {
 		ret = mmc_gpio_request_cd(mmc, pd->cd_gpio, 0);
 		if (ret < 0)
-			goto erqcd;
+			goto err_clk;
 	}
 
 	mutex_init(&host->thread_lock);
@@ -1470,7 +1470,7 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 	clk_disable_unprepare(host->hclk);
 	ret = mmc_add_host(mmc);
 	if (ret < 0)
-		goto emmcaddh;
+		goto err_pm;
 
 	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
 
@@ -1480,8 +1480,6 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
 	return ret;
 
-emmcaddh:
-erqcd:
 err_clk:
 	clk_disable_unprepare(host->hclk);
 err_pm:
-- 
1.9.0

resend - sh mmcif driver cleanup patch series

[Ben Dooks]

This is a re-send of the mmcif driver cleanups that seem to have
been missed. If possible, could this be merged for the current merge
window?

These should be available at:

	http://git.codethink.co.uk/linux.git bjdooks/315-rc8/mmcif

[PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Ben Dooks]

Start tidying the probe/release code by using devm_ioremap_resource() to
map the IO registers.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Fixes from v1:
	- return the PTR_ERR() from devm_ioremap
	- remove error print as it already shows an error
---
 drivers/mmc/host/sh_mmcif.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 7f819f1..3ed54c8 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1377,22 +1377,15 @@ static int sh_mmcif_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Get irq error\n");
 		return -ENXIO;
 	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "platform_get_resource error.\n");
-		return -ENXIO;
-	}
-	reg = ioremap(res->start, resource_size(res));
-	if (!reg) {
-		dev_err(&pdev->dev, "ioremap error.\n");
-		return -ENOMEM;
-	}
+	reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
 
 	mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
-	if (!mmc) {
-		ret = -ENOMEM;
-		goto ealloch;
-	}
+	if (!mmc)
+		return -ENOMEM;
 
 	ret = mmc_of_parse(mmc);
 	if (ret < 0)
@@ -1497,8 +1490,6 @@ eclkget:
 	pm_runtime_disable(&pdev->dev);
 eofparse:
 	mmc_free_host(mmc);
-ealloch:
-	iounmap(reg);
 	return ret;
 }
 
@@ -1523,9 +1514,6 @@ static int sh_mmcif_remove(struct platform_device *pdev)
 	 */
 	cancel_delayed_work_sync(&host->timeout_work);
 
-	if (host->addr)
-		iounmap(host->addr);
-
 	irq[0] = platform_get_irq(pdev, 0);
 	irq[1] = platform_get_irq(pdev, 1);
 
-- 
2.0.0.rc2

Re: [PATCH 2/6] mmc: sh-mmcif: use devm_ for ioremap

[Ulf Hansson]

On 4 June 2014 13:42, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> Start tidying the probe/release code by using devm_ioremap_resource() to
> map the IO registers.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
> Fixes from v1:
>         - return the PTR_ERR() from devm_ioremap
>         - remove error print as it already shows an error
> ---
>  drivers/mmc/host/sh_mmcif.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 7f819f1..3ed54c8 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1377,22 +1377,15 @@ static int sh_mmcif_probe(struct platform_device *pdev)
>                 dev_err(&pdev->dev, "Get irq error\n");
>                 return -ENXIO;
>         }
> +
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -       if (!res) {
> -               dev_err(&pdev->dev, "platform_get_resource error.\n");
> -               return -ENXIO;
> -       }
> -       reg = ioremap(res->start, resource_size(res));
> -       if (!reg) {
> -               dev_err(&pdev->dev, "ioremap error.\n");
> -               return -ENOMEM;
> -       }
> +       reg = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(reg))
> +               return PTR_ERR(reg);
>
>         mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
> -       if (!mmc) {
> -               ret = -ENOMEM;
> -               goto ealloch;
> -       }
> +       if (!mmc)
> +               return -ENOMEM;
>
>         ret = mmc_of_parse(mmc);
>         if (ret < 0)
> @@ -1497,8 +1490,6 @@ eclkget:
>         pm_runtime_disable(&pdev->dev);
>  eofparse:
>         mmc_free_host(mmc);
> -ealloch:
> -       iounmap(reg);
>         return ret;
>  }
>
> @@ -1523,9 +1514,6 @@ static int sh_mmcif_remove(struct platform_device *pdev)
>          */
>         cancel_delayed_work_sync(&host->timeout_work);
>
> -       if (host->addr)
> -               iounmap(host->addr);
> -
>         irq[0] = platform_get_irq(pdev, 0);
>         irq[1] = platform_get_irq(pdev, 1);
>
> --
> 2.0.0.rc2
>