Skip to contentSkip to conversation

SPI fixes for documentation and spi_s3c24xx driver

PATCH In review Ben Dooks 2 participants 10 messages Aug 13, 2009 – Aug 14, 2009

Subsystems (2)
SPI Arm Samsung Exynos
lore
Share & tools
lore

Ben Dooks

author
#0 lore raw

cover SPI fixes for documentation and spi_s3c24xx driver

Fix spello in the SPI documentation and fix the behaviour and bugs in the spi_s3c24xx driver.

author
#1 lore raw

PATCH 1/7 spi: fix spelling of automatically in documentation

Fix spelling of automatically in Documentation/spi/spi-summary.

Signed-off-by Ben Dooks <[email protected]>
---
 Documentation/spi/spi-summary |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Documentation/spi/spi-summary +1 -1
Index: b/Documentation/spi/spi-summary
===================================================================
@@ -350,7 +350,7 @@ SPI protocol drivers somewhat resemble p
 		.resume		= CHIP_resume,
 	};
 
-The driver core will autmatically attempt to bind this driver to any SPI
+The driver core will automatically attempt to bind this driver to any SPI
 device whose board_info gave a modalias of "CHIP".  Your probe() code
 might look like this unless you're creating a device which is managing
 a bus (appearing under /sys/class/spi_master).
author
#2 lore raw

PATCH 2/7 spi_s3c24xx: fix clock rate calculation

In reply to Ben Dooks

Currently the clock rate calculation may round as pleased, which means that it is possible that we will round down and end up with a faster clock rate than intended.

Change the calculation to use DIV_ROUND_UP() to ensure that we end up with a clock rate either the same as or lower than the user requested one.

Signed-off-by Ben Dooks <[email protected]>
---
 drivers/spi/spi_s3c24xx.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
drivers/spi/spi_s3c24xx.c +7 -10
Index: b/drivers/spi/spi_s3c24xx.c
===================================================================
@@ -111,6 +111,7 @@ static int s3c24xx_spi_setupxfer(struct 
 	unsigned int bpw;
 	unsigned int hz;
 	unsigned int div;
+	unsigned long clk;
 
 	bpw = t ? t->bits_per_word : spi->bits_per_word;
 	hz  = t ? t->speed_hz : spi->max_speed_hz;

@@ -120,20 +121,16 @@ static int s3c24xx_spi_setupxfer(struct 
 		return -EINVAL;
 	}
 
-	div = clk_get_rate(hw->clk) / hz;
-
-	/* is clk = pclk / (2 * (pre+1)), or is it
-	 *    clk = (pclk * 2) / ( pre + 1) */
-
-	div /= 2;
-
-	if (div > 0)
-		div -= 1;
+	clk = clk_get_rate(hw->clk);
+	div = DIV_ROUND_UP(clk, hz * 2) - 1;
 
 	if (div > 255)
 		div = 255;
 
-	dev_dbg(&spi->dev, "setting pre-scaler to %d (hz %d)\n", div, hz);
+	dev_dbg(&spi->dev, "setting pre-scaler to %d (wanted %d, got %ld)\n",
+		div, hz, clk / (2 * (div + 1)));
+
+
 	writeb(div, hw->regs + S3C2410_SPPRE);
 
 	spin_lock(&hw->bitbang.lock);
author
#3 lore raw

PATCH 3/7 spi_s3c24xx; Fix transfer setup code

In reply to Ben Dooks

Since the changes to the bitbang driver, there is the possibility we will be called with either the speed_hz or bpw values zero. We take these to mean that the default values (8 bits per word, or maximum bus speed).

Signed-off-by Ben Dooks <[email protected]>

---

drivers/spi/spi_s3c24xx.c +6 -0
Index: b/drivers/spi/spi_s3c24xx.c
===================================================================
@@ -116,6 +116,12 @@ static int s3c24xx_spi_setupxfer(struct 
 	bpw = t ? t->bits_per_word : spi->bits_per_word;
 	hz  = t ? t->speed_hz : spi->max_speed_hz;
 
+	if (!bpw)
+		bpw = 8;
+
+	if (!hz)
+		hz = spi->max_speed_hz;
+
 	if (bpw != 8) {
 		dev_err(&spi->dev, "invalid bits-per-word (%d)\n", bpw);
 		return -EINVAL;
author
#4 lore raw

PATCH 4/7 spi_s3c24xx: fix header includes

In reply to Ben Dooks

The driver includes <asm/io.h> where it should be including <linux/io.h> and also includes <mach/hardware.h> and <asm/dma.h> without using anything from these.

Signed-off-by; Ben Dooks <[email protected]>

---
 drivers/spi/spi_s3c24xx.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/spi/spi_s3c24xx.c +1 -4
Index: b/drivers/spi/spi_s3c24xx.c
===================================================================
@@ -20,14 +20,11 @@
 #include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
+#include <linux/io.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_bitbang.h>
 
-#include <asm/io.h>
-#include <asm/dma.h>
-#include <mach/hardware.h>
-
 #include <plat/regs-spi.h>
 #include <mach/spi.h>
author
#5 lore raw

PATCH 5/7 spi_s3c24xx: use resource_size() to get resource size

In reply to Ben Dooks

Change the use of (res->end - res->start) to use resource_size() to get the size of the resource.

Signed-off-by Ben Dooks <[email protected]>
---
 drivers/spi/spi_s3c24xx.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/spi_s3c24xx.c +2 -2
Index: b/drivers/spi/spi_s3c24xx.c
===================================================================
@@ -299,7 +299,7 @@ static int __init s3c24xx_spi_probe(stru
 		goto err_no_iores;
 	}
 
-	hw->ioarea = request_mem_region(res->start, (res->end - res->start)+1,
+	hw->ioarea = request_mem_region(res->start, resource_size(res),
 					pdev->name);
 
 	if (hw->ioarea == NULL) {

@@ -308,7 +308,7 @@ static int __init s3c24xx_spi_probe(stru
 		goto err_no_iores;
 	}
 
-	hw->regs = ioremap(res->start, (res->end - res->start)+1);
+	hw->regs = ioremap(res->start, resource_size(res));
 	if (hw->regs == NULL) {
 		dev_err(&pdev->dev, "Cannot map IO\n");
 		err = -ENXIO;
author
#6 lore raw

PATCH 6/7 spi_s3c24xx: use dev_pm_ops

In reply to Ben Dooks

Change the spi_s3c24xx driver to use dev_pm_ops to avoid the following warning during probe:

Platform driver 's3c2410-spi' needs updating - please use dev_pm_ops

Signed-off-by Ben Dooks <[email protected]>
---
 drivers/spi/spi_s3c24xx.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)
drivers/spi/spi_s3c24xx.c +13 -9
Index: b/drivers/spi/spi_s3c24xx.c
===================================================================
@@ -418,9 +418,9 @@ static int __exit s3c24xx_spi_remove(str
 
 #ifdef CONFIG_PM
 
-static int s3c24xx_spi_suspend(struct platform_device *pdev, pm_message_t msg)
+static int s3c24xx_spi_suspend(struct device *dev)
 {
-	struct s3c24xx_spi *hw = platform_get_drvdata(pdev);
+	struct s3c24xx_spi *hw = platform_get_drvdata(to_platform_device(dev));
 
 	if (hw->pdata && hw->pdata->gpio_setup)
 		hw->pdata->gpio_setup(hw->pdata, 0);

@@ -429,27 +429,31 @@ static int s3c24xx_spi_suspend(struct pl
 	return 0;
 }
 
-static int s3c24xx_spi_resume(struct platform_device *pdev)
+static int s3c24xx_spi_resume(struct device *dev)
 {
-	struct s3c24xx_spi *hw = platform_get_drvdata(pdev);
+	struct s3c24xx_spi *hw = platform_get_drvdata(to_platform_device(dev));
 
 	s3c24xx_spi_initialsetup(hw);
 	return 0;
 }
 
+static struct dev_pm_ops s3c24xx_spi_pmops = {
+	.suspend	= s3c24xx_spi_suspend,
+	.resume		= s3c24xx_spi_resume,
+};
+
+#define S3C24XX_SPI_PMOPS &s3c24xx_spi_pmops
 #else
-#define s3c24xx_spi_suspend NULL
-#define s3c24xx_spi_resume  NULL
-#endif
+#define S3C24XX_SPI_PMOPS NULL
+#endif /* CONFIG_PM */
 
 MODULE_ALIAS("platform:s3c2410-spi");
 static struct platform_driver s3c24xx_spi_driver = {
 	.remove		= __exit_p(s3c24xx_spi_remove),
-	.suspend	= s3c24xx_spi_suspend,
-	.resume		= s3c24xx_spi_resume,
 	.driver		= {
 		.name	= "s3c2410-spi",
 		.owner	= THIS_MODULE,
+		.pm	= S3C24XX_SPI_PMOPS,
 	},
 };
author
#7 lore raw

PATCH 7/7 spi_s3c24xx: cache device setup data

In reply to Ben Dooks

With the update to the spi_bitbang driver, the transfer setup code is being called more often, and thus is often re-doing calculations that have been done before. The SPI layer allows our driver to add its own data to each device so add a result cache to each device.

This should also remove the problem where we where directly setting up registers in the setup call which meant we might overwrite the state of an extant transfer.,

Signed-off-by Ben Dooks <[email protected]>
---
 drivers/spi/spi_s3c24xx.c |  126 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 89 insertions(+), 37 deletions(-)
drivers/spi/spi_s3c24xx.c +89 -37
Index: b/drivers/spi/spi_s3c24xx.c
===================================================================
@@ -28,6 +28,20 @@
 #include <plat/regs-spi.h>
 #include <mach/spi.h>
 
+/**
+ * s3c24xx_spi_devstate - per device data
+ * @hz: Last frequency calculated for @sppre field.
+ * @mode: Last mode setting for the @spcon field.
+ * @spcon: Value to write to the SPCON register.
+ * @sppre: Value to write to the SPPRE register.
+ */
+struct s3c24xx_spi_devstate {
+	unsigned int	hz;
+	unsigned int	mode;
+	u8		spcon;
+	u8		sppre;
+};
+
 struct s3c24xx_spi {
 	/* bitbang has to be first */
 	struct spi_bitbang	 bitbang;

@@ -68,43 +82,31 @@ static void s3c24xx_spi_gpiocs(struct s3
 
 static void s3c24xx_spi_chipsel(struct spi_device *spi, int value)
 {
+	struct s3c24xx_spi_devstate *cs = spi->controller_state;
 	struct s3c24xx_spi *hw = to_hw(spi);
 	unsigned int cspol = spi->mode & SPI_CS_HIGH ? 1 : 0;
-	unsigned int spcon;
+
+	/* change the chipselect state and the state of the spi engine clock */
 
 	switch (value) {
 	case BITBANG_CS_INACTIVE:
 		hw->set_cs(hw->pdata, spi->chip_select, cspol^1);
+		writeb(cs->spcon, hw->regs + S3C2410_SPCON);
 		break;
 
 	case BITBANG_CS_ACTIVE:
-		spcon = readb(hw->regs + S3C2410_SPCON);
-
-		if (spi->mode & SPI_CPHA)
-			spcon |= S3C2410_SPCON_CPHA_FMTB;
-		else
-			spcon &= ~S3C2410_SPCON_CPHA_FMTB;
-
-		if (spi->mode & SPI_CPOL)
-			spcon |= S3C2410_SPCON_CPOL_HIGH;
-		else
-			spcon &= ~S3C2410_SPCON_CPOL_HIGH;
-
-		spcon |= S3C2410_SPCON_ENSCK;
-
-		/* write new configration */
-
-		writeb(spcon, hw->regs + S3C2410_SPCON);
+		writeb(cs->spcon | S3C2410_SPCON_ENSCK,
+		       hw->regs + S3C2410_SPCON);
 		hw->set_cs(hw->pdata, spi->chip_select, cspol);
-
 		break;
 	}
 }
 
-static int s3c24xx_spi_setupxfer(struct spi_device *spi,
-				 struct spi_transfer *t)
+static int s3c24xx_spi_update_state(struct spi_device *spi,
+				    struct spi_transfer *t)
 {
 	struct s3c24xx_spi *hw = to_hw(spi);
+	struct s3c24xx_spi_devstate *cs = spi->controller_state;
 	unsigned int bpw;
 	unsigned int hz;
 	unsigned int div;

@@ -124,41 +126,89 @@ static int s3c24xx_spi_setupxfer(struct 
 		return -EINVAL;
 	}
 
-	clk = clk_get_rate(hw->clk);
-	div = DIV_ROUND_UP(clk, hz * 2) - 1;
+	if (spi->mode != cs->mode) {
+		u8 spcon = SPCON_DEFAULT;
 
-	if (div > 255)
-		div = 255;
+		if (spi->mode & SPI_CPHA)
+			spcon |= S3C2410_SPCON_CPHA_FMTB;
 
-	dev_dbg(&spi->dev, "setting pre-scaler to %d (wanted %d, got %ld)\n",
-		div, hz, clk / (2 * (div + 1)));
+		if (spi->mode & SPI_CPOL)
+			spcon |= S3C2410_SPCON_CPOL_HIGH;
 
+		cs->mode = spi->mode;
+		cs->spcon = spcon;
+	}
 
-	writeb(div, hw->regs + S3C2410_SPPRE);
+	if (cs->hz != hz) {
+		clk = clk_get_rate(hw->clk);
+		div = DIV_ROUND_UP(clk, hz * 2) - 1;
 
-	spin_lock(&hw->bitbang.lock);
-	if (!hw->bitbang.busy) {
-		hw->bitbang.chipselect(spi, BITBANG_CS_INACTIVE);
-		/* need to ndelay for 0.5 clocktick ? */
+		if (div > 255)
+			div = 255;
+
+		dev_dbg(&spi->dev, "pre-scaler=%d (wanted %d, got %ld)\n",
+			div, hz, clk / (2 * (div + 1)));
+
+		cs->hz = hz;
+		cs->sppre = div;
 	}
-	spin_unlock(&hw->bitbang.lock);
 
 	return 0;
 }
 
+static int s3c24xx_spi_setupxfer(struct spi_device *spi,
+				 struct spi_transfer *t)
+{
+	struct s3c24xx_spi_devstate *cs = spi->controller_state;
+	struct s3c24xx_spi *hw = to_hw(spi);
+	int ret;
+
+	ret = s3c24xx_spi_update_state(spi, t);
+	if (!ret)
+		writeb(cs->sppre, hw->regs + S3C2410_SPPRE);
+
+	return ret;
+}
+
 static int s3c24xx_spi_setup(struct spi_device *spi)
 {
+	struct s3c24xx_spi_devstate *cs = spi->controller_state;
+	struct s3c24xx_spi *hw = to_hw(spi);
 	int ret;
 
-	ret = s3c24xx_spi_setupxfer(spi, NULL);
-	if (ret < 0) {
-		dev_err(&spi->dev, "setupxfer returned %d\n", ret);
+	/* allocate settings on the first call */
+	if (!cs) {
+		cs = kzalloc(sizeof(struct s3c24xx_spi_devstate), GFP_KERNEL);
+		if (!cs) {
+			dev_err(&spi->dev, "no memory for controller state\n");
+			return -ENOMEM;
+		}
+
+		cs->spcon = SPCON_DEFAULT;
+		cs->hz = -1;
+		spi->controller_state = cs;
+	}
+
+	/* initialise the state from the device */
+	ret = s3c24xx_spi_update_state(spi, NULL);
+	if (ret)
 		return ret;
+
+	spin_lock(&hw->bitbang.lock);
+	if (!hw->bitbang.busy) {
+		hw->bitbang.chipselect(spi, BITBANG_CS_INACTIVE);
+		/* need to ndelay for 0.5 clocktick ? */
 	}
+	spin_unlock(&hw->bitbang.lock);
 
 	return 0;
 }
 
+static void s3c24xx_spi_cleanup(struct spi_device *spi)
+{
+	kfree(spi->controller_state);
+}
+
 static inline unsigned int hw_txbyte(struct s3c24xx_spi *hw, int count)
 {
 	return hw->tx ? hw->tx[count] : 0;

@@ -286,7 +336,9 @@ static int __init s3c24xx_spi_probe(stru
 	hw->bitbang.setup_transfer = s3c24xx_spi_setupxfer;
 	hw->bitbang.chipselect     = s3c24xx_spi_chipsel;
 	hw->bitbang.txrx_bufs      = s3c24xx_spi_txrx;
-	hw->bitbang.master->setup  = s3c24xx_spi_setup;
+
+	hw->master->setup  = s3c24xx_spi_setup;
+	hw->master->cleanup = s3c24xx_spi_cleanup;
 
 	dev_dbg(hw->dev, "bitbang at %p\n", &hw->bitbang);
#8 lore raw
In reply to Ben Dooks
Quoted 3 lines · On Thu, 13 Aug 2009 11:06:02 +0100 Ben Dooks <[email protected]> wrote:

Fix spello in the SPI documentation and fix the behaviour and bugs in the spi_s3c24xx driver.

Do you think that any of these are needed in 2.6.31, or earlier?

author
#9 lore raw
Quoted 8 lines · Andrew Morton
Quoted 3 lines · On Thu, 13 Aug 2009 11:06:02 +0100 Ben Dooks <[email protected]> wrote:

Fix spello in the SPI documentation and fix the behaviour and bugs in the spi_s3c24xx driver.

Do you think that any of these are needed in 2.6.31, or earlier?

The following would be useful to get in:
spi_s3c24xx-fix-clock-rate-calculation.patch spi_s3c24xx-fix-transfer-setup-code.patch

The rest can wait.

Signature · 3 lines
Ben Dooks, Software Engineer, Simtec Electronics

http://www.simtec.co.uk/