spi/bcm63xx: various small cleanups and fixes

PATCH In review Jonas Gorski 2 participants 14 messages Mar 11, 2013 1 message not in the archive

Subsystems (2)

SPI Broadcom BCM6348/BCM6358 SPI controller driver

lore

Share & tools

lore

Jonas Gorski

author

2013-03-11 23:13 UTC #0 lore raw

cover spi/bcm63xx: various small cleanups and fixes

This patch series cleans up spi-bcm63xx and removes mostly redundant checks that are already done by the spi core system itself, and also adds some clock handling improvements. All in all nothing serious, just some corner cases.

The only Patch I'm not 100% sure about is patch 7/12 ("check spi bits_per_word in spi_setup"), because I'm not sure if setup is the right place for that.

For all we care the spi can claim what it wants as its bit_per_word as long as all transfers have it set to 8, so arguably we maybe shouldn't reject it then.

Jonas Gorski (11):
spi/bcm63xx: don't disable non enabled clocks in probe error path spi/bcm63xx: properly prepare clocks before enabling them spi/bcm63xx: remove duplicated mode bits check spi/bcm63xx: remove unneeded debug message spi/bcm63xx: remove unused variable bs from bcm63xx_spi_setup spi/bcm63xx: check spi bits_per_word in spi_setup spi/bcm63xx: simplify bcm63xx_spi_check_transfer spi/bcm63xx: remove spi chip select validity check spi/bcm63xx: inline bcm63xx_spi_check_transfer spi/bcm63xx: inline hz usage in bcm63xx_spi_setup_transfer spi/bcm63xx: use devm_ioremap_resource()

Kevin Cernekee (1):
spi/bcm63xx: Remove unused variable

 drivers/spi/spi-bcm63xx.c |   79 +++++++++++----------------------------------
 1 file changed, 19 insertions(+), 60 deletions(-)

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

A message that is not in the archive (sent off-list or not yet received): 1363043627-23524-1-git-send-email-jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org

Jonas Gorski

author

2013-03-11 23:13 UTC #2 lore raw

PATCH 1/12 spi/bcm63xx: Remove unused variable

From: Kevin Cernekee <[email protected]>

This fixes the following warning:

drivers/spi/spi-bcm63xx.c: In function 'bcm63xx_spi_setup':
drivers/spi/spi-bcm63xx.c:157:6: warning: unused variable 'ret'

Signed-off-by Kevin Cernekee <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |    1 -
 1 file changed, 1 deletion(-)

drivers/spi/spi-bcm63xx.c +0 -1

index 9578af7..0415a32 100644
@@ -152,7 +152,6 @@ static void bcm63xx_spi_setup_transfer(struct spi_device *spi,
 static int bcm63xx_spi_setup(struct spi_device *spi)
 {
 	struct bcm63xx_spi *bs;
-	int ret;
 
 	bs = spi_master_get_devdata(spi->master);

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #3 lore raw

PATCH 2/12 spi/bcm63xx: don't disable non enabled clocks in probe error path

In reply to a message not in the archive

When msg_ctl_width is set to an invalid value we try to disable the clock despite it never being enabled. Fix it by jumping to the correct label.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/spi/spi-bcm63xx.c +1 -1

index 0415a32..d7df435 100644
@@ -489,7 +489,7 @@ static int bcm63xx_spi_probe(struct platform_device *pdev)
 	default:
 		dev_err(dev, "unsupported MSG_CTL width: %d\n",
 			 bs->msg_ctl_width);
-		goto out_clk_disable;
+		goto out_err;
 	}
 
 	/* Initialize hardware */

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #4 lore raw

PATCH 3/12 spi/bcm63xx: properly prepare clocks before enabling them

In reply to a message not in the archive

Use proper clk_prepare/unprepare calls in preparation for switching to the generic clock framework.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/spi/spi-bcm63xx.c +5 -5

index d7df435..ef9b89f 100644
@@ -493,7 +493,7 @@ static int bcm63xx_spi_probe(struct platform_device *pdev)
 	}
 
 	/* Initialize hardware */
-	clk_enable(bs->clk);
+	clk_prepare_enable(bs->clk);
 	bcm_spi_writeb(bs, SPI_INTR_CLEAR_ALL, SPI_INT_STATUS);
 
 	/* register and we are done */

@@ -509,7 +509,7 @@ static int bcm63xx_spi_probe(struct platform_device *pdev)
 	return 0;
 
 out_clk_disable:
-	clk_disable(clk);
+	clk_disable_unprepare(clk);
 out_err:
 	platform_set_drvdata(pdev, NULL);
 	spi_master_put(master);

@@ -530,7 +530,7 @@ static int bcm63xx_spi_remove(struct platform_device *pdev)
 	bcm_spi_writeb(bs, 0, SPI_INT_MASK);
 
 	/* HW shutdown */
-	clk_disable(bs->clk);
+	clk_disable_unprepare(bs->clk);
 	clk_put(bs->clk);
 
 	platform_set_drvdata(pdev, 0);

@@ -549,7 +549,7 @@ static int bcm63xx_spi_suspend(struct device *dev)
 
 	spi_master_suspend(master);
 
-	clk_disable(bs->clk);
+	clk_disable_unprepare(bs->clk);
 
 	return 0;
 }

@@ -560,7 +560,7 @@ static int bcm63xx_spi_resume(struct device *dev)
 			platform_get_drvdata(to_platform_device(dev));
 	struct bcm63xx_spi *bs = spi_master_get_devdata(master);
 
-	clk_enable(bs->clk);
+	clk_prepare_enable(bs->clk);
 
 	spi_master_resume(master);

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #5 lore raw

PATCH 4/12 spi/bcm63xx: remove duplicated mode bits check

In reply to a message not in the archive

The spi subsystem already checks the mode bits before calling setup.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |    6 ------
 1 file changed, 6 deletions(-)

drivers/spi/spi-bcm63xx.c +0 -6

index ef9b89f..79ad8bc 100644
@@ -158,12 +158,6 @@ static int bcm63xx_spi_setup(struct spi_device *spi)
 	if (!spi->bits_per_word)
 		spi->bits_per_word = 8;
 
-	if (spi->mode & ~MODEBITS) {
-		dev_err(&spi->dev, "%s, unsupported mode bits %x\n",
-			__func__, spi->mode & ~MODEBITS);
-		return -EINVAL;
-	}
-
 	dev_dbg(&spi->dev, "%s, mode %d, %u bits/w, %u nsec/bit\n",
 		__func__, spi->mode & MODEBITS, spi->bits_per_word, 0);

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #6 lore raw

PATCH 5/12 spi/bcm63xx: remove unneeded debug message

In reply to a message not in the archive

The spi subsystem already provides this info in a more extensive debug print except for the nsecs/bit - which wasn't calculated anyway and fixed to 0.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |    3 ---
 1 file changed, 3 deletions(-)

drivers/spi/spi-bcm63xx.c +0 -3

index 79ad8bc..13806e3 100644
@@ -158,9 +158,6 @@ static int bcm63xx_spi_setup(struct spi_device *spi)
 	if (!spi->bits_per_word)
 		spi->bits_per_word = 8;
 
-	dev_dbg(&spi->dev, "%s, mode %d, %u bits/w, %u nsec/bit\n",
-		__func__, spi->mode & MODEBITS, spi->bits_per_word, 0);
-
 	return 0;
 }

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #7 lore raw

PATCH 6/12 spi/bcm63xx: remove unused variable bs from bcm63xx_spi_setup

In reply to a message not in the archive

It is only written, but never read.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |    4 ----
 1 file changed, 4 deletions(-)

drivers/spi/spi-bcm63xx.c +0 -4

index 13806e3..04c460e 100644
@@ -151,10 +151,6 @@ static void bcm63xx_spi_setup_transfer(struct spi_device *spi,
 
 static int bcm63xx_spi_setup(struct spi_device *spi)
 {
-	struct bcm63xx_spi *bs;
-
-	bs = spi_master_get_devdata(spi->master);
-
 	if (!spi->bits_per_word)
 		spi->bits_per_word = 8;

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #8 lore raw

PATCH 7/12 spi/bcm63xx: check spi bits_per_word in spi_setup

In reply to a message not in the archive

Instead of fixing up the bits_per_word (which the spi subsystem already does for us), check it for supported values.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/spi/spi-bcm63xx.c +5 -2

index 04c460e..b64229c 100644
@@ -151,8 +151,11 @@ static void bcm63xx_spi_setup_transfer(struct spi_device *spi,
 
 static int bcm63xx_spi_setup(struct spi_device *spi)
 {
-	if (!spi->bits_per_word)
-		spi->bits_per_word = 8;
+	if (spi->bits_per_word != 8) {
+		dev_err(&spi->dev, "%s, unsupported bits_per_word=%d\n",
+			__func__, spi->bits_per_word);
+		return -EINVAL;
+	}
 
 	return 0;
 }

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #9 lore raw

PATCH 8/12 spi/bcm63xx: simplify bcm63xx_spi_check_transfer

In reply to a message not in the archive

bcm63xx_spi_check_transfer is only called from one place that has t always set, so directly check the transfer's bits_per_word.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/spi/spi-bcm63xx.c +2 -5

index b64229c..b9c9431 100644
@@ -96,12 +96,9 @@ static const unsigned bcm63xx_spi_freq_table[SPI_CLK_MASK][2] = {
 static int bcm63xx_spi_check_transfer(struct spi_device *spi,
 					struct spi_transfer *t)
 {
-	u8 bits_per_word;
-
-	bits_per_word = (t) ? t->bits_per_word : spi->bits_per_word;
-	if (bits_per_word != 8) {
+	if (t->bits_per_word != 8) {
 		dev_err(&spi->dev, "%s, unsupported bits_per_word=%d\n",
-			__func__, bits_per_word);
+			__func__, t->bits_per_word);
 		return -EINVAL;
 	}

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #10 lore raw

PATCH 9/12 spi/bcm63xx: remove spi chip select validity check

In reply to a message not in the archive

The check would belong in bcm63xx_spi_setup if the spi subsystem weren't already doing the check for us, so just drop it.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |    6 ------
 1 file changed, 6 deletions(-)

drivers/spi/spi-bcm63xx.c +0 -6

index b9c9431..9574e47 100644
@@ -102,12 +102,6 @@ static int bcm63xx_spi_check_transfer(struct spi_device *spi,
 		return -EINVAL;
 	}
 
-	if (spi->chip_select > spi->master->num_chipselect) {
-		dev_err(&spi->dev, "%s, unsupported slave %d\n",
-			__func__, spi->chip_select);
-		return -EINVAL;
-	}
-
 	return 0;
 }

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #11 lore raw

PATCH 10/12 spi/bcm63xx: inline bcm63xx_spi_check_transfer

In reply to a message not in the archive

It only does one check, so just do the check directly in the caller.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

drivers/spi/spi-bcm63xx.c +5 ~~-14~~

index 9574e47..d777f63 100644
@@ -93,18 +93,6 @@ static const unsigned bcm63xx_spi_freq_table[SPI_CLK_MASK][2] = {
 	{   391000, SPI_CLK_0_391MHZ }
 };
 
-static int bcm63xx_spi_check_transfer(struct spi_device *spi,
-					struct spi_transfer *t)
-{
-	if (t->bits_per_word != 8) {
-		dev_err(&spi->dev, "%s, unsupported bits_per_word=%d\n",
-			__func__, t->bits_per_word);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static void bcm63xx_spi_setup_transfer(struct spi_device *spi,
 				      struct spi_transfer *t)
 {

@@ -293,9 +281,12 @@ static int bcm63xx_spi_transfer_one(struct spi_master *master,
 	 * full-duplex transfers.
 	 */
 	list_for_each_entry(t, &m->transfers, transfer_list) {
-		status = bcm63xx_spi_check_transfer(spi, t);
-		if (status < 0)
+		if (t->bits_per_word != 8) {
+			dev_err(&spi->dev, "%s, unsupported bits_per_word=%d\n",
+				__func__, t->bits_per_word);
+			status = -EINVAL;
 			goto exit;
+		}
 
 		if (!first)
 			first = t;

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #12 lore raw

PATCH 11/12 spi/bcm63xx: inline hz usage in bcm63xx_spi_setup_transfer

In reply to a message not in the archive

bcm63xx_spi_setup_transfer is called from only one place, and that has t always set, to hz will always be t->speed_hz - just use it directly in the two places instead of moving it in a local variable.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/spi/spi-bcm63xx.c +2 -5

index d777f63..2d64db4 100644
@@ -97,15 +97,12 @@ static void bcm63xx_spi_setup_transfer(struct spi_device *spi,
 				      struct spi_transfer *t)
 {
 	struct bcm63xx_spi *bs = spi_master_get_devdata(spi->master);
-	u32 hz;
 	u8 clk_cfg, reg;
 	int i;
 
-	hz = (t) ? t->speed_hz : spi->max_speed_hz;
-
 	/* Find the closest clock configuration */
 	for (i = 0; i < SPI_CLK_MASK; i++) {
-		if (hz >= bcm63xx_spi_freq_table[i][0]) {
+		if (t->speed_hz >= bcm63xx_spi_freq_table[i][0]) {
 			clk_cfg = bcm63xx_spi_freq_table[i][1];
 			break;
 		}

@@ -122,7 +119,7 @@ static void bcm63xx_spi_setup_transfer(struct spi_device *spi,
 
 	bcm_spi_writeb(bs, reg, SPI_CLK_CFG);
 	dev_dbg(&spi->dev, "Setting clock register to %02x (hz %d)\n",
-		clk_cfg, hz);
+		clk_cfg, t->speed_hz);
 }
 
 /* the spi->mode bits understood by this driver: */

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Jonas Gorski

author

2013-03-11 23:13 UTC #13 lore raw

PATCH 12/12 spi/bcm63xx: use devm_ioremap_resource()

In reply to a message not in the archive

Use devm_ioremap_resource() which provides its own error messages.

Signed-off-by Jonas Gorski <[email protected]>

---
 drivers/spi/spi-bcm63xx.c |   15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

drivers/spi/spi-bcm63xx.c +3 ~~-12~~

index 2d64db4..973099b 100644
@@ -412,18 +412,9 @@ static int bcm63xx_spi_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, master);
 	bs->pdev = pdev;
 
-	if (!devm_request_mem_region(&pdev->dev, r->start,
-					resource_size(r), PFX)) {
-		dev_err(dev, "iomem request failed\n");
-		ret = -ENXIO;
-		goto out_err;
-	}
-
-	bs->regs = devm_ioremap_nocache(&pdev->dev, r->start,
-							resource_size(r));
-	if (!bs->regs) {
-		dev_err(dev, "unable to ioremap regs\n");
-		ret = -ENOMEM;
+	bs->regs = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(bs->regs)) {
+		ret = PTR_ERR(bs->regs);
 		goto out_err;
 	}

Signature · 9 lines

1.7.10.4


------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev

Florian Fainelli

Acked-by

2013-03-11 23:20 UTC #14 lore raw

In reply to a message not in the archive

Quoted 5 lines · On Tuesday 12 March 2013 00:13:35 Jonas Gorski wrote:

This patch series cleans up spi-bcm63xx and removes mostly redundant checks that are already done by the spi core system itself, and also adds some clock handling improvements. All in all nothing serious, just some corner cases.

Quoted 7 lines

The only Patch I'm not 100% sure about is patch 7/12 ("check spi bits_per_word in spi_setup"), because I'm not sure if setup is the right place for that.
For all we care the spi can claim what it wants as its bit_per_word as long as all transfers have it set to 8, so arguably we maybe shouldn't reject it then.

For the entire serie:

Acked-by Florian Fainelli <[email protected]>

thanks Jonas!

Signature · 8 lines

Florian

------------------------------------------------------------------------------
Symantec Endpoint Protection 12 positioned as A LEADER in The Forrester  
Wave(TM): Endpoint Security, Q1 2013 and "remains a good choice" in the  
endpoint security space. For insight on selecting the right partner to 
tackle endpoint security challenges, access the full report. 
http://p.sf.net/sfu/symantec-dev2dev