spi: fsl-(e)spi: migrate to using devm_ functions to simplify cleanup

PATCH In review Heiner Kallweit 2 participants 5 messages Aug 23, 2015 – Aug 25, 2015 3 messages not in the archive SPI

lore

Share & tools

lore

Heiner Kallweit

author

2015-08-23 12:20 UTC #0 lore raw

PATCH 1/3 spi: fsl-(e)spi: migrate to using devm_ functions to simplify cleanup

Migrate fsl-espi and fsl-spi to using the managed devm_ functions for resource handling. This simplifies the cleanup.

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/spi/spi-fsl-espi.c | 16 ++++++----------
 drivers/spi/spi-fsl-lib.c  |  4 ----
 drivers/spi/spi-fsl-spi.c  | 22 +++++++++-------------
 3 files changed, 15 insertions(+), 27 deletions(-)

drivers/spi/spi-fsl-espi.c +6 ~~-10~~

index d3f05a0..bfd6259 100644
@@ -606,7 +606,6 @@ static irqreturn_t fsl_espi_irq(s32 irq, void *context_data)
 
 static void fsl_espi_remove(struct mpc8xxx_spi *mspi)
 {
-	iounmap(mspi->reg_base);
 }
 
 static int fsl_espi_suspend(struct spi_master *master)

@@ -673,7 +672,8 @@ static struct spi_master * fsl_espi_probe(struct device *dev,
 	mpc8xxx_spi = spi_master_get_devdata(master);
 	mpc8xxx_spi->spi_remove = fsl_espi_remove;
 
-	mpc8xxx_spi->reg_base = ioremap(mem->start, resource_size(mem));
+	mpc8xxx_spi->reg_base = devm_ioremap(dev, mem->start,
+					     resource_size(mem));
 	if (!mpc8xxx_spi->reg_base) {
 		ret = -ENOMEM;
 		goto err_probe;

@@ -682,10 +682,10 @@ static struct spi_master * fsl_espi_probe(struct device *dev,
 	reg_base = mpc8xxx_spi->reg_base;
 
 	/* Register for SPI Interrupt */
-	ret = request_irq(mpc8xxx_spi->irq, fsl_espi_irq,
+	ret = devm_request_irq(dev, mpc8xxx_spi->irq, fsl_espi_irq,
 			  0, "fsl_espi", mpc8xxx_spi);
 	if (ret)
-		goto free_irq;
+		goto err_probe;
 
 	if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
 		mpc8xxx_spi->rx_shift = 16;

@@ -731,18 +731,14 @@ static struct spi_master * fsl_espi_probe(struct device *dev,
 
 	mpc8xxx_spi_write_reg(&reg_base->mode, regval);
 
-	ret = spi_register_master(master);
+	ret = devm_spi_register_master(dev, master);
 	if (ret < 0)
-		goto unreg_master;
+		goto err_probe;
 
 	dev_info(dev, "at 0x%p (irq = %d)\n", reg_base, mpc8xxx_spi->irq);
 
 	return master;
 
-unreg_master:
-	free_irq(mpc8xxx_spi->irq, mpc8xxx_spi);
-free_irq:
-	iounmap(mpc8xxx_spi->reg_base);
 err_probe:
 	spi_master_put(master);
 err:

drivers/spi/spi-fsl-lib.c +0 -4

index cb35d2f..e50fd06 100644
@@ -122,10 +122,6 @@ int mpc8xxx_spi_remove(struct device *dev)
 	master = dev_get_drvdata(dev);
 	mpc8xxx_spi = spi_master_get_devdata(master);
 
-	spi_unregister_master(master);
-
-	free_irq(mpc8xxx_spi->irq, mpc8xxx_spi);
-
 	if (mpc8xxx_spi->spi_remove)
 		mpc8xxx_spi->spi_remove(mpc8xxx_spi);

drivers/spi/spi-fsl-spi.c +9 ~~-13~~

index 60c5907..fbbab60 100644
@@ -561,7 +561,6 @@ static irqreturn_t fsl_spi_irq(s32 irq, void *context_data)
 
 static void fsl_spi_remove(struct mpc8xxx_spi *mspi)
 {
-	iounmap(mspi->reg_base);
 	fsl_spi_cpm_free(mspi);
 }

@@ -639,10 +638,11 @@ static struct spi_master * fsl_spi_probe(struct device *dev,
 	if (ret)
 		goto err_cpm_init;
 
-	mpc8xxx_spi->reg_base = ioremap(mem->start, resource_size(mem));
+	mpc8xxx_spi->reg_base = devm_ioremap(dev, mem->start,
+					     resource_size(mem));
 	if (mpc8xxx_spi->reg_base == NULL) {
 		ret = -ENOMEM;
-		goto err_ioremap;
+		goto err_probe;
 	}
 
 	if (mpc8xxx_spi->type == TYPE_GRLIB)

@@ -661,11 +661,11 @@ static struct spi_master * fsl_spi_probe(struct device *dev,
 					&mpc8xxx_spi->tx_shift, 8, 1);
 
 	/* Register for SPI Interrupt */
-	ret = request_irq(mpc8xxx_spi->irq, fsl_spi_irq,
-			  0, "fsl_spi", mpc8xxx_spi);
+	ret = devm_request_irq(dev, mpc8xxx_spi->irq, fsl_spi_irq,
+			       0, "fsl_spi", mpc8xxx_spi);
 
 	if (ret != 0)
-		goto free_irq;
+		goto err_probe;
 
 	reg_base = mpc8xxx_spi->reg_base;

@@ -686,20 +686,16 @@ static struct spi_master * fsl_spi_probe(struct device *dev,
 
 	mpc8xxx_spi_write_reg(&reg_base->mode, regval);
 
-	ret = spi_register_master(master);
+	ret = devm_spi_register_master(dev, master);
 	if (ret < 0)
-		goto unreg_master;
+		goto err_probe;
 
 	dev_info(dev, "at 0x%p (irq = %d), %s mode\n", reg_base,
 		 mpc8xxx_spi->irq, mpc8xxx_spi_strmode(mpc8xxx_spi->flags));
 
 	return master;
 
-unreg_master:
-	free_irq(mpc8xxx_spi->irq, mpc8xxx_spi);
-free_irq:
-	iounmap(mpc8xxx_spi->reg_base);
-err_ioremap:
+err_probe:
 	fsl_spi_cpm_free(mpc8xxx_spi);
 err_cpm_init:
 	spi_master_put(master);

Signature · 6 lines

2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Brown

2015-08-25 17:26 UTC #2 lore raw

In reply to a message not in the archive

drivers/spi/spi-fsl-espi.c static struct spi_master * fsl_espi_probe(struct device *dev, · line 676

Quoted 3 lines · Heiner Kallweit

drivers/spi/spi-fsl-espi.c +2 -1

-	mpc8xxx_spi->reg_base = ioremap(mem->start, resource_size(mem));
+	mpc8xxx_spi->reg_base = devm_ioremap(dev, mem->start,
+					     resource_size(mem));

What would be even nicer here would be to convert to devm_ioremap_resource()

Mark Brown

2015-08-25 17:43 UTC #5 lore raw

In reply to a message not in the archive

drivers/spi/spi-fsl-espi.c static struct spi_master * fsl_espi_probe(struct device *dev, · line 686

Quoted 4 lines · Heiner Kallweit

drivers/spi/spi-fsl-espi.c +1 -1

 	/* Register for SPI Interrupt */
-	ret = request_irq(mpc8xxx_spi->irq, fsl_espi_irq,
+	ret = devm_request_irq(dev, mpc8xxx_spi->irq, fsl_espi_irq,
 			  0, "fsl_espi", mpc8xxx_spi);

Also, are you *sure* this conversion is safe? One trick with interrupts is that the interrupt could still fire after the device removal function runs since devm_ dealloacation runs after that.

Heiner Kallweit

author

2015-08-25 19:54 UTC #4 lore raw

In reply to a message not in the archive

Am 25.08.2015 um 19:26 schrieb Mark Brown:

Quoted 7 lines · Heiner Kallweit

drivers/spi/spi-fsl-espi.c static struct spi_master * fsl_espi_probe(struct device *dev, · line 676
Quoted 4 lines
drivers/spi/spi-fsl-espi.c +2 -1
-	mpc8xxx_spi->reg_base = ioremap(mem->start, resource_size(mem));
+	mpc8xxx_spi->reg_base = devm_ioremap(dev, mem->start,
+					     resource_size(mem));
What would be even nicer here would be to convert to devm_ioremap_resource()

Thanks for the hint, I'll check.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Heiner Kallweit

author

2015-08-25 20:14 UTC #7 lore raw

In reply to a message not in the archive

Am 25.08.2015 um 19:43 schrieb Mark Brown:

Quoted 10 lines · Heiner Kallweit

drivers/spi/spi-fsl-espi.c static struct spi_master * fsl_espi_probe(struct device *dev, · line 686
Quoted 5 lines
drivers/spi/spi-fsl-espi.c +1 -1
 	/* Register for SPI Interrupt */
-	ret = request_irq(mpc8xxx_spi->irq, fsl_espi_irq,
+	ret = devm_request_irq(dev, mpc8xxx_spi->irq, fsl_espi_irq,
 			  0, "fsl_espi", mpc8xxx_spi);
Also, are you *sure* this conversion is safe? One trick with interrupts is that the interrupt could still fire after the device removal function runs since devm_ dealloacation runs after that.

After each transfer interrupts are disabled (at end of fsl_espi_bufs). Therefore I *think* it's safe.
And as resources are freed in reverse order (irq first), shouldn't this be enough to be on the safe side?

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html