[PATCH V3 0/5] Add polling support for 64xx spi controller

[PATCH V3 0/5] Add polling support for 64xx spi controller

[Girish K S]

[PATCH 1/5]: fixes the error handling in the interrupt handler
[PATCH 2/5]: The existing driver support partial polling mode.
	     This patch modifies the current driver to support
	     only polling mode.
[PATCH 3/5]: provision to support SoC's with dedicated i/o pins
[PATCH 4/5]: provision to support dedicated cs pin
[PATCH 5/5]: support exynos5440 SoC in polling mode

Tested this patch on exynos5250 in dma mode, and exynos5440 in
polling mode.

changes in v3: 
	    Rebase it to the linus's master branch. After rebase
there was a logical code change due to the merge of generic dma
patch. resolved the merge conflicts and tested for the functionality
	
Girish K S (5):
  spi: s3c64xx: modified error interrupt handling and init
  spi: s3c64xx: added support for polling mode
  spi: s3c64xx: Added provision for non-gpio i/o's
  spi: s3c64xx: Added provision for dedicated cs pin
  spi: s3c64xx: Added support for exynos5440 spi

 drivers/spi/spi-s3c64xx.c                 | 204 +++++++++++++++++++++---------
 include/linux/platform_data/spi-s3c64xx.h |   3 +
 2 files changed, 148 insertions(+), 59 deletions(-)

-- 
1.8.0


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar

[PATCH V3 1/5] spi: s3c64xx: modified error interrupt handling and init

[Girish K S]

From: Girish K S <girishks2000-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

The status of the interrupt is available in the status register,
so reading the clear pending register and writing back the same
value will not actually clear the pending interrupts. This patch
modifies the interrupt handler to read the status register and
clear the corresponding pending bit in the clear pending register.

Modified the hwInit function to clear all the pending interrupts.

Signed-off-by: Girish K S <ks.giri-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/spi/spi-s3c64xx.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index e862ab8..4188b2f 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -994,25 +994,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
 {
 	struct s3c64xx_spi_driver_data *sdd = data;
 	struct spi_master *spi = sdd->master;
-	unsigned int val;
+	unsigned int val, clr = 0;
 
-	val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
+	val = readl(sdd->regs + S3C64XX_SPI_STATUS);
 
-	val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
-		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
-		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
-		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
-
-	writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
-
-	if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
+	if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
+		clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
 		dev_err(&spi->dev, "RX overrun\n");
-	if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
+	}
+	if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
+		clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
 		dev_err(&spi->dev, "RX underrun\n");
-	if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
+	}
+	if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
+		clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
 		dev_err(&spi->dev, "TX overrun\n");
-	if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
+	}
+	if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
+		clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
 		dev_err(&spi->dev, "TX underrun\n");
+	}
+
+	/* Clear the pending irq by setting and then clearing it */
+	writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
+	writel(0, sdd->regs + S3C64XX_SPI_PENDING_CLR);
 
 	return IRQ_HANDLED;
 }
@@ -1036,9 +1041,13 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
 	writel(0, regs + S3C64XX_SPI_MODE_CFG);
 	writel(0, regs + S3C64XX_SPI_PACKET_CNT);
 
-	/* Clear any irq pending bits */
-	writel(readl(regs + S3C64XX_SPI_PENDING_CLR),
-				regs + S3C64XX_SPI_PENDING_CLR);
+	/* Clear any irq pending bits, should set and clear the bits */
+	val = S3C64XX_SPI_PND_RX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
+		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
+	writel(val, regs + S3C64XX_SPI_PENDING_CLR);
+	writel(0, regs + S3C64XX_SPI_PENDING_CLR);
 
 	writel(0, regs + S3C64XX_SPI_SWAP_CFG);
 
-- 
1.8.0


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar

[PATCH V3 2/5] spi: s3c64xx: added support for polling mode

[Girish K S]

From: Girish K S <girishks2000-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

The 64xx spi driver supports partial polling mode.
Only the last chunk of the transfer length is transferred
or recieved in polling mode.

Some SoC's that adopt this controller might not have have dma
interface. This patch adds support for complete polling mode
and gives flexibity for the user to select poll/dma mode.

Signed-off-by: Girish K S <ks.giri-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/spi/spi-s3c64xx.c | 116 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 81 insertions(+), 35 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 4188b2f..054b8d4 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -35,6 +35,7 @@
 #include <linux/platform_data/spi-s3c64xx.h>
 
 #define MAX_SPI_PORTS		3
+#define S3C64XX_SPI_QUIRK_POLL		(1 << 0)
 
 /* Registers and bit-fields */
 
@@ -126,6 +127,7 @@
 #define S3C64XX_SPI_TRAILCNT		S3C64XX_SPI_MAX_TRAILCNT
 
 #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
+#define is_polling(x)	(x->port_conf->quirks & S3C64XX_SPI_QUIRK_POLL)
 
 #define RXBUSY    (1<<2)
 #define TXBUSY    (1<<3)
@@ -154,6 +156,7 @@ struct s3c64xx_spi_port_config {
 	int	fifo_lvl_mask[MAX_SPI_PORTS];
 	int	rx_lvl_offset;
 	int	tx_st_done;
+	int	quirks;
 	bool	high_speed;
 	bool	clk_from_cmu;
 };
@@ -419,6 +422,27 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
 
 	cs = spi->controller_data;
 	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
+
+	/* Start the signals */
+	writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+}
+
+static u32 wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
+					int timeout_ms)
+{
+	void __iomem *regs = sdd->regs;
+	unsigned long val;
+	u32 status;
+	/* max fifo depth available */
+	u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1;
+
+	val = msecs_to_loops(timeout_ms);
+	do {
+		status = readl(regs + S3C64XX_SPI_STATUS);
+	} while (RX_FIFO_LVL(status, sdd) < max_fifo && --val);
+
+	/* return the actual received data length */
+	return RX_FIFO_LVL(status, sdd);
 }
 
 static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
@@ -443,20 +467,19 @@ static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
 		} while (RX_FIFO_LVL(status, sdd) < xfer->len && --val);
 	}
 
-	if (!val)
-		return -EIO;
-
 	if (dma_mode) {
 		u32 status;
 
 		/*
+		 * If the previous xfer was completed within timeout, then
+		 * proceed further else return -EIO.
 		 * DmaTx returns after simply writing data in the FIFO,
 		 * w/o waiting for real transmission on the bus to finish.
 		 * DmaRx returns only after Dma read data from FIFO which
 		 * needs bus transmission to finish, so we don't worry if
 		 * Xfer involved Rx(with or without Tx).
 		 */
-		if (xfer->rx_buf == NULL) {
+		if (val && !xfer->rx_buf) {
 			val = msecs_to_loops(10);
 			status = readl(regs + S3C64XX_SPI_STATUS);
 			while ((TX_FIFO_LVL(status, sdd)
@@ -466,30 +489,53 @@ static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
 				status = readl(regs + S3C64XX_SPI_STATUS);
 			}
 
-			if (!val)
-				return -EIO;
 		}
+
+		/* If timed out while checking rx/tx status return error */
+		if (!val)
+			return -EIO;
 	} else {
+		int loops;
+		u32 cpy_len;
+		u8 *buf;
+
 		/* If it was only Tx */
-		if (xfer->rx_buf == NULL) {
+		if (!xfer->rx_buf) {
 			sdd->state &= ~TXBUSY;
 			return 0;
 		}
 
-		switch (sdd->cur_bpw) {
-		case 32:
-			ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
-				xfer->rx_buf, xfer->len / 4);
-			break;
-		case 16:
-			ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
-				xfer->rx_buf, xfer->len / 2);
-			break;
-		default:
-			ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
-				xfer->rx_buf, xfer->len);
-			break;
-		}
+		/*
+		 * If the receive length is bigger than the controller fifo
+		 * size, calculate the loops and read the fifo as many times.
+		 * loops = length / max fifo size (calculated by using the
+		 * fifo mask).
+		 * For any size less than the fifo size the below code is
+		 * executed atleast once.
+		 */
+		loops = xfer->len / ((FIFO_LVL_MASK(sdd) >> 1) + 1);
+		buf = xfer->rx_buf;
+		do{
+			/* wait for data to be received in the fifo */
+			cpy_len = wait_for_timeout(sdd, ms);
+
+			switch (sdd->cur_bpw) {
+			case 32:
+				ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
+					buf, cpy_len / 4);
+				break;
+			case 16:
+				ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
+					buf, cpy_len / 2);
+				break;
+			default:
+				ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
+					buf, cpy_len);
+				break;
+			}
+
+			buf = buf + cpy_len;
+		}while(loops--);
 		sdd->state &= ~RXBUSY;
 	}
 
@@ -505,6 +551,10 @@ static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd,
 		sdd->tgl_spi = NULL;
 
 	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
+
+	/* Quiese the signals */
+	writel(S3C64XX_SPI_SLAVE_SIG_INACT,
+	sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 }
 
 static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
@@ -586,7 +636,7 @@ static int s3c64xx_spi_map_mssg(struct s3c64xx_spi_driver_data *sdd,
 	struct device *dev = &sdd->pdev->dev;
 	struct spi_transfer *xfer;
 
-	if (msg->is_dma_mapped)
+	if (is_polling(sdd) || msg->is_dma_mapped)
 		return 0;
 
 	/* First mark all xfer unmapped */
@@ -635,7 +685,7 @@ static void s3c64xx_spi_unmap_mssg(struct s3c64xx_spi_driver_data *sdd,
 	struct device *dev = &sdd->pdev->dev;
 	struct spi_transfer *xfer;
 
-	if (msg->is_dma_mapped)
+	if (is_polling(sdd) || msg->is_dma_mapped)
 		return;
 
 	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
@@ -713,7 +763,8 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 		}
 
 		/* Polling method for xfers not bigger than FIFO capacity */
-		if (xfer->len <= ((FIFO_LVL_MASK(sdd) >> 1) + 1))
+		if (is_polling(sdd) ||
+			xfer->len <= ((FIFO_LVL_MASK(sdd) >> 1) + 1))
 			use_dma = 0;
 		else
 			use_dma = 1;
@@ -729,17 +780,10 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 		/* Slave Select */
 		enable_cs(sdd, spi);
 
-		/* Start the signals */
-		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 */
-		writel(S3C64XX_SPI_SLAVE_SIG_INACT,
-		       sdd->regs + S3C64XX_SPI_SLAVE_SEL);
-
 		if (status) {
 			dev_err(&spi->dev, "I/O Error: rx-%d tx-%d res:rx-%c tx-%c len-%d\n",
 				xfer->rx_buf ? 1 : 0, xfer->tx_buf ? 1 : 0,
@@ -795,7 +839,7 @@ static int s3c64xx_spi_prepare_transfer(struct spi_master *spi)
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(spi);
 
 	/* Acquire DMA channels */
-	while (!acquire_dma(sdd))
+	while (!is_polling(sdd) && !acquire_dma(sdd))
 		usleep_range(10000, 11000);
 
 	pm_runtime_get_sync(&sdd->pdev->dev);
@@ -808,8 +852,10 @@ static int s3c64xx_spi_unprepare_transfer(struct spi_master *spi)
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(spi);
 
 	/* Free DMA channels */
-	sdd->ops->release(sdd->rx_dma.ch, &s3c64xx_spi_dma_client);
-	sdd->ops->release(sdd->tx_dma.ch, &s3c64xx_spi_dma_client);
+	if (!is_polling(sdd)) {
+		sdd->ops->release(sdd->rx_dma.ch, &s3c64xx_spi_dma_client);
+		sdd->ops->release(sdd->tx_dma.ch, &s3c64xx_spi_dma_client);
+	}
 
 	pm_runtime_put(&sdd->pdev->dev);
 
@@ -1217,7 +1263,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 
 	sdd->cur_bpw = 8;
 
-	if (!sdd->pdev->dev.of_node) {
+	if (!sdd->pdev->dev.of_node && !is_polling(sdd)) {
 		res = platform_get_resource(pdev, IORESOURCE_DMA,  0);
 		if (!res) {
 			dev_err(&pdev->dev, "Unable to get SPI tx dma "
-- 
1.8.0


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar

[PATCH V3 3/5] spi: s3c64xx: Added provision for non-gpio i/o's

[Girish K S]

From: Girish K S <girishks2000-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Currently the drivers supports only the GPIO based i/o pins.
But there are Exynos SoC's that use the same controller with
dedicated i/o pins.

This patch  provides provision to support gpio/dedicated pins.
The decision is made by parsing the "gpios" property in the spi
node.

Signed-off-by: Girish K S <ks.giri-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/spi/spi-s3c64xx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 054b8d4..94716d1 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1112,6 +1112,9 @@ static int s3c64xx_spi_parse_dt_gpio(struct s3c64xx_spi_driver_data *sdd)
 	struct device *dev = &sdd->pdev->dev;
 	int idx, gpio, ret;
 
+	if (!of_find_property(dev->of_node, "gpios", NULL))
+		return 0;
+
 	/* find gpios for mosi, miso and clock lines */
 	for (idx = 0; idx < 3; idx++) {
 		gpio = of_get_gpio(dev->of_node, idx);
@@ -1138,6 +1141,11 @@ free_gpio:
 static void s3c64xx_spi_dt_gpio_free(struct s3c64xx_spi_driver_data *sdd)
 {
 	unsigned int idx;
+	struct device *dev = &sdd->pdev->dev;
+
+	if (!of_find_property(dev->of_node, "gpios", NULL))
+		return;
+
 	for (idx = 0; idx < 3; idx++)
 		gpio_free(sdd->gpios[idx]);
 }
-- 
1.8.0


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar

[PATCH V3 4/5] spi: s3c64xx: Added provision for dedicated cs pin

[Girish K S]

From: Girish K S <girishks2000-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

The existing driver supports gpio based /cs signal.
For controller's that have one device per controller,
the slave device's /cs signal might be internally controlled
by the chip select bit of slave select register. They are not
externally asserted/deasserted using gpio pin.

This patch adds support for controllers with dedicated /cs pin.
if "cs-gpio" property doesnt exist in a spi dts node, the controller
would treat the /cs pin as dedicated.

Signed-off-by: Girish K S <ks.giri-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/spi/spi-s3c64xx.c                 | 27 +++++++++++++++++++--------
 include/linux/platform_data/spi-s3c64xx.h |  3 +++
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 94716d1..ac557f8 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -414,14 +414,16 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
 		if (sdd->tgl_spi != spi) { /* if last mssg on diff device */
 			/* Deselect the last toggled device */
 			cs = sdd->tgl_spi->controller_data;
-			gpio_set_value(cs->line,
-				spi->mode & SPI_CS_HIGH ? 0 : 1);
+			if (cs->cs_gpio)
+				gpio_set_value(cs->line,
+					spi->mode & SPI_CS_HIGH ? 0 : 1);
 		}
 		sdd->tgl_spi = NULL;
 	}
 
 	cs = spi->controller_data;
-	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
+	if (cs->cs_gpio)
+		gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
 
 	/* Start the signals */
 	writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
@@ -550,7 +552,8 @@ static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd,
 	if (sdd->tgl_spi == spi)
 		sdd->tgl_spi = NULL;
 
-	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
+	if (cs->cs_gpio)
+		gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
 
 	/* Quiese the signals */
 	writel(S3C64XX_SPI_SLAVE_SIG_INACT,
@@ -889,7 +892,12 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 		return ERR_PTR(-ENOMEM);
 	}
 
-	cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
+	if (of_find_property(data_np, "cs-gpio", NULL)) {
+		/* The CS line is asserted/deasserted by the gpio pin */
+		cs->cs_gpio = true;
+		cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
+	}
+
 	if (!gpio_is_valid(cs->line)) {
 		dev_err(&spi->dev, "chip select gpio is not specified or invalid\n");
 		kfree(cs);
@@ -929,7 +937,8 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 		return -ENODEV;
 	}
 
-	if (!spi_get_ctldata(spi)) {
+	/* Request gpio only if cs line is asserted by gpio pins */
+	if (cs->cs_gpio) {
 		err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
 				       dev_name(&spi->dev));
 		if (err) {
@@ -938,9 +947,11 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 				cs->line, err);
 			goto err_gpio_req;
 		}
-		spi_set_ctldata(spi, cs);
 	}
 
+	if (!spi_get_ctldata(spi))
+		spi_set_ctldata(spi, cs);
+
 	sci = sdd->cntrlr_info;
 
 	spin_lock_irqsave(&sdd->lock, flags);
@@ -1028,7 +1039,7 @@ static void s3c64xx_spi_cleanup(struct spi_device *spi)
 {
 	struct s3c64xx_spi_csinfo *cs = spi_get_ctldata(spi);
 
-	if (cs) {
+	if (cs && cs->cs_gpio) {
 		gpio_free(cs->line);
 		if (spi->dev.of_node)
 			kfree(cs);
diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
index ceba18d..0343d8d 100644
--- a/include/linux/platform_data/spi-s3c64xx.h
+++ b/include/linux/platform_data/spi-s3c64xx.h
@@ -17,6 +17,8 @@ struct platform_device;
  * struct s3c64xx_spi_csinfo - ChipSelect description
  * @fb_delay: Slave specific feedback delay.
  *            Refer to FB_CLK_SEL register definition in SPI chapter.
+ * @cs_gpio: CS line status, 'true' if CS line is asserted by gpio.
+ * 	     'false' if asserted by internal dedicated pin.
  * @line: Custom 'identity' of the CS line.
  *
  * This is per SPI-Slave Chipselect information.
@@ -25,6 +27,7 @@ struct platform_device;
  */
 struct s3c64xx_spi_csinfo {
 	u8 fb_delay;
+	bool cs_gpio;
 	unsigned line;
 };
 
-- 
1.8.0


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar

[PATCH V3 5/5] spi: s3c64xx: Added support for exynos5440 spi

[Girish K S]

From: Girish K S <girishks2000-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This patch adds support for the exynos5440 spi controller.
The integration of the spi IP in exynos5440 is different from
other SoC's. The I/O pins are no more configured via gpio, they
have dedicated pins.

Signed-off-by: Girish K S <ks.giri-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/spi/spi-s3c64xx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index ac557f8..bb52a70 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1546,6 +1546,15 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
 	.clk_from_cmu	= true,
 };
 
+static struct s3c64xx_spi_port_config exynos5440_spi_port_config = {
+	.fifo_lvl_mask	= { 0x1ff },
+	.rx_lvl_offset	= 15,
+	.tx_st_done	= 25,
+	.high_speed	= true,
+	.clk_from_cmu	= true,
+	.quirks		= S3C64XX_SPI_QUIRK_POLL,
+};
+
 static struct platform_device_id s3c64xx_spi_driver_ids[] = {
 	{
 		.name		= "s3c2443-spi",
@@ -1574,6 +1583,9 @@ static const struct of_device_id s3c64xx_spi_dt_match[] = {
 	{ .compatible = "samsung,exynos4210-spi",
 			.data = (void *)&exynos4_spi_port_config,
 	},
+	{ .compatible = "samsung,exynos5440-spi",
+			.data = (void *)&exynos5440_spi_port_config,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, s3c64xx_spi_dt_match);
-- 
1.8.0


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar

Re: [PATCH V3 0/5] Add polling support for 64xx spi controller

[Girish KS]

Hello grant,

any comments on this patch series?


On Wed, Mar 13, 2013 at 12:13 PM, Girish K S <girishks2000-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> [PATCH 1/5]: fixes the error handling in the interrupt handler
> [PATCH 2/5]: The existing driver support partial polling mode.
>              This patch modifies the current driver to support
>              only polling mode.
> [PATCH 3/5]: provision to support SoC's with dedicated i/o pins
> [PATCH 4/5]: provision to support dedicated cs pin
> [PATCH 5/5]: support exynos5440 SoC in polling mode
>
> Tested this patch on exynos5250 in dma mode, and exynos5440 in
> polling mode.
>
> changes in v3:
>             Rebase it to the linus's master branch. After rebase
> there was a logical code change due to the merge of generic dma
> patch. resolved the merge conflicts and tested for the functionality
>
> Girish K S (5):
>   spi: s3c64xx: modified error interrupt handling and init
>   spi: s3c64xx: added support for polling mode
>   spi: s3c64xx: Added provision for non-gpio i/o's
>   spi: s3c64xx: Added provision for dedicated cs pin
>   spi: s3c64xx: Added support for exynos5440 spi
>
>  drivers/spi/spi-s3c64xx.c                 | 204 +++++++++++++++++++++---------
>  include/linux/platform_data/spi-s3c64xx.h |   3 +
>  2 files changed, 148 insertions(+), 59 deletions(-)
>
> --
> 1.8.0
>

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar

Re: [PATCH V3 4/5] spi: s3c64xx: Added provision for dedicated cs pin

[Mark Brown]

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

On Wed, Mar 13, 2013 at 12:13:33PM +0530, Girish K S wrote:
> From: Girish K S <girishks2000@gmail.com>
> 
> The existing driver supports gpio based /cs signal.
> For controller's that have one device per controller,
> the slave device's /cs signal might be internally controlled
> by the chip select bit of slave select register. They are not
> externally asserted/deasserted using gpio pin.

Applying this patch breaks my S3C64xx based system (Cragganmore, a
non-DT platform).  It'll break all existing non-DT platforms since...

> + * @cs_gpio: CS line status, 'true' if CS line is asserted by gpio.
> + * 	     'false' if asserted by internal dedicated pin.
>   * @line: Custom 'identity' of the CS line.
>   *
>   * This is per SPI-Slave Chipselect information.
> @@ -25,6 +27,7 @@ struct platform_device;
>   */
>  struct s3c64xx_spi_csinfo {
>  	u8 fb_delay;
> +	bool cs_gpio;
>  	unsigned line;
>  };

...you've added this new cs_gpio field to the platform data but not
updated any of the existing users (including Cragganmore).  It would
seem better to make the default behaviour stay as per the current
default and make the new behaviour optional but at a minimum all
existing in-tree users need to be updated.

It's also a bit odd that we end up checking cs_gpio and then using line
in the code, it'd be more idiomatic if cs_gpio were the GPIO number.

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

Re: [PATCH V3 1/5] spi: s3c64xx: modified error interrupt handling and init

[Mark Brown]

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

On Wed, Mar 13, 2013 at 12:13:30PM +0530, Girish K S wrote:
> From: Girish K S <girishks2000@gmail.com>
> 
> The status of the interrupt is available in the status register,
> so reading the clear pending register and writing back the same
> value will not actually clear the pending interrupts. This patch
> modifies the interrupt handler to read the status register and
> clear the corresponding pending bit in the clear pending register.

Applied, thanks.

> Signed-off-by: Girish K S <ks.giri@samsung.com>

This and the committer e-mail address (the From: above) really ought to
match up with each other.

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

Re: [PATCH V3 2/5] spi: s3c64xx: added support for polling mode

[Mark Brown]

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

On Wed, Mar 13, 2013 at 12:13:31PM +0530, Girish K S wrote:

> Some SoC's that adopt this controller might not have have dma
> interface. This patch adds support for complete polling mode
> and gives flexibity for the user to select poll/dma mode.

Ouch, that sounds like a regression.

> @@ -419,6 +422,27 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
>  
>  	cs = spi->controller_data;
>  	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
> +
> +	/* Start the signals */
> +	writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
> +}
> +

This looks odd and not obviously related to the rest of the change -
does it belong as part of some of your other commits adding support for
using the controller /CS functionality?  In general it feels like this
ought to be broken down a bit - there's some refactoring as well as the
new functionality.

> +static u32 wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
> +					int timeout_ms)
> +{
> +	void __iomem *regs = sdd->regs;
> +	unsigned long val;
> +	u32 status;
> +	/* max fifo depth available */
> +	u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1;
> +
> +	val = msecs_to_loops(timeout_ms);
> +	do {
> +		status = readl(regs + S3C64XX_SPI_STATUS);
> +	} while (RX_FIFO_LVL(status, sdd) < max_fifo && --val);
> +
> +	/* return the actual received data length */
> +	return RX_FIFO_LVL(status, sdd);

This is really wait_for_fifo_empty_with_timeout() isn't it?  It feels
like there ought to be at least a cpu_relax() in the busy wait too.

> +		/*
> +		 * If the receive length is bigger than the controller fifo
> +		 * size, calculate the loops and read the fifo as many times.
> +		 * loops = length / max fifo size (calculated by using the
> +		 * fifo mask).
> +		 * For any size less than the fifo size the below code is
> +		 * executed atleast once.
> +		 */
> +		loops = xfer->len / ((FIFO_LVL_MASK(sdd) >> 1) + 1);
> +		buf = xfer->rx_buf;
> +		do{

Coding style.

> -	if (!sdd->pdev->dev.of_node) {
> +	if (!sdd->pdev->dev.of_node && !is_polling(sdd)) {
>  		res = platform_get_resource(pdev, IORESOURCE_DMA,  0);
>  		if (!res) {
>  			dev_err(&pdev->dev, "Unable to get SPI tx dma "

It seems like it'd be sensible to also handle failure to get the DMA
resource by going into polling mode.

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

Re: [PATCH V3 2/5] spi: s3c64xx: added support for polling mode

[Girish KS]

On Mon, Apr 1, 2013 at 6:42 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Mar 13, 2013 at 12:13:31PM +0530, Girish K S wrote:
>
>> Some SoC's that adopt this controller might not have have dma
>> interface. This patch adds support for complete polling mode
>> and gives flexibity for the user to select poll/dma mode.
>
> Ouch, that sounds like a regression.
>
>> @@ -419,6 +422,27 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
>>
>>       cs = spi->controller_data;
>>       gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
>> +
>> +     /* Start the signals */
>> +     writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
>> +}
>> +
>
> This looks odd and not obviously related to the rest of the change -
> does it belong as part of some of your other commits adding support for
> using the controller /CS functionality?  In general it feels like this
> ought to be broken down a bit - there's some refactoring as well as the
> new functionality.

it is part of this patch. It is just the movement of slave active bit from
s3c64xx_spi_config to enable_cs and disable_cs function respectively.
It is not part of chip select gpio patch.

>
>> +static u32 wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
>> +                                     int timeout_ms)
>> +{
>> +     void __iomem *regs = sdd->regs;
>> +     unsigned long val;
>> +     u32 status;
>> +     /* max fifo depth available */
>> +     u32 max_fifo = (FIFO_LVL_MASK(sdd) >> 1) + 1;
>> +
>> +     val = msecs_to_loops(timeout_ms);
>> +     do {
>> +             status = readl(regs + S3C64XX_SPI_STATUS);
>> +     } while (RX_FIFO_LVL(status, sdd) < max_fifo && --val);
>> +
>> +     /* return the actual received data length */
>> +     return RX_FIFO_LVL(status, sdd);
>
> This is really wait_for_fifo_empty_with_timeout() isn't it?  It feels
> like there ought to be at least a cpu_relax() in the busy wait too.

This code existed in the older version, I just made it as a separate function.
Will check it and make necessary change.

>
>> +             /*
>> +              * If the receive length is bigger than the controller fifo
>> +              * size, calculate the loops and read the fifo as many times.
>> +              * loops = length / max fifo size (calculated by using the
>> +              * fifo mask).
>> +              * For any size less than the fifo size the below code is
>> +              * executed atleast once.
>> +              */
>> +             loops = xfer->len / ((FIFO_LVL_MASK(sdd) >> 1) + 1);
>> +             buf = xfer->rx_buf;
>> +             do{
>
> Coding style.

Will change it

>
>> -     if (!sdd->pdev->dev.of_node) {
>> +     if (!sdd->pdev->dev.of_node && !is_polling(sdd)) {
>>               res = platform_get_resource(pdev, IORESOURCE_DMA,  0);
>>               if (!res) {
>>                       dev_err(&pdev->dev, "Unable to get SPI tx dma "
>
> It seems like it'd be sensible to also handle failure to get the DMA
> resource by going into polling mode.

There are 2 cases currently i have identified and handled,
1. The SoC's dont have DMA support for spi controller. For such SoC's we
 would not add the dma resource in the spi dts node. In this case the probe
 would return error if failure for DMA resuorce is handled.

2. The SoC has a DMA support for SPI controller, but due to some x
reason(H/W bug),
 the driver would force polling mode by enabling
S3C64XX_SPI_QUIRK_POLL in driver
 data. For such SoC's there would be a dma entry in the spi controller
dts node, and
 probe can handle failure for DMA resource successfully.

To handle above both situations successfully if
(!sdd->pdev->dev.of_node && !is_polling(sdd)) is used.

Re: [PATCH V3 2/5] spi: s3c64xx: added support for polling mode

[Mark Brown]

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

On Wed, Apr 03, 2013 at 05:00:04PM +0530, Girish KS wrote:
> On Mon, Apr 1, 2013 at 6:42 PM, Mark Brown

> >> -     if (!sdd->pdev->dev.of_node) {
> >> +     if (!sdd->pdev->dev.of_node && !is_polling(sdd)) {
> >>               res = platform_get_resource(pdev, IORESOURCE_DMA,  0);
> >>               if (!res) {
> >>                       dev_err(&pdev->dev, "Unable to get SPI tx dma "

> > It seems like it'd be sensible to also handle failure to get the DMA
> > resource by going into polling mode.

> There are 2 cases currently i have identified and handled,
> 1. The SoC's dont have DMA support for spi controller. For such SoC's we
>  would not add the dma resource in the spi dts node. In this case the probe
>  would return error if failure for DMA resuorce is handled.

That's not what the code currently does...

> 2. The SoC has a DMA support for SPI controller, but due to some x
> reason(H/W bug),
>  the driver would force polling mode by enabling
> S3C64XX_SPI_QUIRK_POLL in driver
>  data. For such SoC's there would be a dma entry in the spi controller
> dts node, and
>  probe can handle failure for DMA resource successfully.

> To handle above both situations successfully if
> (!sdd->pdev->dev.of_node && !is_polling(sdd)) is used.

Right, that's what the code currently does but what I'm suggesting is
that this isn't the most helpful thing to do and that printing a big
warning then soldiering on in polling mode might be more useful.

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

Re: [PATCH V3 2/5] spi: s3c64xx: added support for polling mode

[Girish KS]

On Wed, Apr 3, 2013 at 5:19 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, Apr 03, 2013 at 05:00:04PM +0530, Girish KS wrote:
>> On Mon, Apr 1, 2013 at 6:42 PM, Mark Brown
>
>> >> -     if (!sdd->pdev->dev.of_node) {
>> >> +     if (!sdd->pdev->dev.of_node && !is_polling(sdd)) {
>> >>               res = platform_get_resource(pdev, IORESOURCE_DMA,  0);
>> >>               if (!res) {
>> >>                       dev_err(&pdev->dev, "Unable to get SPI tx dma "
>
>> > It seems like it'd be sensible to also handle failure to get the DMA
>> > resource by going into polling mode.
>
>> There are 2 cases currently i have identified and handled,
>> 1. The SoC's dont have DMA support for spi controller. For such SoC's we
>>  would not add the dma resource in the spi dts node. In this case the probe
>>  would return error if failure for DMA resuorce is handled.
>
> That's not what the code currently does...
>
>> 2. The SoC has a DMA support for SPI controller, but due to some x
>> reason(H/W bug),
>>  the driver would force polling mode by enabling
>> S3C64XX_SPI_QUIRK_POLL in driver
>>  data. For such SoC's there would be a dma entry in the spi controller
>> dts node, and
>>  probe can handle failure for DMA resource successfully.
>
>> To handle above both situations successfully if
>> (!sdd->pdev->dev.of_node && !is_polling(sdd)) is used.
>
> Right, that's what the code currently does but what I'm suggesting is
> that this isn't the most helpful thing to do and that printing a big
> warning then soldiering on in polling mode might be more useful.

Ok I got it. will do it.

Thanks Mark

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

Re: [PATCH V3 4/5] spi: s3c64xx: Added provision for dedicated cs pin

[Girish KS]

On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Mar 13, 2013 at 12:13:33PM +0530, Girish K S wrote:
>> From: Girish K S <girishks2000@gmail.com>
>>
>> The existing driver supports gpio based /cs signal.
>> For controller's that have one device per controller,
>> the slave device's /cs signal might be internally controlled
>> by the chip select bit of slave select register. They are not
>> externally asserted/deasserted using gpio pin.
>
> Applying this patch breaks my S3C64xx based system (Cragganmore, a
> non-DT platform).  It'll break all existing non-DT platforms since...

sure will rebase and take care of the non-dt case

>
>> + * @cs_gpio: CS line status, 'true' if CS line is asserted by gpio.
>> + *        'false' if asserted by internal dedicated pin.
>>   * @line: Custom 'identity' of the CS line.
>>   *
>>   * This is per SPI-Slave Chipselect information.
>> @@ -25,6 +27,7 @@ struct platform_device;
>>   */
>>  struct s3c64xx_spi_csinfo {
>>       u8 fb_delay;
>> +     bool cs_gpio;
>>       unsigned line;
>>  };
>
> ...you've added this new cs_gpio field to the platform data but not
> updated any of the existing users (including Cragganmore).  It would
> seem better to make the default behaviour stay as per the current
> default and make the new behaviour optional but at a minimum all
> existing in-tree users need to be updated.
>
> It's also a bit odd that we end up checking cs_gpio and then using line
> in the code, it'd be more idiomatic if cs_gpio were the GPIO number.

In the original driver it was assumed that the cs line is always a gpio pin.
But the current controller that i am working on has no gpio pin for cs
selection.
All the lines to the device are internally connected. There is no
option to select
the cs signal. So cs-gpio property parsing has to skipped for this
controller, that means
cs_gpio cannot be a GPIO number. If it has to be a number then it has
to be < 0 to say
it is not gpio. Any >= 0 number implies it is a valid gpio (in reality
for this controller it is not.)

So i introduced a new member cs_gpio. I will checkout the possibility
to avoid this member.

Re: [PATCH V3 4/5] spi: s3c64xx: Added provision for dedicated cs pin

[Mark Brown]

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

On Mon, Apr 08, 2013 at 03:21:03PM +0530, Girish KS wrote:
> On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown

> > It's also a bit odd that we end up checking cs_gpio and then using line
> > in the code, it'd be more idiomatic if cs_gpio were the GPIO number.

> In the original driver it was assumed that the cs line is always a gpio pin.
> But the current controller that i am working on has no gpio pin for cs
> selection.
> All the lines to the device are internally connected. There is no
> option to select
> the cs signal. So cs-gpio property parsing has to skipped for this
> controller, that means
> cs_gpio cannot be a GPIO number. If it has to be a number then it has
> to be < 0 to say
> it is not gpio. Any >= 0 number implies it is a valid gpio (in reality
> for this controller it is not.)

Two options here, one is to just assume nobody will use GPIO 0 and the
other is to set the number appopriately during probe so that only probe
needs to worry about the issue.

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

Re: [PATCH V3 4/5] spi: s3c64xx: Added provision for dedicated cs pin

[Girish KS]

On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Apr 08, 2013 at 03:21:03PM +0530, Girish KS wrote:
>> On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown
>
>> > It's also a bit odd that we end up checking cs_gpio and then using line
>> > in the code, it'd be more idiomatic if cs_gpio were the GPIO number.
>
>> In the original driver it was assumed that the cs line is always a gpio pin.
>> But the current controller that i am working on has no gpio pin for cs
>> selection.
>> All the lines to the device are internally connected. There is no
>> option to select
>> the cs signal. So cs-gpio property parsing has to skipped for this
>> controller, that means
>> cs_gpio cannot be a GPIO number. If it has to be a number then it has
>> to be < 0 to say
>> it is not gpio. Any >= 0 number implies it is a valid gpio (in reality
>> for this controller it is not.)
>
> Two options here, one is to just assume nobody will use GPIO 0 and the
> other is to set the number appopriately during probe so that only probe
> needs to worry about the issue.

regarding the first option, may be others also should agree to it (in
case if somebody is
using the gpio 0).
In the second option if the gpio number is set in the probe, then we
are overwriting the
actual gpio number assigned in the platform data.
If I move the cs_gpio from the platform data to controller private
data then the dependency
on other platforms can be removed, but still the check (true or false)
before setting the gpio
line will remain. If agreed upon this i can go ahead and post the patch

Re: [PATCH V3 4/5] spi: s3c64xx: Added provision for dedicated cs pin

[Girish KS]

On Mon, Apr 8, 2013 at 5:15 PM, Girish KS <girishks2000@gmail.com> wrote:
> On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Mon, Apr 08, 2013 at 03:21:03PM +0530, Girish KS wrote:
>>> On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown
>>
>>> > It's also a bit odd that we end up checking cs_gpio and then using line
>>> > in the code, it'd be more idiomatic if cs_gpio were the GPIO number.
>>
>>> In the original driver it was assumed that the cs line is always a gpio pin.
>>> But the current controller that i am working on has no gpio pin for cs
>>> selection.
>>> All the lines to the device are internally connected. There is no
>>> option to select
>>> the cs signal. So cs-gpio property parsing has to skipped for this
>>> controller, that means
>>> cs_gpio cannot be a GPIO number. If it has to be a number then it has
>>> to be < 0 to say
>>> it is not gpio. Any >= 0 number implies it is a valid gpio (in reality
>>> for this controller it is not.)
>>
>> Two options here, one is to just assume nobody will use GPIO 0 and the
>> other is to set the number appopriately during probe so that only probe
>> needs to worry about the issue.
>
> regarding the first option, may be others also should agree to it (in
> case if somebody is
> using the gpio 0).
> In the second option if the gpio number is set in the probe, then we
> are overwriting the
> actual gpio number assigned in the platform data.
> If I move the cs_gpio from the platform data to controller private
> data then the dependency
> on other platforms can be removed, but still the check (true or false)
> before setting the gpio
> line will remain. If agreed upon this i can go ahead and post the patch

Sorry for the allignment. there is some issue with my interface

Re: [PATCH V3 4/5] spi: s3c64xx: Added provision for dedicated cs pin

[Mark Brown]

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

On Mon, Apr 08, 2013 at 05:15:26PM +0530, Girish KS wrote:
> On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown

> > Two options here, one is to just assume nobody will use GPIO 0 and the
> > other is to set the number appopriately during probe so that only probe
> > needs to worry about the issue.
> 
> In the second option if the gpio number is set in the probe, then we
> are overwriting the
> actual gpio number assigned in the platform data.
> If I move the cs_gpio from the platform data to controller private
> data then the dependency
> on other platforms can be removed, but still the check (true or false)
> before setting the gpio
> line will remain. If agreed upon this i can go ahead and post the patch

I think logic in probe should be fine, it just felt really cumbersome
having this on every single use but doing it once on probe ought to be
OK.

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

Re: [PATCH V3 4/5] spi: s3c64xx: Added provision for dedicated cs pin

[Girish KS]

On Mon, Apr 8, 2013 at 5:50 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Apr 08, 2013 at 05:15:26PM +0530, Girish KS wrote:
>> On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown
>
>> > Two options here, one is to just assume nobody will use GPIO 0 and the
>> > other is to set the number appopriately during probe so that only probe
>> > needs to worry about the issue.
>>
>> In the second option if the gpio number is set in the probe, then we
>> are overwriting the
>> actual gpio number assigned in the platform data.
>> If I move the cs_gpio from the platform data to controller private
>> data then the dependency
>> on other platforms can be removed, but still the check (true or false)
>> before setting the gpio
>> line will remain. If agreed upon this i can go ahead and post the patch
>
> I think logic in probe should be fine, it just felt really cumbersome
> having this on every single use but doing it once on probe ought to be
> OK.

I will move the populating cs_gpio logic to probe. But the enable_cs
and disable_cs will have the
check before calling the gpio_set_value api (because it takes cs-line
as a parameter),

Re: [PATCH V3 4/5] spi: s3c64xx: Added provision for dedicated cs pin

[Mark Brown]

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

On Mon, Apr 08, 2013 at 07:19:05PM +0530, Girish KS wrote:

> I will move the populating cs_gpio logic to probe. But the enable_cs
> and disable_cs will have the
> check before calling the gpio_set_value api (because it takes cs-line
> as a parameter),

Yes, you'll still need to have a check.

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