[PATCH 1/2] spi/s3c64xx: Convert to devm_request_and_ioremap()

[PATCH 1/2] spi/s3c64xx: Convert to devm_request_and_ioremap()

[Mark Brown]

Saves some error handling and a small amount of code.

Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-s3c64xx.c |   25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index f4e2341..b7aeb5d 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1028,19 +1028,7 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 	/* the spi->mode bits understood by this driver: */
 	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
 
-	if (request_mem_region(mem_res->start,
-			resource_size(mem_res), pdev->name) == NULL) {
-		dev_err(&pdev->dev, "Req mem region failed\n");
-		ret = -ENXIO;
-		goto err0;
-	}
-
-	sdd->regs = ioremap(mem_res->start, resource_size(mem_res));
-	if (sdd->regs == NULL) {
-		dev_err(&pdev->dev, "Unable to remap IO\n");
-		ret = -ENXIO;
-		goto err1;
-	}
+	sdd->regs = devm_request_and_ioremap(&pdev->dev, mem_res);
 
 	if (sci->cfg_gpio == NULL || sci->cfg_gpio(pdev)) {
 		dev_err(&pdev->dev, "Unable to config gpio\n");
@@ -1124,10 +1112,6 @@ err4:
 	clk_put(sdd->clk);
 err3:
 err2:
-	iounmap((void *) sdd->regs);
-err1:
-	release_mem_region(mem_res->start, resource_size(mem_res));
-err0:
 	platform_set_drvdata(pdev, NULL);
 	spi_master_put(master);
 
@@ -1138,7 +1122,6 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 {
 	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
-	struct resource	*mem_res;
 
 	pm_runtime_disable(&pdev->dev);
 
@@ -1154,12 +1137,6 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 	clk_disable(sdd->clk);
 	clk_put(sdd->clk);
 
-	iounmap((void *) sdd->regs);
-
-	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (mem_res != NULL)
-		release_mem_region(mem_res->start, resource_size(mem_res));
-
 	platform_set_drvdata(pdev, NULL);
 	spi_master_put(master);
 
-- 
1.7.10


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

[PATCH 2/2] spi/s3c64xx: Expand S3C64XX_SPI_{DE,}ACT macros at call sites

[Mark Brown]

They have very few users and they're both just doing a single register
write so the advantage of having the macro is a bit limited. An inline
function might make sense but it's as easy to just do the writes directly.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-s3c64xx.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index b7aeb5d..3514ef9 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -74,11 +74,6 @@
 #define S3C64XX_SPI_SLAVE_AUTO			(1<<1)
 #define S3C64XX_SPI_SLAVE_SIG_INACT		(1<<0)
 
-#define S3C64XX_SPI_ACT(c) writel(0, (c)->regs + S3C64XX_SPI_SLAVE_SEL)
-
-#define S3C64XX_SPI_DEACT(c) writel(S3C64XX_SPI_SLAVE_SIG_INACT, \
-					(c)->regs + S3C64XX_SPI_SLAVE_SEL)
-
 #define S3C64XX_SPI_INT_TRAILING_EN		(1<<6)
 #define S3C64XX_SPI_INT_RX_OVERRUN_EN		(1<<5)
 #define S3C64XX_SPI_INT_RX_UNDERRUN_EN		(1<<4)
@@ -712,14 +707,15 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 		enable_cs(sdd, spi);
 
 		/* Start the signals */
-		S3C64XX_SPI_ACT(sdd);
+		writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 
 		spin_unlock_irqrestore(&sdd->lock, flags);
 
 		status = wait_for_xfer(sdd, xfer, use_dma);
 
 		/* Quiese the signals */
-		S3C64XX_SPI_DEACT(sdd);
+		writel(S3C64XX_SPI_SLAVE_SIG_INACT,
+		       sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 
 		if (status) {
 			dev_err(&spi->dev, "I/O Error: "
@@ -923,7 +919,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
 
 	sdd->cur_speed = 0;
 
-	S3C64XX_SPI_DEACT(sdd);
+	writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 
 	/* Disable Interrupts - we use Polling if not DMA mode */
 	writel(0, regs + S3C64XX_SPI_INT_EN);
-- 
1.7.10

Re: [PATCH 1/2] spi/s3c64xx: Convert to devm_request_and_ioremap()

[Sylwester Nawrocki]

On 07/04/2012 06:11 PM, Mark Brown wrote:
> Saves some error handling and a small amount of code.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/spi/spi-s3c64xx.c |   25 +------------------------
>  1 file changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index f4e2341..b7aeb5d 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1028,19 +1028,7 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
>  	/* the spi->mode bits understood by this driver: */
>  	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
>  
> -	if (request_mem_region(mem_res->start,
> -			resource_size(mem_res), pdev->name) == NULL) {
> -		dev_err(&pdev->dev, "Req mem region failed\n");
> -		ret = -ENXIO;
> -		goto err0;
> -	}
> -
> -	sdd->regs = ioremap(mem_res->start, resource_size(mem_res));
> -	if (sdd->regs == NULL) {
> -		dev_err(&pdev->dev, "Unable to remap IO\n");
> -		ret = -ENXIO;
> -		goto err1;
> -	}
> +	sdd->regs = devm_request_and_ioremap(&pdev->dev, mem_res);

It doesn't seem right. Why is is the check for valid sdd->regs removed ?
This should have rather been:

+	sdd->regs = devm_request_and_ioremap(&pdev->dev, mem_res);
+	if (sdd->regs == NULL) {
+		dev_err(&pdev->dev, "Failed to request memory region\n");
+		return -ENXIO;
+	}

Currently whne devm_request_and_ioremap() fails and returns NULL kernel oops
would happen right on first writel() in s3c64xx_spi_hwninit().


Thanks,
Sylwester

>  	if (sci->cfg_gpio == NULL || sci->cfg_gpio(pdev)) {
>  		dev_err(&pdev->dev, "Unable to config gpio\n");
> @@ -1124,10 +1112,6 @@ err4:
>  	clk_put(sdd->clk);
>  err3:
>  err2:
> -	iounmap((void *) sdd->regs);
> -err1:
> -	release_mem_region(mem_res->start, resource_size(mem_res));
> -err0:
>  	platform_set_drvdata(pdev, NULL);
>  	spi_master_put(master);
>  
> @@ -1138,7 +1122,6 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
>  {
>  	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
>  	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
> -	struct resource	*mem_res;
>  
>  	pm_runtime_disable(&pdev->dev);
>  
> @@ -1154,12 +1137,6 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
>  	clk_disable(sdd->clk);
>  	clk_put(sdd->clk);
>  
> -	iounmap((void *) sdd->regs);
> -
> -	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (mem_res != NULL)
> -		release_mem_region(mem_res->start, resource_size(mem_res));
> -
>  	platform_set_drvdata(pdev, NULL);
>  	spi_master_put(master);

Re: [PATCH 1/2] spi/s3c64xx: Convert to devm_request_and_ioremap()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

On Thu, Jul 05, 2012 at 10:41:04AM +0200, Sylwester Nawrocki wrote:

> It doesn't seem right. Why is is the check for valid sdd->regs removed ?
> This should have rather been:

Mostly just because the structure of the code is a bit error prone when
making quick updates with the if statement afterwards that looks like
error handling.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] spi/s3c64xx: Convert to devm_request_and_ioremap()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

On Thu, Jun 28, 2012 at 08:51:44PM +0200, Linus Walleij wrote:

> I'm starting to wonder if it would not be possible to mass-convert these
> using coccinelle.

Probably, yes.  I'm not actually going through these particularly,
really what I'm doing is looking to find drivers I run on my systems
that might be able to use regmap-mmio and fixing things I find as I
look.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] spi/s3c64xx: Convert to devm_request_and_ioremap()

[Linus Walleij]

On Thu, Jun 28, 2012 at 3:23 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> Saves some error handling and a small amount of code.
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Elegant, monsieur.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

I'm starting to wonder if it would not be possible to mass-convert these
using coccinelle.

Yours,
Linus Walleij

[PATCH 1/2] spi/s3c64xx: Convert to devm_request_and_ioremap()

[Mark Brown]

Saves some error handling and a small amount of code.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/spi/spi-s3c64xx.c |   25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index f4e2341..b7aeb5d 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1028,19 +1028,7 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 	/* the spi->mode bits understood by this driver: */
 	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
 
-	if (request_mem_region(mem_res->start,
-			resource_size(mem_res), pdev->name) == NULL) {
-		dev_err(&pdev->dev, "Req mem region failed\n");
-		ret = -ENXIO;
-		goto err0;
-	}
-
-	sdd->regs = ioremap(mem_res->start, resource_size(mem_res));
-	if (sdd->regs == NULL) {
-		dev_err(&pdev->dev, "Unable to remap IO\n");
-		ret = -ENXIO;
-		goto err1;
-	}
+	sdd->regs = devm_request_and_ioremap(&pdev->dev, mem_res);
 
 	if (sci->cfg_gpio == NULL || sci->cfg_gpio(pdev)) {
 		dev_err(&pdev->dev, "Unable to config gpio\n");
@@ -1124,10 +1112,6 @@ err4:
 	clk_put(sdd->clk);
 err3:
 err2:
-	iounmap((void *) sdd->regs);
-err1:
-	release_mem_region(mem_res->start, resource_size(mem_res));
-err0:
 	platform_set_drvdata(pdev, NULL);
 	spi_master_put(master);
 
@@ -1138,7 +1122,6 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 {
 	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
-	struct resource	*mem_res;
 
 	pm_runtime_disable(&pdev->dev);
 
@@ -1154,12 +1137,6 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 	clk_disable(sdd->clk);
 	clk_put(sdd->clk);
 
-	iounmap((void *) sdd->regs);
-
-	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (mem_res != NULL)
-		release_mem_region(mem_res->start, resource_size(mem_res));
-
 	platform_set_drvdata(pdev, NULL);
 	spi_master_put(master);
 
-- 
1.7.10