Skip to contentSkip to conversation

spi: spi-s3c64xx: Remove unused driver data structure tgl_spi field

PATCH New Sylwester Nawrocki 4 participants 21 messages Apr 16, 2018 – Apr 17, 2018 1 message not in the archive

Subsystems (3)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): CGME20180416154055epcas2p4c08bb2254e79368a29eb481cddc59855@epcas2p4.samsung.com

#1 lore raw

PATCH 1/6 spi: spi-s3c64xx: Remove unused driver data structure tgl_spi field

The tgl_spi pointer is now unused so remove it.

---
 drivers/spi/spi-s3c64xx.c | 2 --
 1 file changed, 2 deletions(-)
drivers/spi/spi-s3c64xx.c +0 -2
index baa3a9fa2638..f7ca7ed33e1e 100644
@@ -156,7 +156,6 @@ struct s3c64xx_spi_port_config {
  * @ioclk: Pointer to the i/o clock between master and slave
  * @master: Pointer to the SPI Protocol master.
  * @cntrlr_info: Platform specific data for the controller this driver manages.
- * @tgl_spi: Pointer to the last CS left untoggled by the cs_change hint.
  * @lock: Controller specific lock.
  * @state: Set of FLAGS to indicate status.
  * @rx_dmach: Controller's DMA channel for Rx.
@@ -177,7 +176,6 @@ struct s3c64xx_spi_driver_data {
 	struct platform_device          *pdev;
 	struct spi_master               *master;
 	struct s3c64xx_spi_info  *cntrlr_info;
-	struct spi_device               *tgl_spi;
 	spinlock_t                      lock;
 	unsigned long                   sfr_start;
 	struct completion               xfer_completion;
Signature · 1 line
2.14.2
#2 lore raw

PATCH 2/6 spi: spi-s3c64xx: Remove unused s3c64xx_spi_hwinit() function argument

The channel argument is not used and anyway it could be retrieved from the passed driver data structure.

---
 drivers/spi/spi-s3c64xx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/spi/spi-s3c64xx.c +3 -3
index f7ca7ed33e1e..b7333a883442 100644
@@ -889,7 +889,7 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
+static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
 {
 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
 	void __iomem *regs = sdd->regs;
@@ -1143,7 +1143,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	pm_runtime_get_sync(&pdev->dev);
 
 	/* Setup Deufult Mode */
-	s3c64xx_spi_hwinit(sdd, sdd->port_id);
+	s3c64xx_spi_hwinit(sdd);
 
 	spin_lock_init(&sdd->lock);
 	init_completion(&sdd->xfer_completion);
@@ -1258,7 +1258,7 @@ static int s3c64xx_spi_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	s3c64xx_spi_hwinit(sdd, sdd->port_id);
+	s3c64xx_spi_hwinit(sdd);
 
 	return spi_master_resume(master);
 }
Signature · 1 line
2.14.2
#4 lore raw

PATCH 2/6 spi: spi-s3c64xx: Remove unused s3c64xx_spi_hwinit() function argument

In reply to Sylwester Nawrocki
Quoted 8 lines · Sylwester Nawrocki

The channel argument is not used and anyway it could be retrieved from the passed driver data structure.

---
 drivers/spi/spi-s3c64xx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Best regards,
Krzysztof

#5 lore raw

PATCH 3/6 spi: spi-s3c64xx: Drop unused enable_datapath() function argument

In reply to Sylwester Nawrocki

The spi pointer argument is not used now so remove it. While at it s3c64xx_ prefix is added to the function name.

---
 drivers/spi/spi-s3c64xx.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/spi/spi-s3c64xx.c +3 -4
index b7333a883442..e90e60bc1ecc 100644
@@ -348,9 +348,8 @@ static bool s3c64xx_spi_can_dma(struct spi_master *master,
 	return xfer->len > (FIFO_LVL_MASK(sdd) >> 1) + 1;
 }
 
-static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
-				struct spi_device *spi,
-				struct spi_transfer *xfer, int dma_mode)
+static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
+				    struct spi_transfer *xfer, int dma_mode)
 {
 	void __iomem *regs = sdd->regs;
 	u32 modecfg, chcfg;
@@ -665,7 +664,7 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 	sdd->state &= ~RXBUSY;
 	sdd->state &= ~TXBUSY;
 
-	enable_datapath(sdd, spi, xfer, use_dma);
+	s3c64xx_enable_datapath(sdd, xfer, use_dma);
 
 	/* Start the signals */
 	s3c64xx_spi_set_cs(spi, true);

--
2.14.2

#6 lore raw

PATCH 3/6 spi: spi-s3c64xx: Drop unused enable_datapath() function argument

Hi Sylwester,

Quoted 2 lines · Sylwester Nawrocki

The spi pointer argument is not used now so remove it. While at it s3c64xx_ prefix is added to the function name.

These for me are two different patches, but in any case it's trivial so that I don't mind it too much.

Quoted 1 line

Thanks,
Andi

#7 lore raw

PATCH 3/6 spi: spi-s3c64xx: Drop unused enable_datapath() function argument

Hi Andi,

Quoted 6 lines · andi at etezian.org
Quoted 2 lines · Sylwester Nawrocki

The spi pointer argument is not used now so remove it. While at it s3c64xx_ prefix is added to the function name.

These for me are two different patches, but in any case it's trivial so that I don't mind it too much.

I am going to drop renaming from this patch. There are 2 more functions (wait_for_pio(), wait_for_dma()) without prefix, should we rename them too or rather leave all 3 as they are now?

Signature · 2 lines
Regards,
Sylwester
#8 lore raw

PATCH 3/6 spi: spi-s3c64xx: Drop unused enable_datapath() function argument

Quoted 7 lines · Sylwester Nawrocki

Hi Andi,

Quoted 4 lines · andi at etezian.org
Quoted 2 lines · Sylwester Nawrocki

The spi pointer argument is not used now so remove it. While at it s3c64xx_ prefix is added to the function name.

Quoted 6 lines
Quoted 2 lines

These for me are two different patches, but in any case it's trivial so that I don't mind it too much.

I am going to drop renaming from this patch. There are 2 more functions (wait_for_pio(), wait_for_dma()) without prefix, should we rename them too or rather leave all 3 as they are now?

Indeed it is kind of mixed up - some purely local functions (not even passed through pointer to some ops) have prefix, some do not. If you ask me, I would prefer to have the prefix on all of them because:
1. If they appear in call trace (where backtrace pointers are not telling anything) it is easy to understand their location,
2. having generic name like "wait_for_dma" sounds for me like generic, Linux-wide macro (but it is not).

However I do not insist on that.

Best regards,
Krzysztof

#9 lore raw

PATCH 3/6 spi: spi-s3c64xx: Drop unused enable_datapath() function argument

In reply to Sylwester Nawrocki
Quoted 7 lines · Sylwester Nawrocki

The spi pointer argument is not used now so remove it. While at it s3c64xx_ prefix is added to the function name.

---
 drivers/spi/spi-s3c64xx.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Best regards,
Krzysztof

#10 lore raw

PATCH 4/6 spi: spi-s3c64xx: Do not ignore timeout errors in polling I/O mode

In reply to Sylwester Nawrocki

Currently timeout errors in polling I/O mode transfer are silently ignored. Fix it by returning an error when we time out waiting on the RX FIFO level to reach the transfer length.

---
 drivers/spi/spi-s3c64xx.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/spi/spi-s3c64xx.c +2 -0
index e90e60bc1ecc..d87fa7d9fad3 100644
@@ -502,6 +502,8 @@ static int wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 		status = readl(regs + S3C64XX_SPI_STATUS);
 	} while (RX_FIFO_LVL(status, sdd) < xfer->len && --val);
 
+	if (!val)
+		return -EIO;
 
 	/* If it was only Tx */
 	if (!xfer->rx_buf) {
Signature · 1 line
2.14.2
#11 lore raw

PATCH 4/6 spi: spi-s3c64xx: Do not ignore timeout errors in polling I/O mode

Hi Sylwester,

Quoted 5 lines · Sylwester Nawrocki

Currently timeout errors in polling I/O mode transfer are silently ignored.
Fix it by returning an error when we time out waiting on the RX FIFO level
to reach the transfer length.

yes, makes sense.

Quoted 1 line

Thanks,
Andi

#12 lore raw

PATCH 4/6 spi: spi-s3c64xx: Do not ignore timeout errors in polling I/O mode

In reply to Sylwester Nawrocki
Quoted 9 lines · Sylwester Nawrocki

Currently timeout errors in polling I/O mode transfer are silently ignored. Fix it by returning an error when we time out waiting on the RX FIFO level to reach the transfer length.

---
 drivers/spi/spi-s3c64xx.c | 2 ++
 1 file changed, 2 insertions(+)

Best regards,
Krzysztof

maintainer
#13 lore raw

PATCH 4/6 spi: spi-s3c64xx: Do not ignore timeout errors in polling I/O mode

In reply to Sylwester Nawrocki
Quoted 3 lines · Sylwester Nawrocki

Currently timeout errors in polling I/O mode transfer are silently ignored. Fix it by returning an error when we time out waiting on the RX FIFO level to reach the transfer length.

This would probably have been better sent as a fix before the cosmetic
updates.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180417/f8e2d91c/attachment.sig>

#14 lore raw

PATCH 5/6 spi: spi-s3c64xx: Fix indentation in the register offset definitions

In reply to Sylwester Nawrocki

Change indentation so register address offset and register bit definitions are aligned to same column.

---
 drivers/spi/spi-s3c64xx.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/spi/spi-s3c64xx.c +8 -8
index d87fa7d9fad3..2723fa6bed8b 100644
@@ -28,15 +28,15 @@
 
 #define S3C64XX_SPI_CH_CFG		0x00
 #define S3C64XX_SPI_CLK_CFG		0x04
-#define S3C64XX_SPI_MODE_CFG	0x08
-#define S3C64XX_SPI_SLAVE_SEL	0x0C
+#define S3C64XX_SPI_MODE_CFG		0x08
+#define S3C64XX_SPI_SLAVE_SEL		0x0C
 #define S3C64XX_SPI_INT_EN		0x10
 #define S3C64XX_SPI_STATUS		0x14
 #define S3C64XX_SPI_TX_DATA		0x18
 #define S3C64XX_SPI_RX_DATA		0x1C
-#define S3C64XX_SPI_PACKET_CNT	0x20
-#define S3C64XX_SPI_PENDING_CLR	0x24
-#define S3C64XX_SPI_SWAP_CFG	0x28
+#define S3C64XX_SPI_PACKET_CNT		0x20
+#define S3C64XX_SPI_PENDING_CLR		0x24
+#define S3C64XX_SPI_SWAP_CFG		0x28
 #define S3C64XX_SPI_FB_CLK		0x2C
 
 #define S3C64XX_SPI_CH_HS_EN		(1<<6)	/* High Speed Enable */
@@ -77,9 +77,9 @@
 #define S3C64XX_SPI_INT_TX_FIFORDY_EN		(1<<0)
 
 #define S3C64XX_SPI_ST_RX_OVERRUN_ERR		(1<<5)
-#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR	(1<<4)
+#define S3C64XX_SPI_ST_RX_UNDERRUN_ERR		(1<<4)
 #define S3C64XX_SPI_ST_TX_OVERRUN_ERR		(1<<3)
-#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR	(1<<2)
+#define S3C64XX_SPI_ST_TX_UNDERRUN_ERR		(1<<2)
 #define S3C64XX_SPI_ST_RX_FIFORDY		(1<<1)
 #define S3C64XX_SPI_ST_TX_FIFORDY		(1<<0)
 
@@ -100,7 +100,7 @@
 #define S3C64XX_SPI_SWAP_TX_BIT			(1<<1)
 #define S3C64XX_SPI_SWAP_TX_EN			(1<<0)
 
-#define S3C64XX_SPI_FBCLK_MSK		(3<<0)
+#define S3C64XX_SPI_FBCLK_MSK			(3<<0)
 
 #define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id])
 #define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \

--
2.14.2

#16 lore raw

PATCH 5/6 spi: spi-s3c64xx: Fix indentation in the register offset definitions

In reply to Sylwester Nawrocki
Quoted 7 lines · Sylwester Nawrocki

Change indentation so register address offset and register bit definitions are aligned to same column.

---
 drivers/spi/spi-s3c64xx.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Best regards,
Krzysztof

#17 lore raw

PATCH 6/6 spi: spi-s3c64xx: Allow higher transfer lengths in polling IO mode

In reply to Sylwester Nawrocki

Some variants of the SPI controller have no DMA support, in such case SPI transfers longer than the FIFO length are not currently properly handled by the driver. Fix it by doing multiple transfers in the s3c64xx_spi_transfer_one() function if the SPI transfer length exceeds the FIFO size.

---
 drivers/spi/spi-s3c64xx.c | 108 ++++++++++++++++++++++++++++++----------------
 1 file changed, 71 insertions(+), 37 deletions(-)
drivers/spi/spi-s3c64xx.c +71 -37
index 2723fa6bed8b..b2e8ed354dac 100644
@@ -349,7 +349,7 @@ static bool s3c64xx_spi_can_dma(struct spi_master *master,
 }
 
 static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
-				    struct spi_transfer *xfer, int dma_mode)
+				    struct spi_transfer *xfer, bool dma_mode)
 {
 	void __iomem *regs = sdd->regs;
 	u32 modecfg, chcfg;
@@ -634,11 +634,15 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 				    struct spi_transfer *xfer)
 {
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
+	const unsigned int fifo_len = (FIFO_LVL_MASK(sdd) >> 1) + 1;
+	const void *tx_buf = NULL;
+	void *rx_buf = NULL;
+	int target_len = 0, origin_len = 0;
+	bool use_dma = false;
 	int status;
 	u32 speed;
 	u8 bpw;
 	unsigned long flags;
-	int use_dma;
 
 	reinit_completion(&sdd->xfer_completion);
 
@@ -653,48 +657,78 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 		s3c64xx_spi_config(sdd);
 	}
 
-	/* Polling method for xfers not bigger than FIFO capacity */
-	use_dma = 0;
-	if (!is_polling(sdd) &&
-	    (sdd->rx_dma.ch && sdd->tx_dma.ch &&
-	     (xfer->len > ((FIFO_LVL_MASK(sdd) >> 1) + 1))))
-		use_dma = 1;
+	if (!is_polling(sdd) && (xfer->len > fifo_len) &&
+	    sdd->rx_dma.ch && sdd->tx_dma.ch) {
+		use_dma = true;
 
-	spin_lock_irqsave(&sdd->lock, flags);
+	} else if (is_polling(sdd) && xfer->len > fifo_len) {
+		tx_buf = xfer->tx_buf;
+		rx_buf = xfer->rx_buf;
+		origin_len = xfer->len;
 
-	/* Pending only which is to be done */
-	sdd->state &= ~RXBUSY;
-	sdd->state &= ~TXBUSY;
+		target_len = xfer->len;
+		if (xfer->len > fifo_len)
+			xfer->len = fifo_len;
+	}
+
+	do {
+		spin_lock_irqsave(&sdd->lock, flags);
 
-	s3c64xx_enable_datapath(sdd, xfer, use_dma);
+		/* Pending only which is to be done */
+		sdd->state &= ~RXBUSY;
+		sdd->state &= ~TXBUSY;
 
-	/* Start the signals */
-	s3c64xx_spi_set_cs(spi, true);
+		s3c64xx_enable_datapath(sdd, xfer, use_dma);
 
-	spin_unlock_irqrestore(&sdd->lock, flags);
+		/* Start the signals */
+		s3c64xx_spi_set_cs(spi, true);
 
-	if (use_dma)
-		status = wait_for_dma(sdd, xfer);
-	else
-		status = wait_for_pio(sdd, xfer);
-
-	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,
-			(sdd->state & RXBUSY) ? 'f' : 'p',
-			(sdd->state & TXBUSY) ? 'f' : 'p',
-			xfer->len);
-
-		if (use_dma) {
-			if (xfer->tx_buf != NULL
-			    && (sdd->state & TXBUSY))
-				dmaengine_terminate_all(sdd->tx_dma.ch);
-			if (xfer->rx_buf != NULL
-			    && (sdd->state & RXBUSY))
-				dmaengine_terminate_all(sdd->rx_dma.ch);
+		spin_unlock_irqrestore(&sdd->lock, flags);
+
+		if (use_dma)
+			status = wait_for_dma(sdd, xfer);
+		else
+			status = wait_for_pio(sdd, xfer);
+
+		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,
+				(sdd->state & RXBUSY) ? 'f' : 'p',
+				(sdd->state & TXBUSY) ? 'f' : 'p',
+				xfer->len);
+
+			if (use_dma) {
+				if (xfer->tx_buf && (sdd->state & TXBUSY))
+					dmaengine_terminate_all(sdd->tx_dma.ch);
+				if (xfer->rx_buf && (sdd->state & RXBUSY))
+					dmaengine_terminate_all(sdd->rx_dma.ch);
+			}
+		} else {
+			flush_fifo(sdd);
 		}
-	} else {
-		flush_fifo(sdd);
+
+		if (target_len > 0) {
+			target_len -= xfer->len;
+
+			if (xfer->tx_buf)
+				xfer->tx_buf += xfer->len;
+
+			if (xfer->rx_buf)
+				xfer->rx_buf += xfer->len;
+
+			if (target_len > fifo_len)
+				xfer->len = fifo_len;
+			else
+				xfer->len = target_len;
+		}
+	} while (target_len > 0);
+
+	if (origin_len) {
+		/* Restore original xfer buffers and length */
+		xfer->tx_buf = tx_buf;
+		xfer->rx_buf = rx_buf;
+		xfer->len = origin_len;
 	}
 
 	return status;
Signature · 1 line
2.14.2
#18 lore raw

PATCH 6/6 spi: spi-s3c64xx: Allow higher transfer lengths in polling IO mode

Hi Sylwester,

Quoted 5 lines · Sylwester Nawrocki

Some variants of the SPI controller have no DMA support, in such case SPI transfers longer than the FIFO length are not currently properly handled by the driver. Fix it by doing multiple transfers in the s3c64xx_spi_transfer_one() function if the SPI transfer length exceeds the FIFO size.

this patch is a mix of cosmetics and what you are actually describing in the commit log...

Quoted 5 lines
drivers/spi/spi-s3c64xx.c +0 -0
@@ -634,11 +634,15 @@ static int s3c64xx_spi_transfer_one(struct
 spi_master *master,
 				    struct spi_transfer *xfer)
 {
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);

drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_transfer_one(struct · line 642

Quoted 10 lines
drivers/spi/spi-s3c64xx.c +5 -1
+	const unsigned int fifo_len = (FIFO_LVL_MASK(sdd) >> 1) + 1;
+	const void *tx_buf = NULL;
+	void *rx_buf = NULL;
+	int target_len = 0, origin_len = 0;
+	bool use_dma = false;
 	int status;
 	u32 speed;
 	u8 bpw;
 	unsigned long flags;
-	int use_dma;

... for example 'use_dma' bool instead of int or the 'fifo_len'. I agree with these changes and, even though they are trivial, I would prefer having them in separate patches.

Thanks,
Andi

#19 lore raw

PATCH 6/6 spi: spi-s3c64xx: Allow higher transfer lengths in polling IO mode

Hi Andi,

Quoted 21 lines · andi at etezian.org
Quoted 5 lines · Sylwester Nawrocki

Some variants of the SPI controller have no DMA support, in such case SPI transfers longer than the FIFO length are not currently properly handled by the driver. Fix it by doing multiple transfers in the s3c64xx_spi_transfer_one() function if the SPI transfer length exceeds the FIFO size.

this patch is a mix of cosmetics and what you are actually describing in the commit log...

Quoted 11 lines
drivers/spi/spi-s3c64xx.c +5 -0
@@ -634,11 +634,15 @@ static int s3c64xx_spi_transfer_one(struct
 spi_master *master,
 				    struct spi_transfer *xfer)
 {
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
+	const unsigned int fifo_len = (FIFO_LVL_MASK(sdd) >> 1) + 1;
+	const void *tx_buf = NULL;
+	void *rx_buf = NULL;
+	int target_len = 0, origin_len = 0;
+	bool use_dma = false;
 	int status;
Quoted 8 lines

drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_transfer_one(struct spi_master *master, · line 641

Quoted 4 lines
drivers/spi/spi-s3c64xx.c +0 -1
 	u32 speed;
 	u8 bpw;
 	unsigned long flags;
-	int use_dma;

... for example 'use_dma' bool instead of int or the 'fifo_len'. I agree with these changes and, even though they are trivial, I would prefer having them in separate patches.

Thanks for your review! I will split that into 3 patches then, however it seems a bit dubious to have the fifo_len change on its own.

Signature · 2 lines
Regards,
Sylwester