[PATCH 0/2] Add SPI control driver for Sunplus SP7021 SoC

[PATCH 0/2] Add SPI control driver for Sunplus SP7021 SoC

[LH.Kuo]

This is a patch series for SPI driver for Sunplus SP7021 SoC.

Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and
etc.) into a single chip. It is designed for industrial control.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

LH.Kuo (2):
  SPI: Add SPI driver for Sunplus SP7021
  devicetree bindings SPI Add bindings doc for Sunplus SP7021

 .../bindings/spi/spi-sunplus-sp7021.yaml           |   95 ++
 MAINTAINERS                                        |    7 +
 drivers/spi/Kconfig                                |   11 +
 drivers/spi/Makefile                               |    1 +
 drivers/spi/spi-sunplus-sp7021.c                   | 1356 ++++++++++++++++++++
 5 files changed, 1470 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
 create mode 100644 drivers/spi/spi-sunplus-sp7021.c

-- 
2.7.4

[PATCH 1/2] SPI: Add SPI driver for Sunplus SP7021

[LH.Kuo]

Add SPI driver for Sunplus SP7021.

Signed-off-by: LH.Kuo <lh.kuo@sunplus.com>
---
 MAINTAINERS                      |    6 +
 drivers/spi/Kconfig              |   11 +
 drivers/spi/Makefile             |    1 +
 drivers/spi/spi-sunplus-sp7021.c | 1356 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 1374 insertions(+)
 create mode 100644 drivers/spi/spi-sunplus-sp7021.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3b79fd4..f00c466 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17945,6 +17945,12 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS SPI CONTROLLER INTERFACE DRIVER
+M:	LH Kuo <lh.kuo@sunplus.com>
+L:	sdricohcs-devel@lists.sourceforge.net (subscribers-only)
+S:	Maintained
+F:	drivers/spi/spi-sunplus-sp7021.c
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 83e352b..24a39c8 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -844,6 +844,17 @@ config SPI_SUN6I
 	help
 	  This enables using the SPI controller on the Allwinner A31 SoCs.
 
+config SPI_SUNPLUS_SP7021
+	tristate "Sunplus SP7021 SPI controller"
+	depends on SOC_SP7021
+	help
+	  This enable Sunplus SP7021 spi controller driver on the SP7021 SoCs.
+	  This driver can also be built as a module. If so, the module will be
+	  called as spi-sunplus-sp7021.
+
+	  If you have a  Sunplus SP7021 platform say Y here.
+	  If unsure, say N.
+
 config SPI_SYNQUACER
 	tristate "Socionext's SynQuacer HighSpeed SPI controller"
 	depends on ARCH_SYNQUACER || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 699db95..04428e7 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -117,6 +117,7 @@ obj-$(CONFIG_SPI_STM32_QSPI) 		+= spi-stm32-qspi.o
 obj-$(CONFIG_SPI_ST_SSC4)		+= spi-st-ssc4.o
 obj-$(CONFIG_SPI_SUN4I)			+= spi-sun4i.o
 obj-$(CONFIG_SPI_SUN6I)			+= spi-sun6i.o
+obj-$(CONFIG_SPI_SUNPLUS_SP7021)	+= spi-sunplus-sp7021.o
 obj-$(CONFIG_SPI_SYNQUACER)		+= spi-synquacer.o
 obj-$(CONFIG_SPI_TEGRA210_QUAD)		+= spi-tegra210-quad.o
 obj-$(CONFIG_SPI_TEGRA114)		+= spi-tegra114.o
diff --git a/drivers/spi/spi-sunplus-sp7021.c b/drivers/spi/spi-sunplus-sp7021.c
new file mode 100644
index 0000000..632d4bf
--- /dev/null
+++ b/drivers/spi/spi-sunplus-sp7021.c
@@ -0,0 +1,1356 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Sunplus SPI controller driver
+ *
+ * Author: Sunplus Technology Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+//#define DEBUG
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/delay.h>
+//#include <dt-bindings/pinctrl/sp7021.h>
+
+#define SLAVE_INT_IN
+
+//#define PM_RUNTIME_SPI
+
+/* ---------------------------------------------------------------------------------------------- */
+
+//#define SPI_FUNC_DEBUG
+//#define SPI_DBG_INFO
+#define SPI_DBG_ERR
+
+#ifdef SPI_FUNC_DEBUG
+#define FUNC_DBG(fmt, args ...) pr_info("[SPI] dbg (%d): "  fmt "\n", __LINE__, ## args)
+#else
+#define FUNC_DBG(fmt, args ...)
+#endif
+
+#ifdef SPI_DBG_INFO
+#define DBG_INF(fmt, args ...) pr_info("[SPI] inf (%d): "  fmt "\n", __LINE__, ## args)
+#else
+#define DBG_INF(fmt, args ...)
+#endif
+
+#ifdef SPI_DBG_ERR
+#define DBG_ERR(fmt, args ...) pr_info("[SPI] err (%d): "  fmt "\n", __LINE__, ## args)
+#else
+#define DBG_ERR(fmt, args ...)
+#endif
+/* ---------------------------------------------------------------------------------------------- */
+
+#define SPI_FULL_DUPLEX
+
+#define MAS_REG_NAME "spi_master"
+#define SLA_REG_NAME "spi_slave"
+
+#define DMA_IRQ_NAME "dma_w_intr"
+#define MAS_IRQ_NAME "mas_risc_intr"
+
+#define SLA_IRQ_NAME "slave_risc_intr"
+
+#define SPI_TRANS_DATA_CNT (4)
+#define SPI_TRANS_DATA_SIZE (255)
+#define SPI_MSG_DATA_SIZE (SPI_TRANS_DATA_SIZE * SPI_TRANS_DATA_CNT)
+
+
+#define CLEAR_MASTER_INT (1<<6)
+#define MASTER_INT (1<<7)
+
+#define DMA_READ (0xd)
+#define SLA_SW_RST (1<<1)
+
+
+#define DMA_WRITE (0x4d)
+#define SPI_START (1<<0)
+#define SPI_BUSY (1<<0)
+#define CLEAR_DMA_W_INT (1<<7)
+#define DMA_W_INT (1<<8)
+#define CLEAR_ADDR_BIT (~(0x180))
+#define ADDR_BIT(x) (x<<7)
+#define DMA_DATA_RDY (1<<0)
+#define PENTAGRAM_SPI_SLAVE_SET (0x2c)
+
+
+
+#define TOTAL_LENGTH(x) (x<<24)
+#define TX_LENGTH(x) (x<<16)
+#define GET_LEN(x)     ((x>>24)&0xFF)
+#define GET_TX_LEN(x)  ((x>>16)&0xFF)
+#define GET_RX_CNT(x)  ((x>>12)&0x0F)
+#define GET_TX_CNT(x)  ((x>>8)&0x0F)
+
+
+
+#define FINISH_FLAG (1<<6)
+#define FINISH_FLAG_MASK (1<<15)
+#define RX_FULL_FLAG (1<<5)
+#define RX_FULL_FLAG_MASK (1<<14)
+#define RX_EMP_FLAG (1<<4)
+#define RX_EMP_FLAG_MASK (1<<13)
+#define TX_FULL_FLAG (1<<3)
+#define TX_FULL_FLAG_MASK (1<<12)
+#define TX_EMP_FLAG (1<<2)
+#define TX_EMP_FLAG_MASK (1<<11)
+#define SPI_START_FD (1<<0)
+#define FD_SEL (1<<6)
+#define LSB_SEL (1<<4)
+#define WRITE_BYTE(x) (x<<9)
+#define READ_BYTE(x) (x<<7)
+#define CLK_DIVIDER(x) (x<<16)
+#define INIT_SPI_MODE (~0x7F)
+#define CLEAN_RW_BYTE (~0x780)
+#define CLEAN_FLUG_MASK (~0xF800)
+
+
+#define DELAY_ENABLE (1<<3)
+#define CPOL_FD (1<<0)
+#define CPHA_R (1<<1)
+#define CPHA_W (1<<2)
+#define CS_POR (1<<5)
+
+#define SPI_FD_BUSY (1<<7)
+#define SPI_FD_INTR (1<<7)
+
+#define FD_SW_RST (1<<1)
+
+#define DEG_CORE_SPI_LATCH0 (0xB<<8)
+#define DEG_CORE_SPI_LATCH1 (0xC<<8)
+
+#define LSB_SEL_MST (1<<1)
+
+#define SPI_DEG_SEL(x) (x<<0)
+#define DEG_SPI_MST_MASK (0xFF<<2)
+#define DEG_SPI_MST(x) (x>>2)
+
+#define FIFO_DATA_BITS (16*8)    // 16 BYTES
+
+
+#define INT_BYPASS (1<<3)
+
+
+
+/* slave*/
+#define CLEAR_SLAVE_INT (1<<8)
+#define SLAVE_DATA_RDY (1<<0)
+
+
+
+enum SPI_MODE {
+	SPI_MASTER_READ = 0,
+	SPI_MASTER_WRITE = 1,
+	SPI_MASTER_RW = 2,
+	SPI_SLAVE_READ = 3,
+	SPI_SLAVE_WRITE = 4,
+	SPI_IDLE = 5
+};
+
+
+struct SPI_MAS {
+	// Group 091 : SPI_MASTER
+	unsigned int MST_TX_DATA_ADDR                      ; // 00  (ADDR : 0x9C00_2D80)
+	unsigned int MST_TX_DATA_3_2_1_0                   ; // 01  (ADDR : 0x9C00_2D84)
+	unsigned int MST_TX_DATA_7_6_5_4                   ; // 02  (ADDR : 0x9C00_2D88)
+	unsigned int MST_TX_DATA_11_10_9_8                 ; // 03  (ADDR : 0x9C00_2D8C)
+	unsigned int MST_TX_DATA_15_14_13_12               ; // 04  (ADDR : 0x9C00_2D90)
+	unsigned int G091_RESERVED_0[4]                    ; //     (ADDR : 0x9C00_2D94-2DA0)
+	unsigned int MST_RX_DATA_3_2_1_0                   ; // 09  (ADDR : 0x9C00_2DA4)
+	unsigned int MST_RX_DATA_7_6_5_4                   ; // 10  (ADDR : 0x9C00_2DA8)
+	unsigned int MST_RX_DATA_11_10_9_8                 ; // 11  (ADDR : 0x9C00_2DAC)
+	unsigned int MST_RX_DATA_15_14_13_12               ; // 12  (ADDR : 0x9C00_2DB0)
+	unsigned int FIFO_DATA                             ; // 13  (ADDR : 0x9C00_2DB4)
+	unsigned int SPI_FD_STATUS                         ; // 14  (ADDR : 0x9C00_2DB8)
+	unsigned int SPI_FD_CONFIG                         ; // 15  (ADDR : 0x9C00_2DBC)
+	unsigned int G091_RESERVED_1                       ; // 16  (ADDR : 0x9C00_2DC0)
+	unsigned int SPI_CTRL_CLKSEL                       ; // 17  (ADDR : 0x9C00_2DC4)
+	unsigned int BYTE_NO                               ; // 18  (ADDR : 0x9C00_2DC8)
+	unsigned int SPI_INT_BUSY                          ; // 19  (ADDR : 0x9C00_2DCC)
+	unsigned int DMA_CTRL                              ; // 20  (ADDR : 0x9C00_2DD0)
+	unsigned int DMA_LENGTH                            ; // 21  (ADDR : 0x9C00_2DD4)
+	unsigned int DMA_ADDR                              ; // 22  (ADDR : 0x9C00_2DD8)
+	unsigned int G091_RESERVED_2[1]                    ; // 23  (ADDR : 0x9C00_2DDC)
+	unsigned int DMA_ADDR_STAT                         ; // 24  (ADDR : 0x9C00_2DE0)
+	unsigned int G091_RESERVED_3[1]                    ; // 25  (ADDR : 0x9C00_2DE4)
+	unsigned int UART_DMA_CTRL                         ; // 26  (ADDR : 0x9C00_2DE8)
+	unsigned int G091_RESERVED_4[1]                    ; // 27  (ADDR : 0x9C00_2DEC)
+	unsigned int SPI_MST_DEBUG_SEL                     ; // 28  (ADDR : 0x9C00_2DF0)
+	unsigned int SPI_COM_DEBUG_SEL                     ; // 29  (ADDR : 0x9C00_2DF4)
+	unsigned int SPI_EXTRA_CYCLE                       ; // 30  (ADDR : 0x9C00_2DF8)
+	unsigned int MST_DMA_DATA_RDY                      ; // 31  (ADDR : 0x9C00_2DFC)
+};
+
+
+struct SPI_SLA {
+	// Group 092 : SPI_SLAVE
+	unsigned int SLV_TX_DATA_2_1_0                     ; // 00  (ADDR : 0x9C00_2E00)
+	unsigned int SLV_TX_DATA_6_5_4_3                   ; // 01  (ADDR : 0x9C00_2E04)
+	unsigned int SLV_TX_DATA_10_9_8_7                  ; // 02  (ADDR : 0x9C00_2E08)
+	unsigned int SLV_TX_DATA_14_13_12_11               ; // 03  (ADDR : 0x9C00_2E0C)
+	unsigned int SLV_TX_DATA_15                        ; // 04  (ADDR : 0x9C00_2E10)
+	unsigned int G092_RESERVED_0[4]                    ; //     (ADDR : 0x9C00_2E14-2E20)
+	unsigned int SLV_RX_DATA_3_2_1_0                   ; // 09  (ADDR : 0x9C00_2E24)
+	unsigned int SLV_RX_DATA_7_6_5_4                   ; // 10  (ADDR : 0x9C00_2E28)
+	unsigned int SLV_RX_DATA_11_10_9_8                 ; // 11  (ADDR : 0x9C00_2E2C)
+	unsigned int SLV_RX_DATA_15_14_13_12               ; // 12  (ADDR : 0x9C00_2E30)
+	unsigned int G092_RESERVED_1[4]                    ; //     (ADDR : 0x9C00_2E34-2E40)
+	unsigned int RISC_INT_DATA_RDY                     ; // 17  (ADDR : 0x9C00_2E44)
+	unsigned int SLV_DMA_CTRL                          ; // 18  (ADDR : 0x9C00_2E48)
+	unsigned int SLV_DMA_LENGTH                        ; // 19  (ADDR : 0x9C00_2E4C)
+	unsigned int SLV_DMA_INI_ADDR                      ; // 20  (ADDR : 0x9C00_2E50)
+	unsigned int G092_RESERVED_2[2]                    ; //     (ADDR : 0x9C00_2E54-2E58)
+	unsigned int ADDR_SPI_BUSY                         ; // 23  (ADDR : 0x9C00_2E5C)
+	unsigned int G092_RESERVED_3[8]                    ; //     (ADDR : 0x9C00_2E60-2E7C)
+};
+
+
+
+enum {
+	SPI_MASTER,
+	SPI_SLAVE,
+};
+
+
+struct pentagram_spi_master {
+
+	struct device *dev;
+
+	int mode;
+
+	struct spi_master *master;
+	struct spi_controller *ctlr;
+
+	void __iomem *mas_base;
+
+	void __iomem *sla_base;
+
+	u32 message_config;
+
+	int dma_irq;
+	int mas_irq;
+	int sla_irq;
+
+	struct clk *spi_clk;
+	struct reset_control *rstc;
+
+	spinlock_t lock;
+	struct mutex buf_lock;
+	unsigned int spi_max_frequency;
+	dma_addr_t tx_dma_phy_base;
+	dma_addr_t rx_dma_phy_base;
+	void *tx_dma_vir_base;
+	void *rx_dma_vir_base;
+	struct completion isr_done;	// complete() at *master_(dma|mas)_irq()
+	struct completion sla_isr;	// completion() at spi_S_irq() - slave irq jandler
+	unsigned int bufsiz;
+
+	unsigned int  rx_cur_len;
+	unsigned int  tx_cur_len;
+
+	u8 tx_data_buf[SPI_TRANS_DATA_SIZE];
+	u8 rx_data_buf[SPI_TRANS_DATA_SIZE];
+
+	int isr_flag;
+
+	unsigned int  data_unit;
+};
+
+
+static unsigned int bufsiz = 4096;
+
+static void pentagram_set_cs(struct spi_device *_s, bool _on)
+{
+	if (_s->mode & SPI_NO_CS)
+		return;
+	if (!(_s->cs_gpiod))
+		return;
+	dev_dbg(&(_s->dev), "%d gpiod:%d", _on, desc_to_gpio(_s->cs_gpiod));
+	if (_s->mode & SPI_CS_HIGH)
+		_on = !_on;
+	gpiod_set_value_cansleep(_s->cs_gpiod, _on);
+}
+
+// spi slave irq handler
+static irqreturn_t pentagram_spi_S_irq(int _irq, void *_dev)
+{
+	unsigned long flags;
+	struct pentagram_spi_master *pspim = (struct pentagram_spi_master *)_dev;
+	struct SPI_SLA *sr = (struct SPI_SLA *)pspim->sla_base;
+
+	FUNC_DBG();
+	spin_lock_irqsave(&pspim->lock, flags);
+	writel(readl(&sr->RISC_INT_DATA_RDY) | CLEAR_SLAVE_INT, &sr->RISC_INT_DATA_RDY);
+	spin_unlock_irqrestore(&pspim->lock, flags);
+	complete(&pspim->sla_isr);
+	return IRQ_HANDLED;
+}
+
+// slave DMA rw (unused now)
+// FIXME: probably frop this?
+int pentagram_spi_slave_dma_rw(struct spi_device *spi, u8 *buf, unsigned int len, int RW_phase)
+{
+	struct pentagram_spi_master *pspim = spi_controller_get_devdata(spi->controller);
+
+	struct SPI_SLA *spis_reg = (struct SPI_SLA *)(pspim->sla_base);
+	struct SPI_MAS *spim_reg = (struct SPI_MAS *)(pspim->mas_base);
+	struct device dev = spi->dev;
+	unsigned long timeout = msecs_to_jiffies(2000);
+
+	FUNC_DBG();
+	mutex_lock(&pspim->buf_lock);
+
+	if (RW_phase == SPI_SLAVE_WRITE) {
+		memcpy(pspim->tx_dma_vir_base, buf, len);
+		writel_relaxed(DMA_WRITE, &spis_reg->SLV_DMA_CTRL);
+		writel_relaxed(len, &spis_reg->SLV_DMA_LENGTH);
+		writel_relaxed(pspim->tx_dma_phy_base, &spis_reg->SLV_DMA_INI_ADDR);
+		writel(readl(&spis_reg->RISC_INT_DATA_RDY) | SLAVE_DATA_RDY,
+					&spis_reg->RISC_INT_DATA_RDY);
+		//regs1->SLV_DMA_CTRL = 0x4d;
+		//regs1->SLV_DMA_LENGTH = 0x50;
+		//regs1->SLV_DMA_INI_ADDR = 0x300;
+		//regs1->RISC_INT_DATA_RDY |= 0x1;
+	} else if (RW_phase == SPI_SLAVE_READ) {
+		reinit_completion(&pspim->isr_done);
+		writel(DMA_READ, &spis_reg->SLV_DMA_CTRL);
+		writel(len, &spis_reg->SLV_DMA_LENGTH);
+		writel(pspim->rx_dma_phy_base, &spis_reg->SLV_DMA_INI_ADDR);
+
+		if (!wait_for_completion_timeout(&pspim->isr_done, timeout)) {
+			dev_err(&dev, "wait_for_completion_timeout\n");
+			goto exit_spi_slave_rw;
+		}
+		while ((readl(&spim_reg->DMA_CTRL) & DMA_W_INT) == DMA_W_INT) {
+			dev_dbg(&dev, "spim_reg->DMA_CTRL 0x%x\n", readl(&spim_reg->DMA_CTRL));
+		};
+		memcpy(buf, pspim->rx_dma_vir_base, len);
+		writel(SLA_SW_RST, &spis_reg->SLV_DMA_CTRL);
+		/* read*/
+		//regs1->SLV_DMA_CTRL = 0xd;
+		//regs1->SLV_DMA_LENGTH = 0x50;
+		//regs1->SLV_DMA_INI_ADDR = 0x400;
+	}
+
+exit_spi_slave_rw:
+	mutex_unlock(&pspim->buf_lock);
+	return 0;
+}
+
+// slave only. usually called on driver remove
+static int pentagram_spi_S_abort(struct spi_controller *_c)
+{
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(_c);
+
+	complete(&pspim->sla_isr);
+	complete(&pspim->isr_done);
+	return 0;
+}
+
+// slave R/W, called from S_transfer_one() only
+int pentagram_spi_S_rw(struct spi_device *_s, struct spi_transfer *_t, int RW_phase)
+{
+	struct pentagram_spi_master *pspim = spi_controller_get_devdata(_s->controller);
+	struct SPI_SLA *spis_reg = (struct SPI_SLA *)(pspim->sla_base);
+	struct device *devp = &(_s->dev);
+
+	FUNC_DBG();
+	mutex_lock(&pspim->buf_lock);
+
+	if (RW_phase == SPI_SLAVE_WRITE) {
+		DBG_INF("S_WRITE len %d", _t->len);
+		reinit_completion(&pspim->sla_isr);
+
+		if (_t->tx_dma == pspim->tx_dma_phy_base)
+			memcpy(pspim->tx_dma_vir_base, _t->tx_buf, _t->len);
+
+		writel_relaxed(DMA_WRITE, &spis_reg->SLV_DMA_CTRL);
+		writel_relaxed(_t->len, &spis_reg->SLV_DMA_LENGTH);
+		writel_relaxed(_t->tx_dma, &spis_reg->SLV_DMA_INI_ADDR);
+		writel(readl(&spis_reg->RISC_INT_DATA_RDY) | SLAVE_DATA_RDY,
+				&spis_reg->RISC_INT_DATA_RDY);
+
+		if (wait_for_completion_interruptible(&pspim->sla_isr))
+			dev_err(devp, "%s() wait_for_completion timeout\n", __func__);
+
+	} else if (RW_phase == SPI_SLAVE_READ) {
+		DBG_INF("S_READ len %d", _t->len);
+		reinit_completion(&pspim->isr_done);
+		writel(DMA_READ, &spis_reg->SLV_DMA_CTRL);
+		writel(_t->len, &spis_reg->SLV_DMA_LENGTH);
+		writel(_t->rx_dma, &spis_reg->SLV_DMA_INI_ADDR);
+
+		// wait for DMA to complete
+		if (wait_for_completion_interruptible(&pspim->isr_done)) {
+			dev_err(devp, "%s() wait_for_completion timeout\n", __func__);
+			goto exit_spi_slave_rw;
+		}
+		// FIXME: is "len" correct there?
+		if (_t->tx_dma == pspim->tx_dma_phy_base)
+			memcpy(_t->rx_buf, pspim->rx_dma_vir_base, _t->len);
+
+		writel(SLA_SW_RST, &spis_reg->SLV_DMA_CTRL);
+	}
+
+exit_spi_slave_rw:
+	mutex_unlock(&pspim->buf_lock);
+	return 0;
+}
+
+// spi master irq handler
+static irqreturn_t pentagram_spi_M_irq_dma(int _irq, void *_dev)
+{
+	unsigned long flags;
+	struct pentagram_spi_master *pspim = (struct pentagram_spi_master *)_dev;
+	struct SPI_MAS *sr = (struct SPI_MAS *)pspim->mas_base;
+
+	FUNC_DBG();
+	spin_lock_irqsave(&pspim->lock, flags);
+	writel(readl(&sr->DMA_CTRL) | CLEAR_DMA_W_INT, &sr->DMA_CTRL);
+	spin_unlock_irqrestore(&pspim->lock, flags);
+	complete(&pspim->isr_done);
+	return IRQ_HANDLED;
+}
+
+void sp7021spi_rb(struct pentagram_spi_master *_m, u8 _len)
+{
+	struct SPI_MAS *sr = (struct SPI_MAS *)_m->mas_base;
+	int i;
+
+	for (i = 0; i < _len; i++) {
+		_m->rx_data_buf[_m->rx_cur_len] = readl(&sr->FIFO_DATA);
+		DBG_INF("RX 0x%x _cur_len = %d", _m->rx_data_buf[_m->rx_cur_len], _m->rx_cur_len);
+		_m->rx_cur_len++;
+	}
+}
+void sp7021spi_wb(struct pentagram_spi_master *_m, u8 _len)
+{
+	struct SPI_MAS *sr = (struct SPI_MAS *)_m->mas_base;
+	int i;
+
+	for (i = 0; i < _len; i++) {
+		DBG_INF("TX 0x%02x _cur_len %d", _m->tx_data_buf[_m->tx_cur_len], _m->tx_cur_len);
+		writel(_m->tx_data_buf[_m->tx_cur_len], &sr->FIFO_DATA);
+		_m->tx_cur_len++;
+	}
+}
+
+static irqreturn_t pentagram_spi_M_irq(int _irq, void *_dev)
+{
+	unsigned long flags;
+	struct pentagram_spi_master *pspim = (struct pentagram_spi_master *)_dev;
+	struct SPI_MAS *sr = (struct SPI_MAS *)pspim->mas_base;
+	u32 fd_status = 0;
+	unsigned int tx_len, rx_cnt, tx_cnt;
+	bool isrdone = false;
+
+	FUNC_DBG();
+
+	spin_lock_irqsave(&pspim->lock, flags);
+
+	fd_status = readl(&sr->SPI_FD_STATUS);
+	tx_cnt = GET_TX_CNT(fd_status);
+	tx_len = GET_TX_LEN(fd_status);
+
+	if ((fd_status & TX_EMP_FLAG) && (fd_status & RX_EMP_FLAG) && (GET_LEN(fd_status) == 0))
+		goto fin_irq;
+
+	if (fd_status & FINISH_FLAG)
+		DBG_INF("FINISH_FLAG");
+	if (fd_status & TX_EMP_FLAG)
+		DBG_INF("TX_EMP_FLAG");
+	if (fd_status & RX_FULL_FLAG)
+		DBG_INF("RX_FULL_FLAG");
+	rx_cnt = GET_RX_CNT(fd_status);
+	// RX_FULL_FLAG means RX buffer is full (16 bytes)
+	if (fd_status & RX_FULL_FLAG)
+		rx_cnt = pspim->data_unit;
+
+	tx_cnt = min(tx_len - pspim->tx_cur_len, pspim->data_unit - tx_cnt);
+
+	DBG_INF("fd_st=0x%x rx_c:%d tx_c:%d tx_l:%d", fd_status, rx_cnt, tx_cnt, tx_len);
+
+	if (rx_cnt > 0)
+		sp7021spi_rb(pspim, rx_cnt);
+	if (tx_cnt > 0)
+		sp7021spi_wb(pspim, tx_cnt);
+
+	fd_status = readl(&sr->SPI_FD_STATUS);
+
+	if ((fd_status & FINISH_FLAG) || (GET_TX_LEN(fd_status) == pspim->tx_cur_len)) {
+
+		while (GET_LEN(fd_status) != pspim->rx_cur_len) {
+			fd_status = readl(&sr->SPI_FD_STATUS);
+			if (fd_status & RX_FULL_FLAG)
+				rx_cnt = pspim->data_unit;
+			else
+				rx_cnt = GET_RX_CNT(fd_status);
+
+			if (rx_cnt > 0)
+				sp7021spi_rb(pspim, rx_cnt);
+		}
+		writel(readl(&sr->SPI_INT_BUSY) | CLEAR_MASTER_INT, &sr->SPI_INT_BUSY);
+		isrdone = true;
+	}
+fin_irq:
+	if (isrdone)
+		complete(&pspim->isr_done);
+	spin_unlock_irqrestore(&pspim->lock, flags);
+	DBG_INF("handled irq %d", isrdone);
+	return IRQ_HANDLED;
+}
+
+// called in *controller_transfer_one*
+static void spspi_prep_transfer(struct spi_controller *_c, struct spi_device *_s)
+{
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(_c);
+
+	pspim->tx_cur_len = 0;
+	pspim->rx_cur_len = 0;
+	pspim->data_unit = FIFO_DATA_BITS / _s->bits_per_word;
+	pspim->isr_flag = SPI_IDLE;
+	DBG_INF("pspim->data_unit %d unit", pspim->data_unit);
+}
+
+static void spspi_delay_ns(u32 _ns)
+{
+
+	if (!_ns)
+		return;
+	if (_ns <= 1000)
+		ndelay(_ns);
+	else {
+		u32 us = DIV_ROUND_UP(_ns, 1000);
+
+		if (us <= 10)
+			udelay(us);
+		else
+			usleep_range(us, us + DIV_ROUND_UP(us, 10));
+	}
+}
+
+// called from *transfer* functions, set clock there
+static void pentagram_spi_setup_transfer(struct spi_device *_s,
+					struct spi_controller *_c, struct spi_transfer *_t)
+{
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(_c);
+	struct SPI_MAS *spim_reg = (struct SPI_MAS *)pspim->mas_base;
+	u32 rc = 0, rs = 0;
+	unsigned int clk_rate;
+	unsigned int div;
+	unsigned int clk_sel;
+
+	FUNC_DBG();
+
+	dev_dbg(&(_s->dev), "setup %dHz", _s->max_speed_hz);
+	dev_dbg(&(_s->dev), "tx %p, rx %p, len %d", _t->tx_buf, _t->rx_buf, _t->len);
+	// set clock
+	clk_rate = clk_get_rate(pspim->spi_clk);
+	if (_t->speed_hz <= _s->max_speed_hz)
+		div = clk_rate / _t->speed_hz;
+	else if (_s->max_speed_hz <= _c->max_speed_hz)
+		div = clk_rate / _s->max_speed_hz;
+	else if (_c->max_speed_hz < pspim->spi_max_frequency)
+		div = clk_rate / _c->max_speed_hz;
+	else
+		div = clk_rate / pspim->spi_max_frequency;
+
+	dev_dbg(&(_s->dev), "clk_rate: %d div %d", clk_rate, div);
+
+	clk_sel = (div / 2) - 1;
+	// set full duplex (bit 6) and fd freq (bits 31:16)
+	rc = FD_SEL | (0xffff << 16);
+	rs = FD_SEL | ((clk_sel & 0xffff) << 16);
+	writel((pspim->message_config & ~rc) | rs, &(spim_reg->SPI_FD_CONFIG));
+}
+
+
+static int pentagram_spi_master_combine_write_read(struct spi_controller *_c,
+			struct spi_transfer *first, unsigned int transfers_cnt)
+{
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(_c);
+	struct SPI_MAS *sr = (struct SPI_MAS *)pspim->mas_base;
+	unsigned int data_len = 0;
+	u32 reg_temp = 0;
+	unsigned long timeout = msecs_to_jiffies(200);
+	unsigned int i, len_temp;
+	int ret;
+	struct spi_transfer *t = first;
+	bool xfer_rx = false;
+
+	FUNC_DBG();
+
+	memset(&pspim->tx_data_buf[0], 0, SPI_TRANS_DATA_SIZE);
+	dev_dbg(&(_c->dev), "txrx: tx %p, rx %p, len %d\n", t->tx_buf, t->rx_buf, t->len);
+
+	mutex_lock(&pspim->buf_lock);
+	reinit_completion(&pspim->isr_done);
+
+	for (i = 0; i < transfers_cnt; i++) {
+		if (t->tx_buf)
+			memcpy(&pspim->tx_data_buf[data_len], t->tx_buf, t->len);
+		if (t->rx_buf)
+			xfer_rx = true;
+		data_len += t->len;
+		t = list_entry(t->transfer_list.next, struct spi_transfer, transfer_list);
+	}
+	dev_dbg(&(_c->dev), "data_len %d xfer_rx %d", data_len, xfer_rx);
+
+	// set SPI FIFO data for full duplex (SPI_FD fifo_data)  91.13
+	if (pspim->tx_cur_len < data_len) {
+		len_temp = min(pspim->data_unit, data_len);
+		sp7021spi_wb(pspim, len_temp);
+	}
+	// initial SPI master config and change to Full-Duplex mode (SPI_FD_CONFIG)  91.15
+	reg_temp = readl(&sr->SPI_FD_CONFIG);
+	reg_temp &= CLEAN_RW_BYTE;
+	reg_temp &= CLEAN_FLUG_MASK;
+	reg_temp |= FD_SEL;
+	// set SPI master config for full duplex (SPI_FD_CONFIG)  91.15
+	reg_temp |= FINISH_FLAG_MASK | TX_EMP_FLAG_MASK | RX_FULL_FLAG_MASK;
+	reg_temp |= WRITE_BYTE(0) | READ_BYTE(0);  // set read write byte from fifo
+	writel(reg_temp, &sr->SPI_FD_CONFIG);
+	DBG_INF("SPI_FD_CONFIG =0x%x", readl(&sr->SPI_FD_CONFIG));
+	// set SPI STATUS and start SPI for full duplex (SPI_FD_STATUS)  91.13
+	writel(TOTAL_LENGTH(data_len) | TX_LENGTH(data_len) | SPI_START_FD, &sr->SPI_FD_STATUS);
+	writel(readl(&sr->SPI_INT_BUSY) | INT_BYPASS, &sr->SPI_INT_BUSY);
+
+	if (!wait_for_completion_timeout(&pspim->isr_done, timeout)) {
+		DBG_INF("wait_for_completion timeout");
+		ret = 1;
+		goto free_master_combite_rw;
+	}
+
+	if (xfer_rx == false) {
+		ret = 0;
+		goto free_master_combite_rw;
+	}
+
+	data_len = 0;
+	t = first;
+	for (i = 0; i < transfers_cnt; i++) {
+		if (t->rx_buf)
+			memcpy(t->rx_buf, &pspim->rx_data_buf[data_len], t->len);
+
+		data_len += t->len;
+		t = list_entry(t->transfer_list.next, struct spi_transfer, transfer_list);
+	}
+	ret = 0;
+free_master_combite_rw:
+	// reset SPI
+	writel(readl(&sr->SPI_FD_CONFIG) & CLEAN_FLUG_MASK, &sr->SPI_FD_CONFIG);
+
+	if (pspim->message_config & CPOL_FD)
+		writel(pspim->message_config, &sr->SPI_FD_CONFIG);
+
+	mutex_unlock(&pspim->buf_lock);
+	return ret;
+}
+
+
+static int pentagram_spi_master_transfer(struct spi_controller *_c, struct spi_device *_s,
+	struct spi_transfer *xfer)
+{
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(_c);
+	struct SPI_MAS *sr = (struct SPI_MAS *)pspim->mas_base;
+	u32 reg_temp = 0;
+	unsigned long timeout = msecs_to_jiffies(200);
+	unsigned int i;
+	int ret;
+	long cret;
+	unsigned int xfer_cnt, xfer_len, last_len;
+	struct spi_transfer *t = xfer;
+
+	FUNC_DBG();
+
+	xfer_cnt = t->len / SPI_TRANS_DATA_SIZE;
+	last_len = t->len % SPI_TRANS_DATA_SIZE;
+
+	memset(&pspim->tx_data_buf[0], 0, SPI_TRANS_DATA_SIZE);
+
+	dev_dbg(&(_s->dev), "txrx: tx %p, rx %p, len %d\n", t->tx_buf, t->rx_buf, t->len);
+
+	t->tx_buf+SPI_TRANS_DATA_SIZE, &(t->tx_buf[SPI_TRANS_DATA_SIZE]));
+
+	for (i = 0; i <= xfer_cnt; i++) {
+
+		mutex_lock(&pspim->buf_lock);
+
+		spspi_prep_transfer(_c, _s);
+		pentagram_spi_setup_transfer(_s, _c, xfer);
+
+		reinit_completion(&pspim->isr_done);
+
+		if (i == xfer_cnt)
+			xfer_len = last_len;
+		else
+			xfer_len = SPI_TRANS_DATA_SIZE;
+
+		if (t->tx_buf)
+			memcpy(pspim->tx_data_buf, t->tx_buf+i*SPI_TRANS_DATA_SIZE, xfer_len);
+
+		// set SPI FIFO data for full duplex (SPI_FD fifo_data)  91.13
+		if (pspim->tx_cur_len < xfer_len) {
+			reg_temp = min(pspim->data_unit, xfer_len);
+			sp7021spi_wb(pspim, reg_temp);
+		}
+
+		// initial SPI master config and change to Full-Duplex mode (SPI_FD_CONFIG)  91.15
+		reg_temp = readl(&sr->SPI_FD_CONFIG);
+		reg_temp &= CLEAN_RW_BYTE;
+		reg_temp &= CLEAN_FLUG_MASK;
+		reg_temp |= FD_SEL;
+		// set SPI master config for full duplex (SPI_FD_CONFIG)  91.15
+		reg_temp |= FINISH_FLAG_MASK | TX_EMP_FLAG_MASK | RX_FULL_FLAG_MASK;
+		reg_temp |= WRITE_BYTE(0) | READ_BYTE(0);  // set read write byte from fifo
+		writel(reg_temp, &sr->SPI_FD_CONFIG);
+		dev_dbg(&(_s->dev), "SPI_FD_CONFIG =0x%x", readl(&sr->SPI_FD_CONFIG));
+		// set SPI STATUS and start SPI for full duplex (SPI_FD_STATUS)  91.13
+		dev_dbg(&(_s->dev), "TOTAL_LENGTH =0x%x  TX_LENGTH =0x%x xfer_len =0x%x ",
+					TOTAL_LENGTH(xfer_len), TX_LENGTH(xfer_len), xfer_len);
+		writel(TOTAL_LENGTH(xfer_len) | TX_LENGTH(xfer_len) | SPI_START_FD,
+					&sr->SPI_FD_STATUS);
+		DBG_INF("set SPI_FD_STATUS =0x%x", readl(&sr->SPI_FD_STATUS));
+
+		cret = wait_for_completion_interruptible_timeout(&pspim->isr_done, timeout);
+		if (cret <= 0) {
+			dev_dbg(&(_s->dev), "wait_for_completion cret=%ld\n", cret);
+			writel(readl(&sr->SPI_FD_CONFIG) & CLEAN_FLUG_MASK, &sr->SPI_FD_CONFIG);
+			ret = 1;
+			goto free_master_combite_rw;
+		}
+
+		reg_temp = readl(&sr->SPI_FD_STATUS);
+		if (reg_temp & FINISH_FLAG)
+			writel(readl(&sr->SPI_FD_CONFIG) & CLEAN_FLUG_MASK, &sr->SPI_FD_CONFIG);
+
+		if (t->rx_buf)
+			memcpy(t->rx_buf+i*SPI_TRANS_DATA_SIZE, pspim->rx_data_buf, xfer_len);
+
+		ret = 0;
+
+free_master_combite_rw:
+
+		if (pspim->message_config & CPOL_FD)
+			writel(pspim->message_config, &sr->SPI_FD_CONFIG);
+
+		mutex_unlock(&pspim->buf_lock);
+	}
+
+	return ret;
+}
+
+// called when child device is registering on the bus
+static int pentagram_spi_D_setup(struct spi_device *_s)
+{
+
+#ifdef CONFIG_PM_RUNTIME_SPI
+
+	struct pentagram_spi_master *pspim = spi_controller_get_devdata(_s->controller);
+
+	if (pm_runtime_enabled(pspim->dev)) {
+		ret = pm_runtime_get_sync(pspim->dev)
+		if (ret < 0)
+			goto pm_out;
+	}
+#endif
+
+#ifdef CONFIG_PM_RUNTIME_SPI
+	pm_runtime_put(pspim->dev);
+#endif
+	return 0;
+#ifdef CONFIG_PM_RUNTIME_SPI
+pm_out:
+	pm_runtime_mark_last_busy(pspim->dev);
+	pm_runtime_put_autosuspend(pspim->dev);
+	DBG_INF("pm_out");
+	return 0;
+#endif
+}
+
+// preliminary set CS, CPOL, CPHA and LSB
+// called for both - master and slave. See drivers/spi/spi.c
+static int pentagram_spi_controller_prepare_message(struct spi_controller *_c,
+				    struct spi_message *_m)
+{
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(_c);
+	struct SPI_MAS *sr = (struct SPI_MAS *)pspim->mas_base;
+	struct spi_device *s = _m->spi;
+	// reg clear bits and set bits
+	u32 rs = 0;
+
+	FUNC_DBG();
+	dev_dbg(&(s->dev), "setup %d bpw, %scpol, %scpha, %scs-high, %slsb-first %xcs_gpio\n",
+		s->bits_per_word,
+		s->mode & SPI_CPOL ? "" : "~",
+		s->mode & SPI_CPHA ? "" : "~",
+		s->mode & SPI_CS_HIGH ? "" : "~",
+		s->mode & SPI_LSB_FIRST ? "" : "~",
+		s->cs_gpio);
+
+	writel(readl(&sr->SPI_FD_STATUS) | FD_SW_RST, &sr->SPI_FD_STATUS);
+
+	//set up full duplex frequency and enable  full duplex
+	rs = FD_SEL | ((0xffff) << 16);
+
+	if (s->mode & SPI_CPOL)
+		rs |= CPOL_FD;
+
+	if (s->mode & SPI_LSB_FIRST)
+		rs |= LSB_SEL;
+
+	if (s->mode & SPI_CS_HIGH)
+		rs |= CS_POR;
+
+	if (s->mode & SPI_CPHA)
+		rs |=  CPHA_R;
+	else
+		rs |=  CPHA_W;
+
+	rs |= WRITE_BYTE(0) | READ_BYTE(0);  // set read write byte from fifo
+
+	pspim->message_config = rs;
+
+	if (pspim->message_config & CPOL_FD)
+		writel(pspim->message_config, &sr->SPI_FD_CONFIG);
+
+	return 0;
+}
+static int pentagram_spi_controller_unprepare_message(struct spi_controller *ctlr,
+				    struct spi_message *msg)
+{
+	FUNC_DBG();
+	return 0;
+}
+
+static size_t pentagram_spi_max_length(struct spi_device *spi)
+{
+	return SPI_MSG_DATA_SIZE;
+}
+
+
+
+// SPI-slave R/W
+static int pentagram_spi_S_transfer_one(struct spi_controller *_c, struct spi_device *_s,
+					struct spi_transfer *_t)
+{
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(_c);
+	struct device *dev = pspim->dev;
+	int mode = SPI_IDLE, ret = 0;
+
+	FUNC_DBG();
+#ifdef CONFIG_PM_RUNTIME_SPI
+	if (pm_runtime_enabled(pspim->dev)) {
+		ret = pm_runtime_get_sync(pspim->dev);
+		if (ret < 0)
+			goto pm_out;
+	}
+#endif
+
+	if (spi_controller_is_slave(_c)) {
+
+		pspim->isr_flag = SPI_IDLE;
+
+		if ((_t->tx_buf) && (_t->rx_buf)) {
+			dev_dbg(&_c->dev, "%s() wrong command\n", __func__);
+		} else if (_t->tx_buf) {
+			/* tx_buf is a const void* where we need a void * for
+			 * the dma mapping
+			 */
+			void *nonconst_tx = (void *)_t->tx_buf;
+
+			_t->tx_dma = dma_map_single(dev, nonconst_tx,
+						_t->len, DMA_TO_DEVICE);
+
+			if (dma_mapping_error(dev, _t->tx_dma)) {
+				if (_t->len <= bufsiz) {
+					_t->tx_dma = pspim->tx_dma_phy_base;
+					mode = SPI_SLAVE_WRITE;
+				} else
+					mode = SPI_IDLE;
+
+			} else
+				mode = SPI_SLAVE_WRITE;
+		} else if (_t->rx_buf) {
+
+			_t->rx_dma = dma_map_single(dev, _t->rx_buf,
+				_t->len, DMA_FROM_DEVICE);
+
+			if (dma_mapping_error(dev, _t->rx_dma)) {
+				if (_t->len <= bufsiz) {
+					_t->rx_dma = pspim->rx_dma_phy_base;
+					mode = SPI_SLAVE_READ;
+				} else
+					mode = SPI_IDLE;
+			} else
+				mode = SPI_SLAVE_READ;
+		}
+
+		switch (mode) {
+		case SPI_SLAVE_WRITE:
+		case SPI_SLAVE_READ:
+			pentagram_spi_S_rw(_s, _t, mode);
+			break;
+		default:
+			DBG_INF("idle?");
+			break;
+		}
+
+
+	    //if((xfer->rx_buf) && (xfer->rx_dma == pspim->rx_dma_phy_base)){
+		//    memcpy(xfer->rx_buf, pspim->rx_dma_vir_base, xfer->len);
+	    //}
+
+		if ((_t->tx_buf) && (_t->tx_dma != pspim->tx_dma_phy_base))
+			dma_unmap_single(dev, _t->tx_dma,
+				_t->len, DMA_TO_DEVICE);
+
+		if ((_t->rx_buf) && (_t->rx_dma != pspim->rx_dma_phy_base))
+			dma_unmap_single(dev, _t->rx_dma,
+				_t->len, DMA_FROM_DEVICE);
+
+	}
+
+	spi_finalize_current_transfer(_c);
+
+#ifdef CONFIG_PM_RUNTIME_SPI
+	pm_runtime_put(pspim->dev);
+	DBG_INF("pm_put");
+#endif
+	return ret;
+#ifdef CONFIG_PM_RUNTIME_SPI
+pm_out:
+	pm_runtime_mark_last_busy(pspim->dev);
+	pm_runtime_put_autosuspend(pspim->dev);
+	DBG_INF("pm_out");
+	return ret;
+#endif
+}
+
+
+static int pentagram_spi_M_transfer_one_message(struct spi_controller *ctlr, struct spi_message *m)
+{
+	//struct pentagram_spi_master *pspim = spi_master_get_devdata(ctlr);
+	struct spi_device *spi = m->spi;
+	unsigned int xfer_cnt = 0, total_len = 0;
+	bool start_xfer = false;
+	struct spi_transfer *xfer, *first_xfer = NULL;
+	int ret;
+	bool keep_cs = false;
+
+	FUNC_DBG();
+
+#ifdef CONFIG_PM_RUNTIME_SPI
+	if (pm_runtime_enabled(pspim->dev)) {
+		ret = pm_runtime_get_sync(pspim->dev);
+		if (ret < 0)
+			goto pm_out;
+	}
+#endif
+	pentagram_set_cs(spi, true);
+
+	list_for_each_entry(xfer, &m->transfers, transfer_list) {
+		if (!first_xfer)
+			first_xfer = xfer;
+		total_len +=  xfer->len;
+
+		/* all combined transfers have to have the same speed */
+		if (first_xfer->speed_hz != xfer->speed_hz) {
+			dev_err(&(spi->dev), "unable to change speed between transfers\n");
+			ret = -EINVAL;
+			break;
+		}
+		/* CS will be deasserted directly after transfer */
+//		if ( xfer->delay_usecs) {
+//			DBG_ERR( "can't keep CS asserted after transfer");
+//			ret = -EINVAL;
+//			break;
+//		}
+		if (xfer->len > SPI_MSG_DATA_SIZE) {
+			dev_err(&(spi->dev), "over total trans-length xfer->len = %d", xfer->len);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (list_is_last(&xfer->transfer_list, &m->transfers))
+			DBG_INF("xfer = transfer_list");
+		if (total_len > SPI_TRANS_DATA_SIZE)
+			DBG_INF("(total_len > SPI_TRANS_DATA_SIZE)");
+		if (xfer->cs_change)
+			DBG_INF("xfer->cs_change");
+
+		if (list_is_last(&xfer->transfer_list, &m->transfers)
+			|| (total_len > SPI_TRANS_DATA_SIZE)
+			|| xfer->cs_change || (xfer->len > SPI_TRANS_DATA_SIZE))
+			start_xfer = true;
+
+
+		dev_dbg(&(spi->dev), "start_xfer:%d total_len:%d\n", start_xfer, total_len);
+		if (start_xfer != true) {
+			xfer_cnt++;
+			continue;
+		}
+		if (total_len <= SPI_TRANS_DATA_SIZE)
+			xfer_cnt++;
+
+		if (xfer_cnt > 0) {
+			spspi_prep_transfer(ctlr, spi);
+			pentagram_spi_setup_transfer(spi, ctlr, first_xfer);
+			ret = pentagram_spi_master_combine_write_read(ctlr, first_xfer, xfer_cnt);
+		}
+
+		if (total_len > SPI_TRANS_DATA_SIZE)
+			ret = pentagram_spi_master_transfer(ctlr, spi, xfer);
+
+		if (xfer->delay.value)
+			spspi_delay_ns(xfer->delay.value * 1000);
+		if (xfer->cs_change) {
+			if (list_is_last(&xfer->transfer_list, &m->transfers))
+				keep_cs = true;
+			else {
+				pentagram_set_cs(spi, false);
+				spi_delay_to_ns(&xfer->cs_change_delay, xfer);
+				pentagram_set_cs(spi, true);
+			}
+		}
+
+		m->actual_length += total_len;
+
+		first_xfer = NULL;
+		xfer_cnt = 0;
+		total_len = 0;
+		start_xfer = false;
+	}
+
+	if (ret != 0 || !keep_cs)
+		pentagram_set_cs(spi, false);
+	m->status = ret;
+	spi_finalize_current_message(ctlr);
+
+#ifdef CONFIG_PM_RUNTIME_SPI
+	pm_runtime_put(pspim->dev);
+	DBG_INF("pm_put");
+#endif
+	return ret;
+#ifdef CONFIG_PM_RUNTIME_SPI
+pm_out:
+	pm_runtime_mark_last_busy(pspim->dev);
+	pm_runtime_put_autosuspend(pspim->dev);
+	DBG_INF("pm_out");
+	return 0;
+#endif
+}
+
+static int pentagram_spi_controller_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	int ret;
+	int mode;
+	unsigned int max_freq;
+	struct spi_controller *ctlr;
+	struct pentagram_spi_master *pspim;
+
+	FUNC_DBG();
+
+	pdev->id = 0;
+	mode = SPI_MASTER;
+	if (pdev->dev.of_node) {
+		pdev->id = of_alias_get_id(pdev->dev.of_node, "sp_spi");
+		if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
+			mode = SPI_SLAVE;
+	}
+	dev_dbg(&pdev->dev, "pdev->id = %d\n", pdev->id);
+
+	if (mode == SPI_SLAVE)
+		ctlr = spi_alloc_slave(&pdev->dev, sizeof(*pspim));
+	else
+		ctlr = spi_alloc_master(&pdev->dev, sizeof(*pspim));
+	if (!ctlr)
+		return -ENOMEM;
+
+	ctlr->dev.of_node = pdev->dev.of_node;
+	ctlr->bus_num = pdev->id;
+	// flags, understood by the driver
+	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
+	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
+	ctlr->min_speed_hz = 40000;
+	ctlr->max_speed_hz = 50000000;
+	// ctlr->flags = 0
+	ctlr->max_transfer_size = pentagram_spi_max_length;
+	ctlr->max_message_size = pentagram_spi_max_length;
+	ctlr->setup = pentagram_spi_D_setup;
+	// FIXME: ctlr->auto_runtime_pm = true;
+	ctlr->prepare_message = pentagram_spi_controller_prepare_message;
+	ctlr->unprepare_message = pentagram_spi_controller_unprepare_message;
+
+	if (mode == SPI_SLAVE) {
+		ctlr->transfer_one = pentagram_spi_S_transfer_one;
+		ctlr->slave_abort = pentagram_spi_S_abort;
+	} else {
+		ctlr->use_gpio_descriptors = true;
+		ctlr->transfer_one_message = pentagram_spi_M_transfer_one_message;
+	}
+
+	platform_set_drvdata(pdev, ctlr);
+	pspim = spi_controller_get_devdata(ctlr);
+
+	pspim->ctlr = ctlr;
+	pspim->dev = &pdev->dev;
+	if (!of_property_read_u32(pdev->dev.of_node, "spi-max-frequency", &max_freq)) {
+		dev_dbg(&pdev->dev, "max_freq %d\n", max_freq);
+		pspim->spi_max_frequency = max_freq;
+	} else
+		pspim->spi_max_frequency = 25000000;
+
+	spin_lock_init(&pspim->lock);
+	mutex_init(&pspim->buf_lock);
+	init_completion(&pspim->isr_done);
+	init_completion(&pspim->sla_isr);
+
+	/* find and map our resources */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, MAS_REG_NAME);
+	if (res) {
+		pspim->mas_base = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(pspim->mas_base)) {
+			dev_err(&pdev->dev, "%s devm_ioremap_resource fail\n", MAS_REG_NAME);
+			goto free_alloc;
+		}
+	}
+	dev_dbg(&pdev->dev, "mas_base 0x%x\n", (unsigned int)pspim->mas_base);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, SLA_REG_NAME);
+	if (res) {
+		pspim->sla_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+		if (IS_ERR(pspim->sla_base)) {
+			dev_err(&pdev->dev, "%s devm_ioremap_resource fail\n", SLA_REG_NAME);
+			goto free_alloc;
+		}
+	}
+	dev_dbg(&pdev->dev, "sla_base 0x%x\n", (unsigned int)pspim->sla_base);
+
+	/* irq*/
+	pspim->dma_irq = platform_get_irq_byname(pdev, DMA_IRQ_NAME);
+	if (pspim->dma_irq < 0) {
+		dev_err(&pdev->dev, "failed to get %s\n", DMA_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	pspim->mas_irq = platform_get_irq_byname(pdev, MAS_IRQ_NAME);
+	if (pspim->mas_irq < 0) {
+		dev_err(&pdev->dev, "failed to get %s\n", MAS_IRQ_NAME);
+		goto free_alloc;
+	}
+
+
+	pspim->sla_irq = platform_get_irq_byname(pdev, SLA_IRQ_NAME);
+	if (pspim->sla_irq < 0) {
+		dev_err(&pdev->dev, "failed to get %s\n", SLA_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	/* requset irq*/
+	ret = devm_request_irq(&pdev->dev, pspim->dma_irq, pentagram_spi_M_irq_dma
+						, IRQF_TRIGGER_RISING, pdev->name, pspim);
+	if (ret) {
+		dev_err(&pdev->dev, "%s devm_request_irq fail\n", DMA_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	ret = devm_request_irq(&pdev->dev, pspim->mas_irq, pentagram_spi_M_irq
+						, IRQF_TRIGGER_RISING, pdev->name, pspim);
+	if (ret) {
+		dev_err(&pdev->dev, "%s devm_request_irq fail\n", MAS_IRQ_NAME);
+		goto free_alloc;
+	}
+
+
+	ret = devm_request_irq(&pdev->dev, pspim->sla_irq, pentagram_spi_S_irq
+						, IRQF_TRIGGER_RISING, pdev->name, pspim);
+	if (ret) {
+		dev_err(&pdev->dev, "%s devm_request_irq fail\n", SLA_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	/* clk*/
+	pspim->spi_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pspim->spi_clk)) {
+		dev_err(&pdev->dev, "devm_clk_get fail\n");
+		goto free_alloc;
+	}
+	ret = clk_prepare_enable(pspim->spi_clk);
+	if (ret)
+		goto free_alloc;
+
+	/* reset*/
+	pspim->rstc = devm_reset_control_get(&pdev->dev, NULL);
+	dev_dbg(&pdev->dev, "pspim->rstc : 0x%x\n", (unsigned int)pspim->rstc);
+	if (IS_ERR(pspim->rstc)) {
+		ret = PTR_ERR(pspim->rstc);
+		dev_err(&pdev->dev, "SPI failed to retrieve reset controller: %d\n", ret);
+		goto free_clk;
+	}
+	ret = reset_control_deassert(pspim->rstc);
+	if (ret)
+		goto free_clk;
+
+	/* dma alloc*/
+	pspim->tx_dma_vir_base = dma_alloc_coherent(&pdev->dev, bufsiz,
+					&pspim->tx_dma_phy_base, GFP_ATOMIC);
+	if (!pspim->tx_dma_vir_base)
+		goto free_reset_assert;
+
+	dev_dbg(&pdev->dev, "tx_dma vir 0x%x\n", (unsigned int)pspim->tx_dma_vir_base);
+	dev_dbg(&pdev->dev, "tx_dma phy 0x%x\n", (unsigned int)pspim->tx_dma_phy_base);
+
+	pspim->rx_dma_vir_base = dma_alloc_coherent(&pdev->dev, bufsiz,
+					&pspim->rx_dma_phy_base, GFP_ATOMIC);
+	if (!pspim->rx_dma_vir_base)
+		goto free_tx_dma;
+
+	dev_dbg(&pdev->dev, "rx_dma vir 0x%x\n", (unsigned int)pspim->rx_dma_vir_base);
+	dev_dbg(&pdev->dev, "rx_dma phy 0x%x\n", (unsigned int)pspim->rx_dma_phy_base);
+
+	ret = devm_spi_register_controller(&pdev->dev, ctlr);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "spi_register_master fail\n");
+		goto free_rx_dma;
+	}
+
+#ifdef CONFIG_PM_RUNTIME_SPI
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 5000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	dev_dbg(&pdev->dev, "pm init done\n");
+#endif
+	return 0;
+
+free_rx_dma:
+	dma_free_coherent(&pdev->dev, bufsiz, pspim->rx_dma_vir_base, pspim->rx_dma_phy_base);
+free_tx_dma:
+	dma_free_coherent(&pdev->dev, bufsiz, pspim->tx_dma_vir_base, pspim->tx_dma_phy_base);
+free_reset_assert:
+	reset_control_assert(pspim->rstc);
+free_clk:
+	clk_disable_unprepare(pspim->spi_clk);
+free_alloc:
+	spi_controller_put(ctlr);
+
+	dev_dbg(&pdev->dev, "spi_master_probe done\n");
+	return ret;
+}
+
+static int pentagram_spi_controller_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(master);
+
+	FUNC_DBG();
+
+#ifdef CONFIG_PM_RUNTIME_SPI
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+#endif
+
+	dma_free_coherent(&pdev->dev, bufsiz, pspim->tx_dma_vir_base, pspim->tx_dma_phy_base);
+	dma_free_coherent(&pdev->dev, bufsiz, pspim->rx_dma_vir_base, pspim->rx_dma_phy_base);
+
+	spi_unregister_master(pspim->ctlr);
+	clk_disable_unprepare(pspim->spi_clk);
+	reset_control_assert(pspim->rstc);
+
+	return 0;
+}
+
+static int pentagram_spi_controller_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct spi_controller *ctlr = platform_get_drvdata(pdev);
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(ctlr);
+
+	FUNC_DBG();
+
+	reset_control_assert(pspim->rstc);
+
+	return 0;
+}
+
+static int pentagram_spi_controller_resume(struct platform_device *pdev)
+{
+	struct spi_controller *ctlr = platform_get_drvdata(pdev);
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(ctlr);
+
+	FUNC_DBG();
+
+	reset_control_deassert(pspim->rstc);
+	clk_prepare_enable(pspim->spi_clk);
+
+	return 0;
+}
+
+
+#ifdef CONFIG_PM_RUNTIME_SPI
+static int sp_spi_runtime_suspend(struct device *dev)
+{
+	struct spi_controller *ctlr = platform_get_drvdata(pdev);
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(ctlr);
+
+	dev_dbg(dev, "devid:%d\n", dev->id);
+	reset_control_assert(pspim->rstc);
+
+	return 0;
+}
+
+static int sp_spi_runtime_resume(struct device *dev)
+{
+	struct spi_controller *ctlr = platform_get_drvdata(pdev);
+	struct pentagram_spi_master *pspim = spi_master_get_devdata(ctlr);
+
+	dev_dbg(dev, "devid:%d\n", dev->id);
+	reset_control_deassert(pspim->rstc);
+	clk_prepare_enable(pspim->spi_clk);
+
+	return 0;
+}
+
+static const struct dev_pm_ops sp7021_spi_pm_ops = {
+	.runtime_suspend = sp_spi_runtime_suspend,
+	.runtime_resume  = sp_spi_runtime_resume,
+};
+#endif
+
+static const struct of_device_id pentagram_spi_controller_ids[] = {
+	{ .compatible = "sunplus,sp7021-spi-controller" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pentagram_spi_controller_ids);
+
+static struct platform_driver pentagram_spi_controller_driver = {
+	.probe = pentagram_spi_controller_probe,
+	.remove = pentagram_spi_controller_remove,
+	.suspend	= pentagram_spi_controller_suspend,
+	.resume		= pentagram_spi_controller_resume,
+	.driver = {
+		.name = "sunplus,sp7021-spi-controller",
+		.of_match_table = pentagram_spi_controller_ids,
+#ifdef CONFIG_PM_RUNTIME_SPI
+		.pm     = &sp7021_spi_pm_ops,
+#endif
+	},
+};
+module_platform_driver(pentagram_spi_controller_driver);
+
+MODULE_AUTHOR("lH Kuo <lh.kuo@sunplus.com>");
+MODULE_DESCRIPTION("Sunplus SPI controller driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

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

On Mon, Nov 01, 2021 at 02:18:44PM +0800, LH.Kuo wrote:

> Add SPI driver for Sunplus SP7021.

In general it looks like this needs quite a bit of a refresh for
mainline - a lot of it looks to be a combination of minor, easily
fixable things and stylistic issues which make things hard to follow but
I think there are some more substantial things going on here as well.

One big thing is that the driver appears to copy everything through
bounce buffers ore or less unconditionally.  It's not clear why it's
doing this - if it's for DMA usage in general the buffers supplied to
SPI devices should be suitable for this.

Stylistically the code just doesn't really look like Linux code, there's
a few frequent issues I've highlighted in the review but there's
probably more that are masked, it'd be worth visually comparing the
driver to others and making sure it looks similar.

> +++ b/drivers/spi/spi-sunplus-sp7021.c
> @@ -0,0 +1,1356 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Sunplus SPI controller driver
> + *
> + * Author: Sunplus Technology Co., Ltd.

Please make the entire comment a C++ one so things look more
intentional.

> +//#define DEBUG

Please drop commented out lines like this, there's *lots* of this in the
driver.

> +/* ---------------------------------------------------------------------------------------------- */
> +
> +//#define SPI_FUNC_DEBUG
> +//#define SPI_DBG_INFO
> +#define SPI_DBG_ERR

All this stuff is just duplicating the standard facilities we have in
the kernel for controlling output, please remove it and convert things
over to using normal logging infrastructure rather than a custom setup
for your device driver.

> +#define SPI_FULL_DUPLEX
> +
> +#define MAS_REG_NAME "spi_master"
> +#define SLA_REG_NAME "spi_slave"
> +
> +#define DMA_IRQ_NAME "dma_w_intr"
> +#define MAS_IRQ_NAME "mas_risc_intr"
> +
> +#define SLA_IRQ_NAME "slave_risc_intr"
> +
> +#define SPI_TRANS_DATA_CNT (4)
> +#define SPI_TRANS_DATA_SIZE (255)
> +#define SPI_MSG_DATA_SIZE (SPI_TRANS_DATA_SIZE * SPI_TRANS_DATA_CNT)
> +
> +
> +#define CLEAR_MASTER_INT (1<<6)
> +#define MASTER_INT (1<<7)
> +
> +#define DMA_READ (0xd)
> +#define SLA_SW_RST (1<<1)

Many of these names seem very generic and likely to have collisions in
future, please use prefixes to avoid potential future collisions.

> +struct SPI_MAS {
> +	// Group 091 : SPI_MASTER
> +	unsigned int MST_TX_DATA_ADDR                      ; // 00  (ADDR : 0x9C00_2D80)

Please try to follow the kernel coding style, we don't use upper case
for struct or variable names.

> +	unsigned int MST_TX_DATA_3_2_1_0                   ; // 01  (ADDR : 0x9C00_2D84)

I don't know what these numbers at the end of variable names are
intended to represent but they don't feel like they're helping.

> +	u8 tx_data_buf[SPI_TRANS_DATA_SIZE];
> +	u8 rx_data_buf[SPI_TRANS_DATA_SIZE];

These look like they'd be better as dynamically allocated buffers, aside
from anything else this will ensure that they are well aligned for DMA.

> +static unsigned int bufsiz = 4096;

This should be per device.

> +static void pentagram_set_cs(struct spi_device *_s, bool _on)
> +{
> +	if (_s->mode & SPI_NO_CS)
> +		return;
> +	if (!(_s->cs_gpiod))
> +		return;
> +	dev_dbg(&(_s->dev), "%d gpiod:%d", _on, desc_to_gpio(_s->cs_gpiod));
> +	if (_s->mode & SPI_CS_HIGH)
> +		_on = !_on;
> +	gpiod_set_value_cansleep(_s->cs_gpiod, _on);
> +}

If the device has a GPIO chip select it should use the core support for
GPIO chip selects rather than open coding.

> +static irqreturn_t pentagram_spi_S_irq(int _irq, void *_dev)
> +{
> +	unsigned long flags;
> +	struct pentagram_spi_master *pspim = (struct pentagram_spi_master *)_dev;
> +	struct SPI_SLA *sr = (struct SPI_SLA *)pspim->sla_base;
> +
> +	FUNC_DBG();
> +	spin_lock_irqsave(&pspim->lock, flags);

If you're in an interrupt you should only use the regular spin lock, no
need to save IRQ status.

> +	writel(readl(&sr->RISC_INT_DATA_RDY) | CLEAR_SLAVE_INT, &sr->RISC_INT_DATA_RDY);
> +	spin_unlock_irqrestore(&pspim->lock, flags);
> +	complete(&pspim->sla_isr);
> +	return IRQ_HANDLED;

This appears to unconditionally ack an interrupt regardless of any
status bits?  This seems to be a problem for all the interrupts in this
functions.

> +		writel(readl(&spis_reg->RISC_INT_DATA_RDY) | SLAVE_DATA_RDY,
> +					&spis_reg->RISC_INT_DATA_RDY);
> +		//regs1->SLV_DMA_CTRL = 0x4d;
> +		//regs1->SLV_DMA_LENGTH = 0x50;
> +		//regs1->SLV_DMA_INI_ADDR = 0x300;
> +		//regs1->RISC_INT_DATA_RDY |= 0x1;

This commented out stuff looks worrying...

> +		if (!wait_for_completion_timeout(&pspim->isr_done, timeout)) {
> +			dev_err(&dev, "wait_for_completion_timeout\n");
> +			goto exit_spi_slave_rw;
> +		}

> +exit_spi_slave_rw:
> +	mutex_unlock(&pspim->buf_lock);
> +	return 0;
> +}

If there's an error it's not reported anywhere.

> +	if (RW_phase == SPI_SLAVE_WRITE) {
> +		DBG_INF("S_WRITE len %d", _t->len);
> +		reinit_completion(&pspim->sla_isr);
> +
> +		if (_t->tx_dma == pspim->tx_dma_phy_base)
> +			memcpy(pspim->tx_dma_vir_base, _t->tx_buf, _t->len);
> +
> +		writel_relaxed(DMA_WRITE, &spis_reg->SLV_DMA_CTRL);
> +		writel_relaxed(_t->len, &spis_reg->SLV_DMA_LENGTH);
> +		writel_relaxed(_t->tx_dma, &spis_reg->SLV_DMA_INI_ADDR);
> +		writel(readl(&spis_reg->RISC_INT_DATA_RDY) | SLAVE_DATA_RDY,
> +				&spis_reg->RISC_INT_DATA_RDY);
> +
> +		if (wait_for_completion_interruptible(&pspim->sla_isr))
> +			dev_err(devp, "%s() wait_for_completion timeout\n", __func__);
> +
> +	} else if (RW_phase == SPI_SLAVE_READ) {

These two cases share no code, they should probably be separate
functions (and what happens if it's an unknown phase?).

> +	}
> +}
> +void sp7021spi_wb(struct pentagram_spi_master *_m, u8 _len)

Missing blank line between functions.  In general more blank lines
between blocks would help.

> +{
> +	struct SPI_MAS *sr = (struct SPI_MAS *)_m->mas_base;

These _ names really aren't at all idiomatic for the kernel.  This is a
problem throughout the driver, it's a barrier to legibility.  I'm also
concerned that it looks like you're trying to do accesses to the device
via a struct dereference rather than readl/writel - this will probably
work a lot of the time but I'm not sure it's guaranteed.

> +static irqreturn_t pentagram_spi_M_irq(int _irq, void *_dev)

Nor are capitals in function names.

> +static void spspi_delay_ns(u32 _ns)
> +{
> +

Why is this open coded?

> +	if (_t->speed_hz <= _s->max_speed_hz)
> +		div = clk_rate / _t->speed_hz;
> +	else if (_s->max_speed_hz <= _c->max_speed_hz)
> +		div = clk_rate / _s->max_speed_hz;
> +	else if (_c->max_speed_hz < pspim->spi_max_frequency)
> +		div = clk_rate / _c->max_speed_hz;
> +	else
> +		div = clk_rate / pspim->spi_max_frequency;

The core will set the speed up for the transfer, don't open code this.

> +	if (pspim->tx_cur_len < data_len) {
> +		len_temp = min(pspim->data_unit, data_len);
> +		sp7021spi_wb(pspim, len_temp);
> +	}

What if there's more data?

> +	data_len = 0;
> +	t = first;
> +	for (i = 0; i < transfers_cnt; i++) {
> +		if (t->rx_buf)
> +			memcpy(t->rx_buf, &pspim->rx_data_buf[data_len], t->len);
> +
> +		data_len += t->len;
> +		t = list_entry(t->transfer_list.next, struct spi_transfer, transfer_list);
> +	}

I find it difficult to convince myself that this isn't going to have any
overflow issues, and it'll break operation with anything that does any
manipulation of chip select during the message.  Given that the device
uses GPIO chip selects I'd expect it to just implement transfer_one()
and not handle messages at all.  This would greatly simplify the code.

> +#ifdef CONFIG_PM_RUNTIME_SPI
> +
> +	struct pentagram_spi_master *pspim = spi_controller_get_devdata(_s->controller);
> +
> +	if (pm_runtime_enabled(pspim->dev)) {
> +		ret = pm_runtime_get_sync(pspim->dev)
> +		if (ret < 0)
> +			goto pm_out;
> +	}
> +#endif
> +
> +#ifdef CONFIG_PM_RUNTIME_SPI

The runtime PM code compiles out when disabled, you shouldn't need these
ifdefs (and you definitely shouldn't need two blocks for the same define
together like this).

> +static int pentagram_spi_controller_unprepare_message(struct spi_controller *ctlr,
> +				    struct spi_message *msg)
> +{
> +	FUNC_DBG();
> +	return 0;
> +}

Remove empty functions, the core will cope.

> +static int pentagram_spi_S_transfer_one(struct spi_controller *_c, struct spi_device *_s,
> +					struct spi_transfer *_t)
> +{

So we are using transfer_one?  In that case I'm very confused why the
driver would be walking a transfer list...

> +	struct pentagram_spi_master *pspim = spi_master_get_devdata(_c);
> +	struct device *dev = pspim->dev;
> +	int mode = SPI_IDLE, ret = 0;
> +
> +	FUNC_DBG();
> +#ifdef CONFIG_PM_RUNTIME_SPI
> +	if (pm_runtime_enabled(pspim->dev)) {
> +		ret = pm_runtime_get_sync(pspim->dev);
> +		if (ret < 0)
> +			goto pm_out;
> +	}
> +#endif

Let the core handle runtime PM for you, don't open code it.

> +
> +	if (spi_controller_is_slave(_c)) {
> +
> +		pspim->isr_flag = SPI_IDLE;
> +
> +		if ((_t->tx_buf) && (_t->rx_buf)) {
> +			dev_dbg(&_c->dev, "%s() wrong command\n", __func__);

Return an error, don't just ignore problems.

> +		} else if (_t->tx_buf) {
> +			/* tx_buf is a const void* where we need a void * for
> +			 * the dma mapping
> +			 */
> +			void *nonconst_tx = (void *)_t->tx_buf;

Why do other drivers not need this?  Are you *sure* that's just getting
rid of a const here?

> +			} else
> +				mode = SPI_SLAVE_WRITE;

{ } on both sides of the ifelse.

> +		switch (mode) {
> +		case SPI_SLAVE_WRITE:
> +		case SPI_SLAVE_READ:
> +			pentagram_spi_S_rw(_s, _t, mode);
> +			break;
> +		default:
> +			DBG_INF("idle?");
> +			break;
> +		}

> +	pdev->id = 0;

Why?

> +	ctlr->bus_num = pdev->id;
> +	// flags, understood by the driver
> +	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
> +	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
> +	ctlr->min_speed_hz = 40000;
> +	ctlr->max_speed_hz = 50000000;
> +	// ctlr->flags = 0

The driver should at least be flagging itself as half duplex.

> +	/* find and map our resources */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, MAS_REG_NAME);
> +	if (res) {
> +		pspim->mas_base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource_by_name().

> +	FUNC_DBG();
> +
> +	reset_control_assert(pspim->rstc);
> +
> +	return 0;
> +}
> +
> +static int pentagram_spi_controller_resume(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +	struct pentagram_spi_master *pspim = spi_master_get_devdata(ctlr);
> +
> +	FUNC_DBG();
> +
> +	reset_control_deassert(pspim->rstc);
> +	clk_prepare_enable(pspim->spi_clk);

There's no matching disable...

> +#ifdef CONFIG_PM_RUNTIME_SPI
> +		.pm     = &sp7021_spi_pm_ops,
> +#endif

There's a helper for this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

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

On Fri, Nov 05, 2021 at 03:12:32AM +0000, Lh Kuo 郭力豪 wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

> > > +++ b/drivers/spi/spi-sunplus-sp7021.c
> > > @@ -0,0 +1,1356 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Sunplus SPI controller driver
> > > + *
> > > + * Author: Sunplus Technology Co., Ltd.

> > Please make the entire comment a C++ one so things look more intentional.

> Sorry I don't understand. Is there a explanation?

Make the entire comment block C++ style - //

> > If the device has a GPIO chip select it should use the core support for GPIO
> > chip selects rather than open coding.

> Sorry But I didn't find the core function support to use simply. The core function is too complicated for me.

What did you try and in what way was it complicated?  There's lots of
other drivers using this and it's generally resulted in less code in the
drivers so it seems this should be soemthing we can solve.

> > > +	if (RW_phase == SPI_SLAVE_WRITE) {

> > > +	} else if (RW_phase == SPI_SLAVE_READ) {

> > These two cases share no code, they should probably be separate functions
> > (and what happens if it's an unknown phase?).

> The slave mode of SP7021 is only half duplex.

Not sure that really addresses the concern?

> > > +	if (pspim->tx_cur_len < data_len) {
> > > +		len_temp = min(pspim->data_unit, data_len);
> > > +		sp7021spi_wb(pspim, len_temp);
> > > +	}

> > What if there's more data?

> SP7021 only support 16bytes FIFO transfer.

It can transfer more than one FIFO's worth of data though can't it?

> > I find it difficult to convince myself that this isn't going to have any overflow
> > issues, and it'll break operation with anything that does any manipulation of
> > chip select during the message.  Given that the device uses GPIO chip selects
> > I'd expect it to just implement transfer_one() and not handle messages at all.
> > This would greatly simplify the code.

> More conditions will be checked in the spi-message function.
> In this case, only rx-date is allocated for each transfer of the  message.

Part of the issue with both this and the previous section is code
clarity - it's not just if the code is correct, it's if it's clear that
the code is correct.

> > So we are using transfer_one?  In that case I'm very confused why the driver
> > would be walking a transfer list...

> And the spi of SP7021 includes two working modes: spi-master and spi-slave
> 
> SP7021 spi-master : full-duplex  FIFO-mode only.
> SP7021 spi-slave : helf-duplex  DMA-mode

> It seems that linux can contain these two modes in one drive. And this is what I need.
> Because many registers are overlapped, if they are used in different drives, 
> they will crash if they are declared.

I think the driver needs to be restructured so it's clear which bits
apply to which mode, it's basically two completely separate code paths.
I have to say that it's really very surprising that the two different
modes use such completely different control mechanisms, normally the
differentiation would be more triggered by performance reasons.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] SPI: Add SPI driver for Sunplus SP7021

[Andy Shevchenko]

On Mon, Nov 1, 2021 at 8:37 PM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Nov 01, 2021 at 02:18:44PM +0800, LH.Kuo wrote:
>
> > Add SPI driver for Sunplus SP7021.
>
> In general it looks like this needs quite a bit of a refresh for
> mainline - a lot of it looks to be a combination of minor, easily
> fixable things and stylistic issues which make things hard to follow but
> I think there are some more substantial things going on here as well.

Just looked at their UART driver submission with the similar idea in mind.
Asked them to shrink the size 1.5-2x times. I believe this is possible
(and yes, I already saw a new version which is better, but can be
cleaned up more).

Folks, try again for all your drivers for this SoC.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] SPI: Add SPI driver for Sunplus SP7021

[Philipp Zabel]

On Mon, 2021-11-01 at 14:18 +0800, LH.Kuo wrote:
[...]
> +static int pentagram_spi_controller_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	int ret;
> +	int mode;
> +	unsigned int max_freq;
> +	struct spi_controller *ctlr;
> +	struct pentagram_spi_master *pspim;
> +
> +	FUNC_DBG();

Drop these.

[...]
> +	/* clk*/
> +	pspim->spi_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pspim->spi_clk)) {
> +		dev_err(&pdev->dev, "devm_clk_get fail\n");
> +		goto free_alloc;
> +	}
> +	ret = clk_prepare_enable(pspim->spi_clk);

Move this and reset_control_deassert() as far back as possible.

> +	if (ret)
> +		goto free_alloc;
> +
> +	/* reset*/
> +	pspim->rstc = devm_reset_control_get(&pdev->dev, NULL);

Use devm_reset_control_get_exclusive() instead.
This should be done before clk_prepare_enable().

> +	dev_dbg(&pdev->dev, "pspim->rstc : 0x%x\n", (unsigned int)pspim->rstc);
> +	if (IS_ERR(pspim->rstc)) {
> +		ret = PTR_ERR(pspim->rstc);
> +		dev_err(&pdev->dev, "SPI failed to retrieve reset controller: %d\n", ret);
> +		goto free_clk;
> +	}
> +	ret = reset_control_deassert(pspim->rstc);

Same as the clock, I'd move this after the dma allocations.

> +	if (ret)
> +		goto free_clk;
> +
> +	/* dma alloc*/
> +	pspim->tx_dma_vir_base = dma_alloc_coherent(&pdev->dev, bufsiz,
> +					&pspim->tx_dma_phy_base, GFP_ATOMIC);

Consider using dmam_alloc_coherent, same for rx_dma_vir_base.

regards
Philipp

Re: [PATCH 1/2] SPI: Add SPI driver for Sunplus SP7021

[Lukas Wunner]

On Mon, Nov 01, 2021 at 02:18:44PM +0800, LH.Kuo wrote:
> +	if (mode == SPI_SLAVE)
> +		ctlr = spi_alloc_slave(&pdev->dev, sizeof(*pspim));
> +	else
> +		ctlr = spi_alloc_master(&pdev->dev, sizeof(*pspim));
> +	if (!ctlr)
> +		return -ENOMEM;

You need to use devm_spi_alloc_master() and devm_spi_alloc_slave() here
to avoid a use-after-free in pentagram_spi_controller_remove():

That's because spi_unregister_master() frees the spi_controller struct
and the adjacent pspim allocation and pentagram_spi_controller_remove()
accesses pspim afterwards.

The allocation is *not* freed by spi_unregister_master() if the devm_*
variants are used for allocation.  Rather, the allocation is freed only
after pentagram_spi_controller_remove() has finished.


> +free_alloc:
> +	spi_controller_put(ctlr);

This can be dropped if the devm_* variants are used for allocation.


> +	spi_unregister_master(pspim->ctlr);

Please use spi_unregister_controller() here.  (It could be a slave.)

Thanks,

Lukas

[PATCH 2/2] devicetree bindings SPI Add bindings doc for Sunplus SP7021

[LH.Kuo]

Add devicetree bindings SPI Add bindings doc for Sunplus SP7021

Signed-off-by: LH.Kuo <lh.kuo@sunplus.com>
---
 .../bindings/spi/spi-sunplus-sp7021.yaml           | 95 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 96 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml

diff --git a/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
new file mode 100644
index 0000000..3ce90db
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd. 2021
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/spi-sunplus-sp7021.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sunplus sp7021 SPI controller
+
+allOf:
+  - $ref: "spi-controller.yaml"
+
+maintainers:
+  - lh.kuo <lh.kuo@sunplus.com>
+
+properties:
+  compatible:
+    enum:
+      - sunplus,sp7021-spi-controller
+
+  reg:
+    items:
+      - description: Base address and length of the SPI master registers
+      - description: Base address and length of the SPI slave registers
+
+  reg-names:
+    items:
+      - const: spi_master
+      - const: spi_slave
+
+  interrupt-names:
+    items:
+      - const: dma_w_intr
+      - const: mas_risc_intr
+      - const: slave_risc_intr
+
+  interrupts:
+    minItems: 3
+
+  clocks:
+    maxItems: 1
+
+  clocks-names:
+    items:
+      - const: sys_pll
+
+  resets:
+    maxItems: 1
+
+  pinctrl-names:
+    description:
+      A pinctrl state named "default" must be defined.
+    const: default
+
+  pinctrl-0:
+    description:
+      A phandle to the default pinctrl state.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clocks-names
+  - resets
+  - pinctrl-names
+  - pinctrl-0
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sp-sp7021.h>
+    #include <dt-bindings/reset/sp-sp7021.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi_controller0: spi@9C002D80 {
+        compatible = "sunplus,sp7021-spi-controller";
+        reg = <0x9C002D80 0x80>, <0x9C002E00 0x80>;
+        reg-names = "spi_master", "spi_slave";
+        interrupt-parent = <&intc>;
+        interrupt-names = "dma_w_intr",
+                          "mas_risc_intr",
+                          "slave_risc_intr";
+        interrupts = <144 IRQ_TYPE_LEVEL_HIGH>,
+                     <146 IRQ_TYPE_LEVEL_HIGH>,
+                     <145 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clkc SPI_COMBO_0>;
+        clocks-names = "sys_pll";
+        resets = <&rstc RST_SPI_COMBO_0>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pins_spi0>;
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index f00c466..f6cf9d8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17949,6 +17949,7 @@ SUNPLUS SPI CONTROLLER INTERFACE DRIVER
 M:	LH Kuo <lh.kuo@sunplus.com>
 L:	sdricohcs-devel@lists.sourceforge.net (subscribers-only)
 S:	Maintained
+F:	Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
 F:	drivers/spi/spi-sunplus-sp7021.c
 
 SUPERH
-- 
2.7.4

[PATCH v2 0/2] Add SPI control driver for Sunplus SP7021 SoC

[LH.Kuo]

This is a patch series for SPI driver for Sunplus SP7021 SoC.

Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and
etc.) into a single chip. It is designed for industrial control.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

LH.Kuo (2):
  SPI: Add SPI driver for Sunplus SP7021
  devicetree bindings SPI Add bindings doc for Sunplus SP7021

 .../bindings/spi/spi-sunplus-sp7021.yaml           |   95 ++
 MAINTAINERS                                        |    7 +
 drivers/spi/Kconfig                                |   11 +
 drivers/spi/Makefile                               |    1 +
 drivers/spi/spi-sunplus-sp7021.c                   | 1043 ++++++++++++++++++++
 5 files changed, 1157 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
 create mode 100644 drivers/spi/spi-sunplus-sp7021.c

-- 
2.7.4

[PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

[LH.Kuo]

Add SPI driver for Sunplus SP7021.

Signed-off-by: LH.Kuo <lh.kuo@sunplus.com>
---
 - Addressed all comments from Mr. Mark Brown
 - Addressed all comments from Mr. Philipp Zabel
 - Modified the structure and register access method.

 MAINTAINERS                      |    6 +
 drivers/spi/Kconfig              |   11 +
 drivers/spi/Makefile             |    1 +
 drivers/spi/spi-sunplus-sp7021.c | 1043 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 1061 insertions(+)
 create mode 100644 drivers/spi/spi-sunplus-sp7021.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 170bbbe..34868d0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18189,6 +18189,12 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS SPI CONTROLLER INTERFACE DRIVER
+M:	LH Kuo <lh.kuo@sunplus.com>
+L:	linux-spi@vger.kernel.org
+S:	Maintained
+F:	drivers/spi/spi-sunplus-sp7021.c
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 596705d..30ce0ed 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -866,6 +866,17 @@ config SPI_SUN6I
 	help
 	  This enables using the SPI controller on the Allwinner A31 SoCs.
 
+config SPI_SUNPLUS_SP7021
+	tristate "Sunplus SP7021 SPI controller"
+	depends on SOC_SP7021
+	help
+	  This enable Sunplus SP7021 spi controller driver on the SP7021 SoCs.
+	  This driver can also be built as a module. If so, the module will be
+	  called as spi-sunplus-sp7021.
+
+	  If you have a  Sunplus SP7021 platform say Y here.
+	  If unsure, say N.
+
 config SPI_SYNQUACER
 	tristate "Socionext's SynQuacer HighSpeed SPI controller"
 	depends on ARCH_SYNQUACER || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index dd7393a..b455eaf 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -119,6 +119,7 @@ obj-$(CONFIG_SPI_STM32_QSPI) 		+= spi-stm32-qspi.o
 obj-$(CONFIG_SPI_ST_SSC4)		+= spi-st-ssc4.o
 obj-$(CONFIG_SPI_SUN4I)			+= spi-sun4i.o
 obj-$(CONFIG_SPI_SUN6I)			+= spi-sun6i.o
+obj-$(CONFIG_SPI_SUNPLUS_SP7021)	+= spi-sunplus-sp7021.o
 obj-$(CONFIG_SPI_SYNQUACER)		+= spi-synquacer.o
 obj-$(CONFIG_SPI_TEGRA210_QUAD)		+= spi-tegra210-quad.o
 obj-$(CONFIG_SPI_TEGRA114)		+= spi-tegra114.o
diff --git a/drivers/spi/spi-sunplus-sp7021.c b/drivers/spi/spi-sunplus-sp7021.c
new file mode 100644
index 0000000..45d7aa6
--- /dev/null
+++ b/drivers/spi/spi-sunplus-sp7021.c
@@ -0,0 +1,1043 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (c) 2021 Sunplus Inc.
+// Author: LH Kuo <lh.kuo@sunplus.com>
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/delay.h>
+
+#define SLAVE_INT_IN
+
+
+#define SP7021_MAS_REG_NAME "spi_master"
+#define SP7021_SLA_REG_NAME "spi_slave"
+
+#define SP7021_DMA_IRQ_NAME "dma_w_intr"
+#define SP7021_MAS_IRQ_NAME "mas_risc_intr"
+
+#define SP7021_SLA_IRQ_NAME "slave_risc_intr"
+
+#define SP7021_SPI_DATA_CNT (4)
+#define SP7021_SPI_DATA_SIZE (255)
+#define SP7021_SPI_MSG_SIZE (SP7021_SPI_DATA_SIZE * SP7021_SPI_DATA_CNT)
+
+#define SP7021_CLR_MAS_INT (1<<6)
+
+#define SP7021_SLA_DMA_READ (0xd)
+#define SP7021_SLA_SW_RST (1<<1)
+#define SP7021_SLA_DMA_WRITE (0x4d)
+#define SP7021_SLA_DMA_W_INT (1<<8)
+#define SP7021_SLA_CLR_INT (1<<8)
+#define SP7021_SLA_DATA_RDY (1<<0)
+
+#define SP7021_CLR_MAS_W_INT (1<<7)
+
+#define SP7021_TOTAL_LENGTH(x) (x<<24)
+#define SP7021_TX_LENGTH(x) (x<<16)
+#define SP7021_GET_LEN(x)     ((x>>24)&0xFF)
+#define SP7021_GET_TX_LEN(x)  ((x>>16)&0xFF)
+#define SP7021_GET_RX_CNT(x)  ((x>>12)&0x0F)
+#define SP7021_GET_TX_CNT(x)  ((x>>8)&0x0F)
+
+#define SP7021_FINISH_FLAG (1<<6)
+#define SP7021_FINISH_FLAG_MASK (1<<15)
+#define SP7021_RX_FULL_FLAG (1<<5)
+#define SP7021_RX_FULL_FLAG_MASK (1<<14)
+#define SP7021_RX_EMP_FLAG (1<<4)
+#define SP7021_TX_EMP_FLAG (1<<2)
+#define SP7021_TX_EMP_FLAG_MASK (1<<11)
+#define SP7021_SPI_START_FD (1<<0)
+#define SP7021_FD_SEL (1<<6)
+#define SP7021_LSB_SEL (1<<4)
+#define SP7021_WRITE_BYTE(x) (x<<9)
+#define SP7021_READ_BYTE(x) (x<<7)
+#define SP7021_CLEAN_RW_BYTE (~0x780)
+#define SP7021_CLEAN_FLUG_MASK (~0xF800)
+
+#define SP7021_CPOL_FD (1<<0)
+#define SP7021_CPHA_R (1<<1)
+#define SP7021_CPHA_W (1<<2)
+#define SP7021_CS_POR (1<<5)
+
+#define SP7021_FD_SW_RST (1<<1)
+#define SP7021_FIFO_DATA_BITS (16*8)    // 16 BYTES
+#define SP7021_INT_BYPASS (1<<3)
+
+#define SP7021_FIFO_REG 0x0034
+#define SP7021_SPI_STATUS_REG 0x0038
+#define SP7021_SPI_CONFIG_REG 0x003c
+#define SP7021_INT_BUSY_REG 0x004c
+#define SP7021_DMA_CTRL_REG 0x0050
+
+#define SP7021_DATA_RDY_REG 0x0044
+#define SP7021_SLV_DMA_CTRL_REG 0x0048
+#define SP7021_SLV_DMA_LENGTH_REG 0x004c
+#define SP7021_SLV_DMA_ADDR_REG 0x004c
+
+#define SP7021_BUFSIZE 4096
+
+enum SPI_MODE {
+	SP7021_MAS_READ = 0,
+	SP7021_MAS_WRITE = 1,
+	SP7021_MAS_RW = 2,
+	SP7021_SLA_READ = 3,
+	SP7021_SLA_WRITE = 4,
+	SP7021_SPI_IDLE = 5
+};
+
+enum {
+	SP7021_MASTER_MODE,
+	SP7021_SLAVE_MODE,
+};
+
+
+struct sp7021_spi_ctlr {
+
+	struct device *dev;
+
+	int mode;
+
+	struct spi_master *master;
+	struct spi_controller *ctlr;
+
+	void __iomem *mas_base;
+	void __iomem *sla_base;
+
+	u32 message_config;
+
+	int dma_irq;
+	int mas_irq;
+	int sla_irq;
+
+	struct clk *spi_clk;
+	struct reset_control *rstc;
+
+	spinlock_t lock;
+	struct mutex buf_lock;
+	unsigned int spi_max_frequency;
+	dma_addr_t tx_dma_phy_base;
+	dma_addr_t rx_dma_phy_base;
+	void *tx_dma_vir_base;
+	void *rx_dma_vir_base;
+	struct completion isr_done;	// complete() at *master_(dma|mas)_irq()
+	struct completion sla_isr;	// completion() at spi_S_irq() - slave irq jandler
+	unsigned int data_status;
+
+	unsigned int  rx_cur_len;
+	unsigned int  tx_cur_len;
+
+	u8 tx_data_buf[SP7021_SPI_DATA_SIZE];
+	u8 rx_data_buf[SP7021_SPI_DATA_SIZE];
+
+	int isr_flag;
+
+	unsigned int  data_unit;
+};
+
+static void sp7021_spi_set_cs(struct spi_device *_s, bool _on)
+{
+	if (_s->mode & SPI_NO_CS)
+		return;
+	if (!(_s->cs_gpiod))
+		return;
+	dev_dbg(&(_s->dev), "%d gpiod:%d", _on, desc_to_gpio(_s->cs_gpiod));
+	if (_s->mode & SPI_CS_HIGH)
+		_on = !_on;
+	gpiod_set_value_cansleep(_s->cs_gpiod, _on);
+}
+
+// spi slave irq handler
+static irqreturn_t sp7021_spi_sla_irq(int _irq, void *_dev)
+{
+	struct sp7021_spi_ctlr *pspim = (struct sp7021_spi_ctlr *)_dev;
+
+	spin_lock_irq(&pspim->lock);
+	pspim->data_status = readl(pspim->sla_base + SP7021_DATA_RDY_REG);
+	writel(pspim->data_status | SP7021_SLA_CLR_INT,
+			pspim->sla_base + SP7021_DATA_RDY_REG);
+	spin_unlock_irq(&pspim->lock);
+	complete(&pspim->sla_isr);
+	return IRQ_HANDLED;
+}
+
+// slave only. usually called on driver remove
+static int sp7021_spi_sla_abort(struct spi_controller *_c)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(_c);
+
+	complete(&pspim->sla_isr);
+	complete(&pspim->isr_done);
+	return 0;
+}
+
+// slave R/W, called from S_transfer_one() only
+int sp7021_spi_sla_rw(struct spi_device *_s, struct spi_transfer *_t, int RW_phase)
+{
+	struct sp7021_spi_ctlr *pspim = spi_controller_get_devdata(_s->controller);
+	struct device *devp = &(_s->dev);
+	int err = 0;
+
+
+	mutex_lock(&pspim->buf_lock);
+
+	if (RW_phase == SP7021_SLA_WRITE) {
+		reinit_completion(&pspim->sla_isr);
+
+		if (_t->tx_dma == pspim->tx_dma_phy_base)
+			memcpy(pspim->tx_dma_vir_base, _t->tx_buf, _t->len);
+
+		writel_relaxed(SP7021_SLA_DMA_WRITE, pspim->sla_base + SP7021_SLV_DMA_CTRL_REG);
+		writel_relaxed(_t->len, pspim->sla_base + SP7021_SLV_DMA_LENGTH_REG);
+		writel_relaxed(_t->tx_dma, pspim->sla_base + SP7021_SLV_DMA_ADDR_REG);
+		writel(readl(pspim->sla_base + SP7021_DATA_RDY_REG) | SP7021_SLA_DATA_RDY,
+				pspim->sla_base + SP7021_DATA_RDY_REG);
+
+		if (wait_for_completion_interruptible(&pspim->sla_isr))
+			dev_err(devp, "%s() wait_for_completion timeout\n", __func__);
+
+	} else if (RW_phase == SP7021_SLA_READ) {
+		reinit_completion(&pspim->isr_done);
+		writel(SP7021_SLA_DMA_READ, pspim->sla_base + SP7021_SLV_DMA_CTRL_REG);
+		writel(_t->len, pspim->sla_base + SP7021_SLV_DMA_LENGTH_REG);
+		writel(_t->rx_dma, pspim->sla_base + SP7021_SLV_DMA_ADDR_REG);
+
+		// wait for DMA to complete
+		if (wait_for_completion_interruptible(&pspim->isr_done)) {
+			dev_err(devp, "%s() wait_for_completion timeout\n", __func__);
+			err = -ETIMEDOUT;
+			goto exit_spi_slave_rw;
+		}
+		// FIXME: is "len" correct there?
+		if (_t->tx_dma == pspim->tx_dma_phy_base)
+			memcpy(_t->rx_buf, pspim->rx_dma_vir_base, _t->len);
+
+		writel(SP7021_SLA_SW_RST, pspim->sla_base + SP7021_SLV_DMA_CTRL_REG);
+	}
+
+exit_spi_slave_rw:
+	mutex_unlock(&pspim->buf_lock);
+	return err;
+}
+
+// spi master irq handler
+static irqreturn_t sp7021_spi_mas_irq_dma(int _irq, void *_dev)
+{
+	struct sp7021_spi_ctlr *pspim = (struct sp7021_spi_ctlr *)_dev;
+
+	spin_lock_irq(&pspim->lock);
+	writel(readl(pspim->mas_base + SP7021_DMA_CTRL_REG) | SP7021_CLR_MAS_W_INT,
+		pspim->mas_base + SP7021_DMA_CTRL_REG);
+	spin_unlock_irq(&pspim->lock);
+	complete(&pspim->isr_done);
+	return IRQ_HANDLED;
+}
+
+void sp7021spi_rb(struct sp7021_spi_ctlr *_m, u8 _len)
+{
+	int i;
+
+	for (i = 0; i < _len; i++) {
+		_m->rx_data_buf[_m->rx_cur_len] = readl(_m->mas_base + SP7021_FIFO_REG);
+		_m->rx_cur_len++;
+	}
+}
+void sp7021spi_wb(struct sp7021_spi_ctlr *_m, u8 _len)
+{
+	int i;
+
+	for (i = 0; i < _len; i++) {
+		writel(_m->tx_data_buf[_m->tx_cur_len], _m->mas_base + SP7021_FIFO_REG);
+		_m->tx_cur_len++;
+	}
+}
+
+static irqreturn_t sp7021_spi_mas_irq(int _irq, void *_dev)
+{
+	unsigned long flags;
+	struct sp7021_spi_ctlr *pspim = (struct sp7021_spi_ctlr *)_dev;
+	u32 fd_status = 0;
+	unsigned int tx_len, rx_cnt, tx_cnt;
+	bool isrdone = false;
+
+	spin_lock_irqsave(&pspim->lock, flags);
+
+	fd_status = readl(pspim->mas_base + SP7021_SPI_STATUS_REG);
+	tx_cnt = SP7021_GET_TX_CNT(fd_status);
+	tx_len = SP7021_GET_TX_LEN(fd_status);
+
+	if ((fd_status & SP7021_TX_EMP_FLAG) && (fd_status & SP7021_RX_EMP_FLAG)
+				&& (SP7021_GET_LEN(fd_status) == 0))
+		goto fin_irq;
+
+	rx_cnt = SP7021_GET_RX_CNT(fd_status);
+	// SP7021_RX_FULL_FLAG means RX buffer is full (16 bytes)
+	if (fd_status & SP7021_RX_FULL_FLAG)
+		rx_cnt = pspim->data_unit;
+
+	tx_cnt = min(tx_len - pspim->tx_cur_len, pspim->data_unit - tx_cnt);
+
+	dev_dbg(pspim->dev, "fd_st=0x%x rx_c:%d tx_c:%d tx_l:%d",
+			fd_status, rx_cnt, tx_cnt, tx_len);
+
+	if (rx_cnt > 0)
+		sp7021spi_rb(pspim, rx_cnt);
+	if (tx_cnt > 0)
+		sp7021spi_wb(pspim, tx_cnt);
+
+	fd_status = readl(pspim->mas_base + SP7021_SPI_STATUS_REG);
+
+	if ((fd_status & SP7021_FINISH_FLAG) ||
+		(SP7021_GET_TX_LEN(fd_status) == pspim->tx_cur_len)) {
+
+		while (SP7021_GET_LEN(fd_status) != pspim->rx_cur_len) {
+			fd_status = readl(pspim->mas_base + SP7021_SPI_STATUS_REG);
+			if (fd_status & SP7021_RX_FULL_FLAG)
+				rx_cnt = pspim->data_unit;
+			else
+				rx_cnt = SP7021_GET_RX_CNT(fd_status);
+
+			if (rx_cnt > 0)
+				sp7021spi_rb(pspim, rx_cnt);
+		}
+		writel(readl(pspim->mas_base + SP7021_INT_BUSY_REG) | SP7021_CLR_MAS_INT,
+				pspim->mas_base + SP7021_INT_BUSY_REG);
+		isrdone = true;
+	}
+fin_irq:
+	if (isrdone)
+		complete(&pspim->isr_done);
+	spin_unlock_irqrestore(&pspim->lock, flags);
+	return IRQ_HANDLED;
+}
+
+// called in *controller_transfer_one*
+static void sp7021_prep_transfer(struct spi_controller *_c, struct spi_device *_s)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(_c);
+
+	pspim->tx_cur_len = 0;
+	pspim->rx_cur_len = 0;
+	pspim->data_unit = SP7021_FIFO_DATA_BITS / _s->bits_per_word;
+	pspim->isr_flag = SP7021_SPI_IDLE;
+}
+
+// called from *transfer* functions, set clock there
+static void sp7021_spi_setup_transfer(struct spi_device *_s,
+					struct spi_controller *_c, struct spi_transfer *_t)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(_c);
+	u32 rc = 0, rs = 0;
+	unsigned int clk_rate;
+	unsigned int div;
+	unsigned int clk_sel;
+
+	dev_dbg(&(_s->dev), "setup %dHz", _s->max_speed_hz);
+	dev_dbg(&(_s->dev), "tx %p, rx %p, len %d", _t->tx_buf, _t->rx_buf, _t->len);
+	// set clock
+	clk_rate = clk_get_rate(pspim->spi_clk);
+	div = clk_rate / _t->speed_hz;
+
+	dev_dbg(&(_s->dev), "clk_rate: %d div %d", clk_rate, div);
+
+	clk_sel = (div / 2) - 1;
+	// set full duplex (bit 6) and fd freq (bits 31:16)
+	rc = SP7021_FD_SEL | (0xffff << 16);
+	rs = SP7021_FD_SEL | ((clk_sel & 0xffff) << 16);
+	writel((pspim->message_config & ~rc) | rs, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+}
+
+static int sp7021_spi_master_combine_write_read(struct spi_controller *_c,
+			struct spi_transfer *first, unsigned int transfers_cnt)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(_c);
+	unsigned int data_len = 0;
+	u32 reg_temp = 0;
+	unsigned long timeout = msecs_to_jiffies(200);
+	unsigned int i, len_temp;
+	int ret;
+	struct spi_transfer *t = first;
+	bool xfer_rx = false;
+
+	memset(&pspim->tx_data_buf[0], 0, SP7021_SPI_DATA_SIZE);
+	dev_dbg(&(_c->dev), "txrx: tx %p, rx %p, len %d\n", t->tx_buf, t->rx_buf, t->len);
+
+	mutex_lock(&pspim->buf_lock);
+	reinit_completion(&pspim->isr_done);
+
+	for (i = 0; i < transfers_cnt; i++) {
+		if (t->tx_buf)
+			memcpy(&pspim->tx_data_buf[data_len], t->tx_buf, t->len);
+		if (t->rx_buf)
+			xfer_rx = true;
+		data_len += t->len;
+		t = list_entry(t->transfer_list.next, struct spi_transfer, transfer_list);
+	}
+	dev_dbg(&(_c->dev), "data_len %d xfer_rx %d", data_len, xfer_rx);
+
+	// set SPI FIFO data for full duplex (SPI_FD fifo_data)  91.13
+	if (pspim->tx_cur_len < data_len) {
+		len_temp = min(pspim->data_unit, data_len);
+		sp7021spi_wb(pspim, len_temp);
+	}
+	// initial SPI master config and change to Full-Duplex mode (SPI_FD_CONFIG)  91.15
+	reg_temp = readl(pspim->mas_base + SP7021_SPI_CONFIG_REG);
+	reg_temp &= SP7021_CLEAN_RW_BYTE;
+	reg_temp &= SP7021_CLEAN_FLUG_MASK;
+	reg_temp |= SP7021_FD_SEL;
+	// set SPI master config for full duplex (SPI_FD_CONFIG)  91.15
+	reg_temp |= SP7021_FINISH_FLAG_MASK | SP7021_TX_EMP_FLAG_MASK | SP7021_RX_FULL_FLAG_MASK;
+	reg_temp |= SP7021_WRITE_BYTE(0) | SP7021_READ_BYTE(0);  // set read write byte from fifo
+	writel(reg_temp, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+	// set SPI STATUS and start SPI for full duplex (SPI_FD_STATUS)  91.13
+	writel(SP7021_TOTAL_LENGTH(data_len) | SP7021_TX_LENGTH(data_len) | SP7021_SPI_START_FD,
+				pspim->mas_base + SP7021_SPI_STATUS_REG);
+	writel(readl(pspim->mas_base + SP7021_INT_BUSY_REG) | SP7021_INT_BYPASS,
+				pspim->mas_base + SP7021_INT_BUSY_REG);
+
+	if (!wait_for_completion_timeout(&pspim->isr_done, timeout)) {
+		dev_dbg(&(_c->dev), "wait_for_completion timeout");
+		ret = 1;
+		goto free_master_combite_rw;
+	}
+
+	if (xfer_rx == false) {
+		ret = 0;
+		goto free_master_combite_rw;
+	}
+
+	data_len = 0;
+	t = first;
+	for (i = 0; i < transfers_cnt; i++) {
+		if (t->rx_buf)
+			memcpy(t->rx_buf, &pspim->rx_data_buf[data_len], t->len);
+
+		data_len += t->len;
+		t = list_entry(t->transfer_list.next, struct spi_transfer, transfer_list);
+	}
+	ret = 0;
+free_master_combite_rw:
+	// reset SPI
+	writel(readl(pspim->mas_base + SP7021_SPI_CONFIG_REG) & SP7021_CLEAN_FLUG_MASK,
+					pspim->mas_base + SP7021_SPI_CONFIG_REG);
+
+	if (pspim->message_config & SP7021_CPOL_FD)
+		writel(pspim->message_config, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+
+	mutex_unlock(&pspim->buf_lock);
+	return ret;
+}
+
+static int sp7021_spi_master_transfer(struct spi_controller *_c, struct spi_device *_s,
+		struct spi_transfer *xfer)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(_c);
+	u32 reg_temp = 0;
+	unsigned long timeout = msecs_to_jiffies(200);
+	unsigned int i;
+	int ret;
+	long cret;
+	unsigned int xfer_cnt, xfer_len, last_len;
+	struct spi_transfer *t = xfer;
+
+	xfer_cnt = t->len / SP7021_SPI_DATA_SIZE;
+	last_len = t->len % SP7021_SPI_DATA_SIZE;
+
+	memset(&pspim->tx_data_buf[0], 0, SP7021_SPI_DATA_SIZE);
+
+	dev_dbg(&(_s->dev), "txrx: tx %p, rx %p, len %d\n", t->tx_buf, t->rx_buf, t->len);
+
+	for (i = 0; i <= xfer_cnt; i++) {
+
+		mutex_lock(&pspim->buf_lock);
+
+		sp7021_prep_transfer(_c, _s);
+		sp7021_spi_setup_transfer(_s, _c, xfer);
+
+		reinit_completion(&pspim->isr_done);
+
+		if (i == xfer_cnt)
+			xfer_len = last_len;
+		else
+			xfer_len = SP7021_SPI_DATA_SIZE;
+
+		if (t->tx_buf)
+			memcpy(pspim->tx_data_buf, t->tx_buf+i*SP7021_SPI_DATA_SIZE, xfer_len);
+
+		// set SPI FIFO data for full duplex (SPI_FD fifo_data)  91.13
+		if (pspim->tx_cur_len < xfer_len) {
+			reg_temp = min(pspim->data_unit, xfer_len);
+			sp7021spi_wb(pspim, reg_temp);
+		}
+
+		// initial SPI master config and change to Full-Duplex mode (SPI_FD_CONFIG)  91.15
+		reg_temp = readl(pspim->mas_base + SP7021_SPI_CONFIG_REG);
+		reg_temp &= SP7021_CLEAN_RW_BYTE;
+		reg_temp &= SP7021_CLEAN_FLUG_MASK;
+		reg_temp |= SP7021_FD_SEL;
+		// set SPI master config for full duplex (SPI_FD_CONFIG)  91.15
+		reg_temp |= SP7021_FINISH_FLAG_MASK |
+				SP7021_TX_EMP_FLAG_MASK | SP7021_RX_FULL_FLAG_MASK;
+		reg_temp |= SP7021_WRITE_BYTE(0) | SP7021_READ_BYTE(0);
+		writel(reg_temp, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+		dev_dbg(&(_s->dev), "SP7021_SPI_CONFIG_REG =0x%x",
+			readl(pspim->mas_base + SP7021_SPI_CONFIG_REG));
+		// set SPI STATUS and start SPI for full duplex (SPI_FD_STATUS)  91.13
+		dev_dbg(&(_s->dev), "SP7021_TOTAL_LENGTH =0x%x  SP7021_TX_LENGTH =0x%x ",
+				SP7021_TOTAL_LENGTH(xfer_len), SP7021_TX_LENGTH(xfer_len));
+		writel(SP7021_TOTAL_LENGTH(xfer_len) | SP7021_TX_LENGTH(xfer_len)
+			| SP7021_SPI_START_FD, pspim->mas_base + SP7021_SPI_STATUS_REG);
+
+		cret = wait_for_completion_interruptible_timeout(&pspim->isr_done, timeout);
+		if (cret <= 0) {
+			dev_dbg(&(_s->dev), "wait_for_completion cret=%ld\n", cret);
+			writel(readl(pspim->mas_base + SP7021_SPI_CONFIG_REG) &
+				SP7021_CLEAN_FLUG_MASK, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+			ret = 1;
+			goto free_master_combite_rw;
+		}
+
+		reg_temp = readl(pspim->mas_base + SP7021_SPI_STATUS_REG);
+		if (reg_temp & SP7021_FINISH_FLAG)
+			writel(readl(pspim->mas_base + SP7021_SPI_CONFIG_REG) &
+				SP7021_CLEAN_FLUG_MASK, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+
+		if (t->rx_buf)
+			memcpy(t->rx_buf+i*SP7021_SPI_DATA_SIZE, pspim->rx_data_buf, xfer_len);
+
+		ret = 0;
+
+free_master_combite_rw:
+
+		if (pspim->message_config & SP7021_CPOL_FD)
+			writel(pspim->message_config, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+
+		mutex_unlock(&pspim->buf_lock);
+	}
+
+	return ret;
+}
+
+// called when child device is registering on the bus
+static int sp7021_spi_dev_setup(struct spi_device *_s)
+{
+	struct sp7021_spi_ctlr *pspim = spi_controller_get_devdata(_s->controller);
+	int ret;
+
+	ret = pm_runtime_get_sync(pspim->dev);
+		if (ret < 0)
+			return 0;
+
+	pm_runtime_put(pspim->dev);
+
+	return 0;
+
+}
+
+// preliminary set CS, CPOL, CPHA and LSB
+// called for both - master and slave. See drivers/spi/spi.c
+static int sp7021_spi_controller_prepare_message(struct spi_controller *_c,
+				    struct spi_message *_m)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(_c);
+	struct spi_device *s = _m->spi;
+	// reg clear bits and set bits
+	u32 rs = 0;
+
+	dev_dbg(&(s->dev), "setup %d bpw, %scpol, %scpha, %scs-high, %slsb-first %xcs_gpio\n",
+		s->bits_per_word,
+		s->mode & SPI_CPOL ? "" : "~",
+		s->mode & SPI_CPHA ? "" : "~",
+		s->mode & SPI_CS_HIGH ? "" : "~",
+		s->mode & SPI_LSB_FIRST ? "" : "~",
+		s->cs_gpio);
+
+	writel(readl(pspim->mas_base + SP7021_SPI_STATUS_REG) | SP7021_FD_SW_RST,
+					pspim->mas_base + SP7021_SPI_STATUS_REG);
+
+	//set up full duplex frequency and enable  full duplex
+	rs = SP7021_FD_SEL | ((0xffff) << 16);
+
+	if (s->mode & SPI_CPOL)
+		rs |= SP7021_CPOL_FD;
+
+	if (s->mode & SPI_LSB_FIRST)
+		rs |= SP7021_LSB_SEL;
+
+	if (s->mode & SPI_CS_HIGH)
+		rs |= SP7021_CS_POR;
+
+	if (s->mode & SPI_CPHA)
+		rs |=  SP7021_CPHA_R;
+	else
+		rs |=  SP7021_CPHA_W;
+
+	rs |= SP7021_WRITE_BYTE(0) | SP7021_READ_BYTE(0);  // set read write byte from fifo
+
+	pspim->message_config = rs;
+
+	if (pspim->message_config & SP7021_CPOL_FD)
+		writel(pspim->message_config, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+
+	return 0;
+}
+
+static int sp7021_spi_controller_unprepare_message(struct spi_controller *_c,
+				    struct spi_message *msg)
+{
+	return 0;
+}
+
+static size_t sp7021_spi_max_length(struct spi_device *spi)
+{
+	return SP7021_SPI_MSG_SIZE;
+}
+
+
+
+// SPI-slave R/W
+static int sp7021_spi_sla_transfer_one(struct spi_controller *_c, struct spi_device *_s,
+					struct spi_transfer *_t)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(_c);
+	struct device *dev = pspim->dev;
+	int mode = SP7021_SPI_IDLE, ret = 0;
+
+	pm_runtime_get_sync(pspim->dev);
+
+	if (spi_controller_is_slave(_c)) {
+
+		pspim->isr_flag = SP7021_SPI_IDLE;
+
+		if ((_t->tx_buf) && (_t->rx_buf)) {
+			dev_dbg(&_c->dev, "%s() wrong command\n", __func__);
+			ret = -EINVAL;
+		} else if (_t->tx_buf) {
+			_t->tx_dma = dma_map_single(dev, (void *)_t->tx_buf,
+						_t->len, DMA_TO_DEVICE);
+
+			if (dma_mapping_error(dev, _t->tx_dma)) {
+				if (_t->len <= SP7021_BUFSIZE) {
+					_t->tx_dma = pspim->tx_dma_phy_base;
+					mode = SP7021_SLA_WRITE;
+				} else
+					mode = SP7021_SPI_IDLE;
+
+			} else
+				mode = SP7021_SLA_WRITE;
+		} else if (_t->rx_buf) {
+
+			_t->rx_dma = dma_map_single(dev, _t->rx_buf,
+				_t->len, DMA_FROM_DEVICE);
+
+			if (dma_mapping_error(dev, _t->rx_dma)) {
+				if (_t->len <= SP7021_BUFSIZE) {
+					_t->rx_dma = pspim->rx_dma_phy_base;
+					mode = SP7021_SLA_READ;
+				} else
+					mode = SP7021_SPI_IDLE;
+			} else
+				mode = SP7021_SLA_READ;
+		}
+
+		switch (mode) {
+		case SP7021_SLA_WRITE:
+		case SP7021_SLA_READ:
+			sp7021_spi_sla_rw(_s, _t, mode);
+			break;
+		default:
+			break;
+		}
+
+		if ((_t->tx_buf) && (_t->tx_dma != pspim->tx_dma_phy_base))
+			dma_unmap_single(dev, _t->tx_dma,
+				_t->len, DMA_TO_DEVICE);
+
+		if ((_t->rx_buf) && (_t->rx_dma != pspim->rx_dma_phy_base))
+			dma_unmap_single(dev, _t->rx_dma,
+				_t->len, DMA_FROM_DEVICE);
+
+	}
+
+	spi_finalize_current_transfer(_c);
+
+	pm_runtime_put(pspim->dev);
+	return ret;
+
+}
+
+static int sp7021_spi_mas_transfer_one_message(struct spi_controller *_c, struct spi_message *m)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(_c);
+	struct spi_device *spi = m->spi;
+	unsigned int xfer_cnt = 0, total_len = 0;
+	bool start_xfer = false;
+	struct spi_transfer *xfer, *first_xfer = NULL;
+	int ret;
+	bool keep_cs = false;
+
+	pm_runtime_get_sync(pspim->dev);
+
+	sp7021_spi_set_cs(spi, true);
+
+	list_for_each_entry(xfer, &m->transfers, transfer_list) {
+		if (!first_xfer)
+			first_xfer = xfer;
+		total_len +=  xfer->len;
+
+		/* all combined transfers have to have the same speed */
+		if (first_xfer->speed_hz != xfer->speed_hz) {
+			dev_err(&(spi->dev), "unable to change speed between transfers\n");
+			ret = -EINVAL;
+			break;
+		}
+
+		if (xfer->len > SP7021_SPI_MSG_SIZE) {
+			dev_err(&(spi->dev), "over total trans-length xfer->len = %d", xfer->len);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (list_is_last(&xfer->transfer_list, &m->transfers))
+			dev_dbg(&(spi->dev), "xfer = transfer_list");
+		if (total_len > SP7021_SPI_DATA_SIZE)
+			dev_dbg(&(spi->dev), "(total_len > SP7021_SPI_DATA_SIZE)");
+		if (xfer->cs_change)
+			dev_dbg(&(spi->dev), "xfer->cs_change");
+
+		if (list_is_last(&xfer->transfer_list, &m->transfers)
+			|| (total_len > SP7021_SPI_DATA_SIZE)
+			|| xfer->cs_change || (xfer->len > SP7021_SPI_DATA_SIZE))
+			start_xfer = true;
+
+
+		dev_dbg(&(spi->dev), "start_xfer:%d total_len:%d\n", start_xfer, total_len);
+		if (start_xfer != true) {
+			xfer_cnt++;
+			continue;
+		}
+		if (total_len <= SP7021_SPI_DATA_SIZE)
+			xfer_cnt++;
+
+		if (xfer_cnt > 0) {
+			sp7021_prep_transfer(_c, spi);
+			sp7021_spi_setup_transfer(spi, _c, first_xfer);
+			ret = sp7021_spi_master_combine_write_read(_c, first_xfer, xfer_cnt);
+		}
+
+		if (total_len > SP7021_SPI_DATA_SIZE)
+			ret = sp7021_spi_master_transfer(_c, spi, xfer);
+
+		if (xfer->delay.value)
+			spi_delay_to_ns(&xfer->delay, xfer);
+
+		if (xfer->cs_change) {
+			if (list_is_last(&xfer->transfer_list, &m->transfers))
+				keep_cs = true;
+			else {
+				sp7021_spi_set_cs(spi, false);
+				spi_delay_to_ns(&xfer->cs_change_delay, xfer);
+				sp7021_spi_set_cs(spi, true);
+			}
+		}
+
+		m->actual_length += total_len;
+
+		first_xfer = NULL;
+		xfer_cnt = 0;
+		total_len = 0;
+		start_xfer = false;
+	}
+
+	if (ret != 0 || !keep_cs)
+		sp7021_spi_set_cs(spi, false);
+	m->status = ret;
+	spi_finalize_current_message(_c);
+
+
+	pm_runtime_put(pspim->dev);
+
+	return ret;
+
+	pm_runtime_mark_last_busy(pspim->dev);
+	pm_runtime_put_autosuspend(pspim->dev);
+	return 0;
+
+}
+
+static int sp7021_spi_controller_probe(struct platform_device *pdev)
+{
+	int ret;
+	int mode;
+	struct spi_controller *ctlr;
+	struct sp7021_spi_ctlr *pspim;
+
+	pdev->id = 0;
+	mode = SP7021_MASTER_MODE;
+	if (pdev->dev.of_node) {
+		pdev->id = of_alias_get_id(pdev->dev.of_node, "sp_spi");
+		if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
+			mode = SP7021_SLAVE_MODE;
+	}
+	dev_dbg(&pdev->dev, "pdev->id = %d\n", pdev->id);
+
+	if (mode == SP7021_SLAVE_MODE)
+		ctlr = spi_alloc_slave(&pdev->dev, sizeof(*pspim));
+	else
+		ctlr = spi_alloc_master(&pdev->dev, sizeof(*pspim));
+	if (!ctlr)
+		return -ENOMEM;
+
+	ctlr->dev.of_node = pdev->dev.of_node;
+	ctlr->bus_num = pdev->id;
+	// flags, understood by the driver
+	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
+	ctlr->max_transfer_size = sp7021_spi_max_length;
+	ctlr->max_message_size = sp7021_spi_max_length;
+	ctlr->setup = sp7021_spi_dev_setup;
+	// FIXME: ctlr->auto_runtime_pm = true;
+	ctlr->prepare_message = sp7021_spi_controller_prepare_message;
+	ctlr->unprepare_message = sp7021_spi_controller_unprepare_message;
+
+	if (mode == SP7021_SLAVE_MODE) {
+		ctlr->transfer_one = sp7021_spi_sla_transfer_one;
+		ctlr->slave_abort = sp7021_spi_sla_abort;
+		ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX;
+	} else {
+		ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
+		ctlr->min_speed_hz = 40000;
+		ctlr->max_speed_hz = 25000000;
+		ctlr->use_gpio_descriptors = true;
+		ctlr->transfer_one_message = sp7021_spi_mas_transfer_one_message;
+	}
+
+	dev_set_drvdata(&pdev->dev, ctlr);
+	pspim = spi_controller_get_devdata(ctlr);
+
+	pspim->ctlr = ctlr;
+	pspim->dev = &pdev->dev;
+
+	spin_lock_init(&pspim->lock);
+	mutex_init(&pspim->buf_lock);
+	init_completion(&pspim->isr_done);
+	init_completion(&pspim->sla_isr);
+
+	pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, SP7021_MAS_REG_NAME);
+	pspim->sla_base = devm_platform_ioremap_resource_byname(pdev, SP7021_SLA_REG_NAME);
+
+	dev_dbg(&pdev->dev, "mas_base 0x%x\n", (unsigned int)pspim->mas_base);
+
+	/* irq*/
+	pspim->dma_irq = platform_get_irq_byname(pdev, SP7021_DMA_IRQ_NAME);
+	if (pspim->dma_irq < 0) {
+		dev_err(&pdev->dev, "failed to get %s\n", SP7021_DMA_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	pspim->mas_irq = platform_get_irq_byname(pdev, SP7021_MAS_IRQ_NAME);
+	if (pspim->mas_irq < 0) {
+		dev_err(&pdev->dev, "failed to get %s\n", SP7021_MAS_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	pspim->sla_irq = platform_get_irq_byname(pdev, SP7021_SLA_IRQ_NAME);
+	if (pspim->sla_irq < 0) {
+		dev_err(&pdev->dev, "failed to get %s\n", SP7021_SLA_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	ret = devm_request_irq(&pdev->dev, pspim->dma_irq, sp7021_spi_mas_irq_dma
+						, IRQF_TRIGGER_RISING, pdev->name, pspim);
+	if (ret) {
+		dev_err(&pdev->dev, "%s devm_request_irq fail\n", SP7021_DMA_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	/* requset irq*/
+	ret = devm_request_irq(&pdev->dev, pspim->mas_irq, sp7021_spi_mas_irq
+						, IRQF_TRIGGER_RISING, pdev->name, pspim);
+	if (ret) {
+		dev_err(&pdev->dev, "%s devm_request_irq fail\n", SP7021_MAS_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	ret = devm_request_irq(&pdev->dev, pspim->sla_irq, sp7021_spi_sla_irq
+						, IRQF_TRIGGER_RISING, pdev->name, pspim);
+	if (ret) {
+		dev_err(&pdev->dev, "%s devm_request_irq fail\n", SP7021_SLA_IRQ_NAME);
+		goto free_alloc;
+	}
+
+	/* dma alloc*/
+	pspim->tx_dma_vir_base = dmam_alloc_coherent(&pdev->dev, SP7021_BUFSIZE,
+					&pspim->tx_dma_phy_base, GFP_ATOMIC);
+	if (!pspim->tx_dma_vir_base)
+		goto free_alloc;
+
+	dev_dbg(&pdev->dev, "tx_dma vir 0x%x\n", (unsigned int)pspim->tx_dma_vir_base);
+	dev_dbg(&pdev->dev, "tx_dma phy 0x%x\n", (unsigned int)pspim->tx_dma_phy_base);
+
+	pspim->rx_dma_vir_base = dmam_alloc_coherent(&pdev->dev, SP7021_BUFSIZE,
+					&pspim->rx_dma_phy_base, GFP_ATOMIC);
+	if (!pspim->rx_dma_vir_base)
+		goto free_tx_dma;
+
+	dev_dbg(&pdev->dev, "rx_dma vir 0x%x\n", (unsigned int)pspim->rx_dma_vir_base);
+	dev_dbg(&pdev->dev, "rx_dma phy 0x%x\n", (unsigned int)pspim->rx_dma_phy_base);
+
+	/* clk*/
+	pspim->spi_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pspim->spi_clk)) {
+		dev_err(&pdev->dev, "devm_clk_get fail\n");
+		goto free_rx_dma;
+	}
+
+	/* reset*/
+	pspim->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	dev_dbg(&pdev->dev, "pspim->rstc : 0x%x\n", (unsigned int)pspim->rstc);
+	if (IS_ERR(pspim->rstc)) {
+		ret = PTR_ERR(pspim->rstc);
+		dev_err(&pdev->dev, "SPI failed to retrieve reset controller: %d\n", ret);
+		goto free_reset_assert;
+	}
+
+	ret = clk_prepare_enable(pspim->spi_clk);
+	if (ret)
+		goto free_reset_assert;
+
+	ret = reset_control_deassert(pspim->rstc);
+	if (ret)
+		goto free_reset_assert;
+
+	ret = devm_spi_register_controller(&pdev->dev, ctlr);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "spi_register_master fail\n");
+		goto free_reset_assert;
+	}
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 5000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+	dev_dbg(&pdev->dev, "pm init done\n");
+
+	return 0;
+
+free_reset_assert:
+	reset_control_assert(pspim->rstc);
+	clk_disable_unprepare(pspim->spi_clk);
+free_rx_dma:
+	dma_free_coherent(&pdev->dev, SP7021_BUFSIZE,
+		pspim->rx_dma_vir_base, pspim->rx_dma_phy_base);
+free_tx_dma:
+	dma_free_coherent(&pdev->dev, SP7021_BUFSIZE,
+		pspim->tx_dma_vir_base, pspim->tx_dma_phy_base);
+free_alloc:
+	spi_controller_put(ctlr);
+
+	dev_dbg(&pdev->dev, "spi_master_probe done\n");
+	return ret;
+}
+
+static int sp7021_spi_controller_remove(struct platform_device *pdev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+
+	dma_free_coherent(&pdev->dev, SP7021_BUFSIZE,
+		pspim->tx_dma_vir_base, pspim->tx_dma_phy_base);
+	dma_free_coherent(&pdev->dev, SP7021_BUFSIZE,
+		pspim->rx_dma_vir_base, pspim->rx_dma_phy_base);
+
+	spi_unregister_master(pspim->ctlr);
+	clk_disable_unprepare(pspim->spi_clk);
+	reset_control_assert(pspim->rstc);
+
+	return 0;
+}
+
+static int __maybe_unused sp7021_spi_controller_suspend(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	reset_control_assert(pspim->rstc);
+
+	return 0;
+}
+
+static int __maybe_unused sp7021_spi_controller_resume(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	reset_control_deassert(pspim->rstc);
+	clk_prepare_enable(pspim->spi_clk);
+
+	return 0;
+}
+
+static int sp7021_spi_runtime_suspend(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	dev_dbg(dev, "devid:%d\n", dev->id);
+	reset_control_assert(pspim->rstc);
+
+	return 0;
+}
+
+static int sp7021_spi_runtime_resume(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	dev_dbg(dev, "devid:%d\n", dev->id);
+	reset_control_deassert(pspim->rstc);
+	clk_prepare_enable(pspim->spi_clk);
+
+	return 0;
+}
+
+static const struct dev_pm_ops sp7021_spi_pm_ops = {
+	SET_RUNTIME_PM_OPS(sp7021_spi_runtime_suspend,
+				sp7021_spi_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(sp7021_spi_controller_suspend,
+				sp7021_spi_controller_resume)
+};
+
+static const struct of_device_id sp7021_spi_controller_ids[] = {
+	{ .compatible = "sunplus,sp7021-spi-controller" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sp7021_spi_controller_ids);
+
+static struct platform_driver sp7021_spi_controller_driver = {
+	.probe = sp7021_spi_controller_probe,
+	.remove = sp7021_spi_controller_remove,
+	.driver = {
+		.name = "sunplus,sp7021-spi-controller",
+		.of_match_table = sp7021_spi_controller_ids,
+		.pm     = &sp7021_spi_pm_ops,
+	},
+};
+module_platform_driver(sp7021_spi_controller_driver);
+
+MODULE_AUTHOR("lH Kuo <lh.kuo@sunplus.com>");
+MODULE_DESCRIPTION("Sunplus SPI controller driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

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

On Tue, Nov 09, 2021 at 05:01:27PM +0800, LH.Kuo wrote:

A lot of my previous feedback on this driver still applies, while some
of the issues have been addressed most of the major structural issues
continue to apply here.  A lot of the code is replicating code from the
core or is really hard to explain, it's hard to see anything super
unusual with the hardware here that would require such unusual code.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.

> +static void sp7021_spi_set_cs(struct spi_device *_s, bool _on)
> +{
> +	if (_s->mode & SPI_NO_CS)
> +		return;
> +	if (!(_s->cs_gpiod))
> +		return;
> +	dev_dbg(&(_s->dev), "%d gpiod:%d", _on, desc_to_gpio(_s->cs_gpiod));
> +	if (_s->mode & SPI_CS_HIGH)
> +		_on = !_on;
> +	gpiod_set_value_cansleep(_s->cs_gpiod, _on);
> +}

This is *still* open coding a GPIO chip select, to repeat what I said
last time this is not OK - use the core facilities to avoid introducing
bugs like double application of SPI_CS_HIGH you have here.

> +// spi slave irq handler
> +static irqreturn_t sp7021_spi_sla_irq(int _irq, void *_dev)
> +{
> +	struct sp7021_spi_ctlr *pspim = (struct sp7021_spi_ctlr *)_dev;

If you need this cast something is very wrong, do you need it?

> +int sp7021_spi_sla_rw(struct spi_device *_s, struct spi_transfer *_t, int RW_phase)

Please use the normal kernel coding style, using _s for parameter names
or mixed case symbol names isn't normal for the kernel.  There's also
issues with line lengths over 80 columns all over, while it's not a
strict limit it's still good try to keep things to a reasonable length.

> +	if (RW_phase == SP7021_SLA_WRITE) {

This looks like a switch statement, though given how little code is
shared it's not clear why this is all in one function.

> +		if (_t->tx_dma == pspim->tx_dma_phy_base)
> +			memcpy(pspim->tx_dma_vir_base, _t->tx_buf, _t->len);

Why are we copying data into a DMA transfer buffer, doesn't this defeat
the point of DMA?  I'd expect to DMA data directly.  I'd also expect
some synchronisation operations to ensure that everything has a
consistent view of the memory.

> +// spi master irq handler
> +static irqreturn_t sp7021_spi_mas_irq_dma(int _irq, void *_dev)
> +{
> +	struct sp7021_spi_ctlr *pspim = (struct sp7021_spi_ctlr *)_dev;
> +
> +	spin_lock_irq(&pspim->lock);

Why are we using spin_lock_irq() when we're already in an interrupt
handler?

> +	}
> +}
> +void sp7021spi_wb(struct sp7021_spi_ctlr *_m, u8 _len)

Blank lines between functions.

> +fin_irq:
> +	if (isrdone)
> +		complete(&pspim->isr_done);
> +	spin_unlock_irqrestore(&pspim->lock, flags);
> +	return IRQ_HANDLED;
> +}

This unconditionally reports IRQ_HANDLED even if we didn't actually see
any interrupt status flagged, this will break shared interrupts and
reduce the ability of the interrupt core to handle errors.

> +	for (i = 0; i < transfers_cnt; i++) {
> +		if (t->tx_buf)
> +			memcpy(&pspim->tx_data_buf[data_len], t->tx_buf, t->len);
> +		if (t->rx_buf)
> +			xfer_rx = true;
> +		data_len += t->len;
> +		t = list_entry(t->transfer_list.next, struct spi_transfer, transfer_list);
> +	}

This is still copying all data for no apparent reason as highlighted
last time.

> +	dev_dbg(&(_c->dev), "data_len %d xfer_rx %d", data_len, xfer_rx);
> +
> +	// set SPI FIFO data for full duplex (SPI_FD fifo_data)  91.13
> +	if (pspim->tx_cur_len < data_len) {
> +		len_temp = min(pspim->data_unit, data_len);
> +		sp7021spi_wb(pspim, len_temp);
> +	}

Is the device full duplex or half duplex?  The code immediately above
this treats both transmit and recieve buffers as optional.  If the
device does actually need to be full duplex then the driver should flag
it as such.

> +// called when child device is registering on the bus
> +static int sp7021_spi_dev_setup(struct spi_device *_s)
> +{
> +	struct sp7021_spi_ctlr *pspim = spi_controller_get_devdata(_s->controller);
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(pspim->dev);
> +		if (ret < 0)
> +			return 0;
> +
> +	pm_runtime_put(pspim->dev);
> +
> +	return 0;
> +
> +}

This function does nothing except bounce the power on the device, this
is obviously not useful and should be removed.

> +static int sp7021_spi_controller_unprepare_message(struct spi_controller *_c,
> +				    struct spi_message *msg)
> +{
> +	return 0;
> +}

Remove empty functions.

> +static size_t sp7021_spi_max_length(struct spi_device *spi)
> +{
> +	return SP7021_SPI_MSG_SIZE;
> +}

Is there any actual limit in the hardware?  This looks very much like
it's a completely artificial limit in the driver for no obvious reason.

> +static int sp7021_spi_mas_transfer_one_message(struct spi_controller *_c, struct spi_message *m)
> +{
> +	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(_c);
> +	struct spi_device *spi = m->spi;
> +	unsigned int xfer_cnt = 0, total_len = 0;
> +	bool start_xfer = false;
> +	struct spi_transfer *xfer, *first_xfer = NULL;
> +	int ret;
> +	bool keep_cs = false;
> +
> +	pm_runtime_get_sync(pspim->dev);

To repeat the feedback from last time do not open code runtime PM, use
the core support.

> +	sp7021_spi_set_cs(spi, true);
> +
> +	list_for_each_entry(xfer, &m->transfers, transfer_list) {
> +		if (!first_xfer)
> +			first_xfer = xfer;
> +		total_len +=  xfer->len;

To further repeat my prior feedback I can't see any reason why this
driver doesn't just use transfer_one().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

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

On Wed, Nov 10, 2021 at 02:42:01AM +0000, Lh Kuo 郭力豪 wrote:

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

> > This is *still* open coding a GPIO chip select, to repeat what I said last time
> > this is not OK - use the core facilities to avoid introducing bugs like double
> > application of SPI_CS_HIGH you have here.

> I try to find some function can replay this part.
> EX:
>   Spi.c -> spi_set_cs but it is not EXPORT_SYMBOL_GPL and it looks not fit in the driver
> 
>   Spi-gpio.c -> spi_gpio_chipselect it looks not fit in the driver.
> 
> Sorry maybe i misunderstood this issue
> 
>   Or the problem is 	gpiod_set_value_cansleep  can't be used in here ?
> 
> Which function can I use for GPIO_CS?

The same way other controllers do: by setting use_gpio_descriptors in
the controller.  The core will then request the GPIOs for the driver
using the standard binding, this requires no further work on the part of
the driver.

> > > +// spi slave irq handler
> > > +static irqreturn_t sp7021_spi_sla_irq(int _irq, void *_dev) {
> > > +	struct sp7021_spi_ctlr *pspim = (struct sp7021_spi_ctlr *)_dev;

> > If you need this cast something is very wrong, do you need it?

> So the vold* should be struct * spi_controller ??

No, interrupt handlers need to take an int and a void *.  There should
be no cast.

> > > +	if (RW_phase == SP7021_SLA_WRITE) {

> > This looks like a switch statement, though given how little code is shared it's
> > not clear why this is all in one function.

> It is easy to check the flow and setting for me

It's contributing to making the code hard for other people to follow.

> > > +		if (_t->tx_dma == pspim->tx_dma_phy_base)
> > > +			memcpy(pspim->tx_dma_vir_base, _t->tx_buf, _t->len);

> > Why are we copying data into a DMA transfer buffer, doesn't this defeat the
> > point of DMA?  I'd expect to DMA data directly.  I'd also expect some
> > synchronisation operations to ensure that everything has a consistent view of
> > the memory.

> It only happens when tx_dma = pspim->tx_dma_phy_base
> And if it can't get dma-addr or wrong case. I will set tx_dma = pspim->tx_dma_phy_base.

What does that mean at a higher level - what is tx_dma here?  Why does
not being able to get an address to DMA mean that we need to memcpy()
things?  I can't see any reason for the memcpy() at all.

> > > +// spi master irq handler
> > > +static irqreturn_t sp7021_spi_mas_irq_dma(int _irq, void *_dev) {
> > > +	struct sp7021_spi_ctlr *pspim = (struct sp7021_spi_ctlr *)_dev;

> > > +	spin_lock_irq(&pspim->lock);

> > Why are we using spin_lock_irq() when we're already in an interrupt handler?

> Yes it is in interrupt handler

Yes, I know it's an interrupt handler - to repeat my question why are we
we using spin_lock_irq() in that case?

> > > +	return IRQ_HANDLED;
> > > +}

> > This unconditionally reports IRQ_HANDLED even if we didn't actually see any
> > interrupt status flagged, this will break shared interrupts and reduce the ability
> > of the interrupt core to handle errors.

> Sorry I'm confuse. What should i do in this issue

Report IRQ_NONE if there was no interrupts reported by the hardware.

> > This is still copying all data for no apparent reason as highlighted last time.

> It is difference case. It is in master mode and and can only be transmitted through FIFO

> And It is transmitting for one message and I need collect the all tx data first.

For what reason do you need to collect all the tx data?  It really is
not at all apparent from the code and seems especially unusual in the
PIO case.

> > Is the device full duplex or half duplex?  The code immediately above this
> > treats both transmit and recieve buffers as optional.  If the device does
> > actually need to be full duplex then the driver should flag it as such.

> You mean set the flsg of should be struct * spi_controller in probe function

> Ctlr-flags = SPI_CONTROLLER_FULL_DUPLEX  right ?

Yes.

> > > +// called when child device is registering on the bus static int
> > > +sp7021_spi_dev_setup(struct spi_device *_s) {
> > > +	struct sp7021_spi_ctlr *pspim =
> > spi_controller_get_devdata(_s->controller);
> > > +	int ret;
> > > +
> > > +	ret = pm_runtime_get_sync(pspim->dev);
> > > +		if (ret < 0)
> > > +			return 0;
> > > +
> > > +	pm_runtime_get_sync(pspim->dev);;
> > > +
> > > +	return 0;
> > > +
> > > +}

> > This function does nothing except bounce the power on the device, this is
> > obviously not useful and should be removed.

> You mean set the auto_runtime_pm of should be struct * spi_controller in probe function
> And remove pm_runtime_get_sync(pspim->dev);

> pm_runtime_get_sync(pspim->dev);

> even the pm_runtime in the probe should be remove . right ?

You should only take a runtime reference for the period that it's
actually needed.  Taking one in probe and then not dropping it before
the end of probe would defeat the point of having runtime PM.

> > > +static size_t sp7021_spi_max_length(struct spi_device *spi) {
> > > +	return SP7021_SPI_MSG_SIZE;
> > > +}

> > Is there any actual limit in the hardware?  This looks very much like it's a
> > completely artificial limit in the driver for no obvious reason.

>   The limit of the hardware is only 255 bytes . But more user need more than the limit.
> So I try to extend by software that is one reason use one message transfer and use CS-GPIO

As ever *please* use the core features rather than open coding - if you
specify a maximum transfer size the core already supports using a
software controllable chip select to handle messages with transfers of
arbatrary lengths.  There is no need for the driver to do anything here
other than providing a length, but that needs to be per transfer and not
per message.

In general if you're doing something that doesn't interact directly with
the hardware it shouldn't be in the driver, it's a pure software thing
which will also apply to any other similar hardware and should therefore
be done in the core.  

> > > +	pm_runtime_get_sync(pspim->dev);

> > To repeat the feedback from last time do not open code runtime PM, use the
> > core support.

> Only set set the auto_runtime_pm of should be struct * spi_controller in probe function  right ?

Yes.

> > > +	list_for_each_entry(xfer, &m->transfers, transfer_list) {
> > > +		if (!first_xfer)
> > > +			first_xfer = xfer;
> > > +		total_len +=  xfer->len;

> > To further repeat my prior feedback I can't see any reason why this driver
> > doesn't just use transfer_one().

> The FIFO only 16bytes for one action. I need push tx_data and pull rx_data as soon as possible.
> Use one message I can collect the data first and start transmitting 
> It is more efficient than transfer_one at real case.

That doesn't mean it's a good idea to just duplicate the core code, that
means that you should look into improving the performance of the core
code so any optimisation benefits everyone and the driver doesn't end
up with some combination of bugs, missing features and reduced
performance in the open coded version.  Having small FIFOs isn't at all
unusual so it seems unlikely that there's any need for anything here to
be device specific, and any benefits from copying all the data into a
linear buffer have got to be application specific, indeed it'll clearly
make things worse in some common cases.  For example with something like
flash where we have frequent use of large transfers for data payloads
the data is already mostly in large buffers the overhead of copying
those buffers is going to be very noticable compared to any saving,
especially given that there's only two transfers.  On the other end of
things when something like regmap is already linearising small writes
into single transfers then the copy is just pure overhead.

There's definitely scope for improving things here, the main one being
to pull advancing to the next transfer into spi_finalize_current_transfer()
which would benefit DMA as well, that don't introduce these overheads
and don't involve all this code duplication.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

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

On Thu, Nov 11, 2021 at 08:32:39AM +0000, Lh Kuo 郭力豪 wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> #define SPI_CS_HIGH   0x04 /* chipselect active high? */
> Is it mean?
> CASE1 : standby, CS high => start transfer CS become low => transfer end CS become high and standby
> CASE2 : standby, CS low => start transfer CS become high => transfer end CS become low and standby

> I think SPI_CS_HIGH means CASE2, But it is strange that more chipset work in CASE1 but drivers set SPI_CS_HIGH as defined.

SPI_CS_HIGH is case 2.

> 2. And in the CASE1 I should set 
> cs_gpios = 	<gpio 3 2 GPIO_ACTIVE_LOW>,
> or
> cs_gpios = 	<gpio 3 2 GPIO_ACTIVE_HIGH>,

_ACTIVE_HIGH if _CS_HIGH is specified, though the binding will try to
sort things out either way.

> 3. If I did not set the max_transfer_size of spi_control
> And use transfer_one set max_transfer_size and use_gpio_descriptors
> Can it transmit data that exceeds FIFO max bytes (even exceed HW's one-time transfer) in one transmission activity?

Yes, if you don't set a maximum transfer size the driver might get any
transfer size.  If you set a maximum transfer size then the driver will
not see any transfers that exceed the maximum transfer size.

> This is my concern, so I use Transfer_One_message

I can't understand how that would follow on.  If there's a limit on the
size of an individual transfer then tell the framework about that limit,
that's all that needs doing.  Why would it be preferable to not tell the
core about the limit and instead open code things?

*Please* think about the lengthy explanation I provided in my last
message about putting things that are not device specific in the
framework not the driver.

> Ex : Need to transmit 4000 bytes. 
>   Then I set Ctlr->transfer_one and use_gpio_descriptors
>     ctlr->max_transfer_size = 255;
>     The CS of device is low active

>    When the transmission starts, I can see the signal gpio-CS changes from high to low
> Ctlr->transfer_one will be triggered to execute 16 times, and transfer end gpio-CS changes from low to high.

This is exactly what will happen if you do as has been repeatedly
suggested.  Set a maximum *transfer* (not message) size, let the core
handle the chip select GPIO and implement transfer_one().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

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

On Wed, Nov 17, 2021 at 09:11:08AM +0000, Lh Kuo 郭力豪 wrote:

> The main function are as follows
> 
> The sp7021_spi_mas_transfer_one is replace the transfer_one_message function.
> 
> static int sp7021_spi_mas_transfer_one(struct spi_controller *ctlr,
> 		struct spi_device *spi, struct spi_transfer *xfer)
> {
> 	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
> 	u32 reg_temp = 0;
> 	unsigned long timeout = msecs_to_jiffies(1000);

I'm still not clear why this needs to be transfer_one_message() and not
just transfer_one()?  The whole thing with copying everything into a
buffer is a bit confusing to me.

> The probe function is as follows.
> 
> static int sp7021_spi_controller_probe(struct platform_device *pdev)
> {
> 	int ret;
> 	int mode;
> 	struct spi_controller *ctlr;
> 	struct sp7021_spi_ctlr *pspim;

This looks fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

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

On Fri, Nov 19, 2021 at 01:51:15AM +0000, Lh Kuo 郭力豪 wrote:

>    The driver made a lot of changes. Which function do you want to check first, or can i make a new patch ? And we can review on this basis.

It will be easiest to send a new patch.  The bits you included
here looked fine at first glance.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

[Randy Dunlap]

On 11/9/21 1:01 AM, LH.Kuo wrote:
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 596705d..30ce0ed 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -866,6 +866,17 @@ config SPI_SUN6I
>   	help
>   	  This enables using the SPI controller on the Allwinner A31 SoCs.
>   
> +config SPI_SUNPLUS_SP7021
> +	tristate "Sunplus SP7021 SPI controller"
> +	depends on SOC_SP7021
> +	help
> +	  This enable Sunplus SP7021 spi controller driver on the SP7021 SoCs.

	       enables the Sunplus SP021 SPI

> +	  This driver can also be built as a module. If so, the module will be
> +	  called as spi-sunplus-sp7021.
> +
> +	  If you have a  Sunplus SP7021 platform say Y here.

	         have a Sunplus
(i.e., drop one space)

> +	  If unsure, say N.


-- 
~Randy

Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

[Randy Dunlap]

On 11/9/21 9:39 PM, Lh Kuo 郭力豪 wrote:
> Hi
> 
>> -----Original Message-----
>> From: Randy Dunlap <rdunlap@infradead.org>
>> Sent: Wednesday, November 10, 2021 12:55 AM
>> To: LH.Kuo <lhjeff911@gmail.com>; p.zabel@pengutronix.de;
>> broonie@kernel.org; robh+dt@kernel.org; linux-spi@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: dvorkin@tibbo.com; qinjian@cqplus1.com; Wells Lu 呂芳騰
>> <wells.lu@sunplus.com>; Lh Kuo 郭力豪 <lh.Kuo@sunplus.com>
>> Subject: Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021
>>
>> On 11/9/21 1:01 AM, LH.Kuo wrote:
>>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
>>> 596705d..30ce0ed 100644
>>> --- a/drivers/spi/Kconfig
>>> +++ b/drivers/spi/Kconfig
>>> @@ -866,6 +866,17 @@ config SPI_SUN6I
>>>    	help
>>>    	  This enables using the SPI controller on the Allwinner A31 SoCs.
>>>
>>> +config SPI_SUNPLUS_SP7021
>>> +	tristate "Sunplus SP7021 SPI controller"
>>> +	depends on SOC_SP7021
>>> +	help
>>> +	  This enable Sunplus SP7021 spi controller driver on the SP7021 SoCs.
>>
>> 	       enables the Sunplus SP021 SPI
>>
>>> +	  This driver can also be built as a module. If so, the module will be
>>> +	  called as spi-sunplus-sp7021.
>>> +
>>> +	  If you have a  Sunplus SP7021 platform say Y here.
>>
>> 	         have a Sunplus
>> (i.e., drop one space)
>>
>>> +	  If unsure, say N.
>>
>>
> 
> I will make change as below  is it OK ?
> 
> config SPI_SUNPLUS_SP7021
> 	tristate "Sunplus SP7021 SPI controller"
> 	depends on SOC_SP7021
> 	help
> 	  This enable Sunplus SP7021 spi controller driver on the SP7021 SoCs.

	       enables               SPI

> 	  This driver can also be built as a module. If so, the module will be
> 	  called as spi-sunplus-sp7021.
> 
> 	  If you have a  Sunplus SP7021 platform say Y here.

drop one space:       a Sunplus

> 
> 	  If unsure, say N.
> 
>> --
>> ~Randy


-- 
~Randy

[PATCH v2 2/2] devicetree bindings SPI Add bindings doc for Sunplus SP7021

[LH.Kuo]

Add devicetree bindings SPI Add bindings doc for Sunplus SP7021

Signed-off-by: LH.Kuo <lh.kuo@sunplus.com>
---
 - Addressed all comments from Mr. Mark Brown
 - Addressed all comments from Mr. Philipp Zabel
 - Modified the structure and register access method.

 .../bindings/spi/spi-sunplus-sp7021.yaml           | 95 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 96 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml

diff --git a/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
new file mode 100644
index 0000000..5502f15
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd. 2021
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/spi-sunplus-sp7021.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sunplus sp7021 SPI controller
+
+allOf:
+  - $ref: "spi-controller.yaml"
+
+maintainers:
+  - lh.kuo <lh.kuo@sunplus.com>
+
+properties:
+  compatible:
+    enum:
+      - sunplus,sp7021-spi-controller
+
+  reg:
+    items:
+      - description: Base address and length of the SPI master registers
+      - description: Base address and length of the SPI slave registers
+
+  reg-names:
+    items:
+      - const: spi_master
+      - const: spi_slave
+
+  interrupt-names:
+    items:
+      - const: dma_w_intr
+      - const: mas_risc_intr
+      - const: slave_risc_intr
+
+  interrupts:
+    minItems: 3
+
+  clocks:
+    maxItems: 1
+
+  clocks-names:
+    items:
+      - const: sys_pll
+
+  resets:
+    maxItems: 1
+
+  pinctrl-names:
+    description:
+      A pinctrl state named "default" must be defined.
+    const: default
+
+  pinctrl-0:
+    description:
+      A phandle to the default pinctrl state.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clocks-names
+  - resets
+  - pinctrl-names
+  - pinctrl-0
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sp-sp7021.h>
+    #include <dt-bindings/reset/sp-sp7021.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi@9C002D80 {
+        compatible = "sunplus,sp7021-spi-controller";
+        reg = <0x9C002D80 0x80>, <0x9C002E00 0x80>;
+        reg-names = "spi_master", "spi_slave";
+        interrupt-parent = <&intc>;
+        interrupt-names = "dma_w_intr",
+                          "mas_risc_intr",
+                          "slave_risc_intr";
+        interrupts = <144 IRQ_TYPE_LEVEL_HIGH>,
+                     <146 IRQ_TYPE_LEVEL_HIGH>,
+                     <145 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clkc SPI_COMBO_0>;
+        clocks-names = "sys_pll";
+        resets = <&rstc RST_SPI_COMBO_0>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pins_spi0>;
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 34868d0..ef416a2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18193,6 +18193,7 @@ SUNPLUS SPI CONTROLLER INTERFACE DRIVER
 M:	LH Kuo <lh.kuo@sunplus.com>
 L:	linux-spi@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
 F:	drivers/spi/spi-sunplus-sp7021.c
 
 SUPERH
-- 
2.7.4

Re: [PATCH v2 2/2] devicetree bindings SPI Add bindings doc for Sunplus SP7021

[Mark Brown]

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

On Tue, Nov 09, 2021 at 05:01:28PM +0800, LH.Kuo wrote:
> Add devicetree bindings SPI Add bindings doc for Sunplus SP7021

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

> +  reg:
> +    items:
> +      - description: Base address and length of the SPI master registers
> +      - description: Base address and length of the SPI slave registers
> +
> +  reg-names:
> +    items:
> +      - const: spi_master
> +      - const: spi_slave

What exactly is the physical overlap between the two controllers - is it
just the pinmux?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] devicetree bindings SPI Add bindings doc for Sunplus SP7021

[Mark Brown]

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

On Wed, Nov 10, 2021 at 02:47:09AM +0000, Lh Kuo 郭力豪 wrote:

> > > +  reg-names:
> > > +    items:
> > > +      - const: spi_master
> > > +      - const: spi_slave

> > What exactly is the physical overlap between the two controllers - is it just the
> > pinmux?

> According to the designer not only pinmux, The power source and more design are overlap.

> That is why I hope set it in one driver.

The power source just sounds like a power domain which doesn't need a
shared driver, and shared elements in the design aren't really relevant
if they're not visible in the usage of the device which given the very
limited code sharing there seems to be in the driver doesn't seem to be
the case here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 0/2] Add SPI control driver for Sunplus SP7021 SoC

[LH.Kuo]

This is a patch series for SPI driver for Sunplus SP7021 SoC.

Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and
etc.) into a single chip. It is designed for industrial control.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

LH.Kuo (2):
  SPI: Add SPI driver for Sunplus SP7021
  devicetree: bindings SPI Add bindings doc for Sunplus SP7021

 .../bindings/spi/spi-sunplus-sp7021.yaml           |  95 +++
 MAINTAINERS                                        |   7 +
 drivers/spi/Kconfig                                |  11 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-sunplus-sp7021.c                   | 706 +++++++++++++++++++++
 5 files changed, 820 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
 create mode 100644 drivers/spi/spi-sunplus-sp7021.c

-- 
2.7.4

[PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[LH.Kuo]

Add SPI driver for Sunplus SP7021.

Signed-off-by: LH.Kuo <lh.kuo@sunplus.com>
---
Changes in v3:
 - Addressed all comments from Mr. Mark Brown
 - Addressed all comments from Mr. Philipp Zabel
 - Addressed all comments from Mr. Rob Herring
 - remove spi transfer_one_message

 MAINTAINERS                      |   6 +
 drivers/spi/Kconfig              |  11 +
 drivers/spi/Makefile             |   1 +
 drivers/spi/spi-sunplus-sp7021.c | 706 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 724 insertions(+)
 create mode 100644 drivers/spi/spi-sunplus-sp7021.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5250298..75fa7d5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18216,6 +18216,12 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS SPI CONTROLLER INTERFACE DRIVER
+M:	LH Kuo <lh.kuo@sunplus.com>
+L:	linux-spi@vger.kernel.org
+S:	Maintained
+F:	drivers/spi/spi-sunplus-sp7021.c
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 596705d..30ce0ed 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -866,6 +866,17 @@ config SPI_SUN6I
 	help
 	  This enables using the SPI controller on the Allwinner A31 SoCs.
 
+config SPI_SUNPLUS_SP7021
+	tristate "Sunplus SP7021 SPI controller"
+	depends on SOC_SP7021
+	help
+	  This enable Sunplus SP7021 spi controller driver on the SP7021 SoCs.
+	  This driver can also be built as a module. If so, the module will be
+	  called as spi-sunplus-sp7021.
+
+	  If you have a  Sunplus SP7021 platform say Y here.
+	  If unsure, say N.
+
 config SPI_SYNQUACER
 	tristate "Socionext's SynQuacer HighSpeed SPI controller"
 	depends on ARCH_SYNQUACER || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index dd7393a..b455eaf 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -119,6 +119,7 @@ obj-$(CONFIG_SPI_STM32_QSPI) 		+= spi-stm32-qspi.o
 obj-$(CONFIG_SPI_ST_SSC4)		+= spi-st-ssc4.o
 obj-$(CONFIG_SPI_SUN4I)			+= spi-sun4i.o
 obj-$(CONFIG_SPI_SUN6I)			+= spi-sun6i.o
+obj-$(CONFIG_SPI_SUNPLUS_SP7021)	+= spi-sunplus-sp7021.o
 obj-$(CONFIG_SPI_SYNQUACER)		+= spi-synquacer.o
 obj-$(CONFIG_SPI_TEGRA210_QUAD)		+= spi-tegra210-quad.o
 obj-$(CONFIG_SPI_TEGRA114)		+= spi-tegra114.o
diff --git a/drivers/spi/spi-sunplus-sp7021.c b/drivers/spi/spi-sunplus-sp7021.c
new file mode 100644
index 0000000..183834f
--- /dev/null
+++ b/drivers/spi/spi-sunplus-sp7021.c
@@ -0,0 +1,706 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (c) 2021 Sunplus Inc.
+// Author: LH Kuo <lh.kuo@sunplus.com>
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/delay.h>
+
+#define SLAVE_INT_IN
+
+#define SP7021_MAS_REG_NAME "spi_master"
+#define SP7021_SLA_REG_NAME "spi_slave"
+
+#define SP7021_MAS_IRQ_NAME "mas_risc_intr"
+#define SP7021_SLA_IRQ_NAME "slave_risc_intr"
+
+#define SP7021_SPI_DATA_SIZE (255)
+
+#define SP7021_CLR_MAS_INT (1<<6)
+
+#define SP7021_SLA_DMA_READ (0xd)
+#define SP7021_SLA_SW_RST (1<<1)
+#define SP7021_SLA_DMA_WRITE (0x4d)
+#define SP7021_SLA_DMA_W_INT (1<<8)
+#define SP7021_SLA_CLR_INT (1<<8)
+#define SP7021_SLA_DATA_RDY (1<<0)
+
+#define SP7021_CLR_MAS_W_INT (1<<7)
+
+#define SP7021_TOTAL_LENGTH(x) (x<<24)
+#define SP7021_TX_LENGTH(x) (x<<16)
+#define SP7021_GET_LEN(x)     ((x>>24)&0xFF)
+#define SP7021_GET_TX_LEN(x)  ((x>>16)&0xFF)
+#define SP7021_GET_RX_CNT(x)  ((x>>12)&0x0F)
+#define SP7021_GET_TX_CNT(x)  ((x>>8)&0x0F)
+
+#define SP7021_FINISH_FLAG (1<<6)
+#define SP7021_FINISH_FLAG_MASK (1<<15)
+#define SP7021_RX_FULL_FLAG (1<<5)
+#define SP7021_RX_FULL_FLAG_MASK (1<<14)
+#define SP7021_RX_EMP_FLAG (1<<4)
+#define SP7021_TX_EMP_FLAG (1<<2)
+#define SP7021_TX_EMP_FLAG_MASK (1<<11)
+#define SP7021_SPI_START_FD (1<<0)
+#define SP7021_FD_SEL (1<<6)
+#define SP7021_LSB_SEL (1<<4)
+#define SP7021_WRITE_BYTE(x) (x<<9)
+#define SP7021_READ_BYTE(x) (x<<7)
+#define SP7021_CLEAN_RW_BYTE (~0x780)
+#define SP7021_CLEAN_FLUG_MASK (~0xF800)
+
+#define SP7021_CPOL_FD (1<<0)
+#define SP7021_CPHA_R (1<<1)
+#define SP7021_CPHA_W (1<<2)
+#define SP7021_CS_POR (1<<5)
+
+#define SP7021_FD_SW_RST (1<<1)
+#define SP7021_FIFO_DATA_BITS (16*8)    // 16 BYTES
+#define SP7021_INT_BYPASS (1<<3)
+
+#define SP7021_FIFO_REG 0x0034
+#define SP7021_SPI_STATUS_REG 0x0038
+#define SP7021_SPI_CONFIG_REG 0x003c
+#define SP7021_INT_BUSY_REG 0x004c
+#define SP7021_DMA_CTRL_REG 0x0050
+
+#define SP7021_DATA_RDY_REG 0x0044
+#define SP7021_SLV_DMA_CTRL_REG 0x0048
+#define SP7021_SLV_DMA_LENGTH_REG 0x004c
+#define SP7021_SLV_DMA_ADDR_REG 0x004c
+
+enum SPI_MODE {
+	SP7021_SLA_READ = 0,
+	SP7021_SLA_WRITE = 1,
+	SP7021_SPI_IDLE = 2
+};
+
+enum {
+	SP7021_MASTER_MODE,
+	SP7021_SLAVE_MODE,
+};
+
+struct sp7021_spi_ctlr {
+
+	struct device *dev;
+	int mode;
+	struct spi_controller *ctlr;
+
+	void __iomem *mas_base;
+	void __iomem *sla_base;
+
+	u32 xfer_conf;
+
+	int mas_irq;
+	int sla_irq;
+
+	struct clk *spi_clk;
+	struct reset_control *rstc;
+
+	spinlock_t lock;
+	struct mutex buf_lock;
+
+	struct completion isr_done;
+	struct completion sla_isr;
+
+	unsigned int  rx_cur_len;
+	unsigned int  tx_cur_len;
+
+	const u8 *tx_buf;
+	u8 *rx_buf;
+
+	unsigned int  data_unit;
+};
+
+// spi slave irq handler
+static irqreturn_t sp7021_spi_sla_irq(int irq, void *dev)
+{
+	struct sp7021_spi_ctlr *pspim = dev;
+	unsigned int data_status;
+
+	data_status = readl(pspim->sla_base + SP7021_DATA_RDY_REG);
+	writel(data_status | SP7021_SLA_CLR_INT,
+			pspim->sla_base + SP7021_DATA_RDY_REG);
+
+	complete(&pspim->sla_isr);
+	return IRQ_NONE;
+}
+
+// slave only. usually called on driver remove
+static int sp7021_spi_sla_abort(struct spi_controller *ctlr)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	complete(&pspim->sla_isr);
+	complete(&pspim->isr_done);
+	return 0;
+}
+
+// slave R/W, called from S_transfer_one() only
+int sp7021_spi_sla_tx(struct spi_device *spi, struct spi_transfer *xfer)
+{
+	struct sp7021_spi_ctlr *pspim = spi_controller_get_devdata(spi->controller);
+	struct device *devp = &(spi->dev);
+	int err = 0;
+
+	mutex_lock(&pspim->buf_lock);
+
+	reinit_completion(&pspim->sla_isr);
+
+	writel_relaxed(SP7021_SLA_DMA_WRITE, pspim->sla_base + SP7021_SLV_DMA_CTRL_REG);
+	writel_relaxed(xfer->len, pspim->sla_base + SP7021_SLV_DMA_LENGTH_REG);
+	writel_relaxed(xfer->tx_dma, pspim->sla_base + SP7021_SLV_DMA_ADDR_REG);
+	writel(readl(pspim->sla_base + SP7021_DATA_RDY_REG) | SP7021_SLA_DATA_RDY,
+			pspim->sla_base + SP7021_DATA_RDY_REG);
+
+	if (wait_for_completion_interruptible(&pspim->sla_isr))
+		dev_err(devp, "%s() wait_for_completion timeout\n", __func__);
+
+	mutex_unlock(&pspim->buf_lock);
+	return err;
+}
+
+int sp7021_spi_sla_rx(struct spi_device *spi, struct spi_transfer *xfer)
+{
+	struct sp7021_spi_ctlr *pspim = spi_controller_get_devdata(spi->controller);
+	struct device *devp = &(spi->dev);
+	int err = 0;
+
+	mutex_lock(&pspim->buf_lock);
+
+	reinit_completion(&pspim->isr_done);
+	writel(SP7021_SLA_DMA_READ, pspim->sla_base + SP7021_SLV_DMA_CTRL_REG);
+	writel(xfer->len, pspim->sla_base + SP7021_SLV_DMA_LENGTH_REG);
+	writel(xfer->rx_dma, pspim->sla_base + SP7021_SLV_DMA_ADDR_REG);
+
+	// wait for DMA to complete
+	if (wait_for_completion_interruptible(&pspim->isr_done)) {
+		dev_err(devp, "%s() wait_for_completion timeout\n", __func__);
+		err = -ETIMEDOUT;
+		goto exit_spi_slave_rw;
+	}
+
+	writel(SP7021_SLA_SW_RST, pspim->sla_base + SP7021_SLV_DMA_CTRL_REG);
+
+exit_spi_slave_rw:
+	mutex_unlock(&pspim->buf_lock);
+	return err;
+
+}
+
+void sp7021_spi_mas_rb(struct sp7021_spi_ctlr *pspim, u8 len)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		pspim->rx_buf[pspim->rx_cur_len] =
+			readl(pspim->mas_base + SP7021_FIFO_REG);
+		pspim->rx_cur_len++;
+	}
+}
+
+void sp7021_spi_mas_wb(struct sp7021_spi_ctlr *pspim, u8 len)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		writel(pspim->tx_buf[pspim->tx_cur_len],
+			pspim->mas_base + SP7021_FIFO_REG);
+		pspim->tx_cur_len++;
+	}
+}
+
+static irqreturn_t sp7021_spi_mas_irq(int irq, void *dev)
+{
+	unsigned long flags;
+	struct sp7021_spi_ctlr *pspim = dev;
+	u32 fd_status = 0;
+	unsigned int tx_len, rx_cnt, tx_cnt, total_len;
+	bool isrdone = false;
+
+	fd_status = readl(pspim->mas_base + SP7021_SPI_STATUS_REG);
+	tx_cnt = SP7021_GET_TX_CNT(fd_status);
+	tx_len = SP7021_GET_TX_LEN(fd_status);
+	total_len = SP7021_GET_LEN(fd_status);
+
+	if ((fd_status & SP7021_TX_EMP_FLAG) &&
+		(fd_status & SP7021_RX_EMP_FLAG) && (total_len == 0))
+		return IRQ_NONE;
+
+	if ((tx_len == 0) && (total_len == 0))
+		return IRQ_NONE;
+
+	spin_lock_irqsave(&pspim->lock, flags);
+
+	rx_cnt = SP7021_GET_RX_CNT(fd_status);
+	// SP7021_RX_FULL_FLAG means RX buffer is full (16 bytes)
+	if (fd_status & SP7021_RX_FULL_FLAG)
+		rx_cnt = pspim->data_unit;
+
+	tx_cnt = min(tx_len - pspim->tx_cur_len, pspim->data_unit - tx_cnt);
+
+	dev_dbg(pspim->dev, "fd_st=0x%x rx_c:%d tx_c:%d tx_l:%d",
+			fd_status, rx_cnt, tx_cnt, tx_len);
+
+	if (rx_cnt > 0)
+		sp7021_spi_mas_rb(pspim, rx_cnt);
+	if (tx_cnt > 0)
+		sp7021_spi_mas_wb(pspim, tx_cnt);
+
+	fd_status = readl(pspim->mas_base + SP7021_SPI_STATUS_REG);
+
+	if ((fd_status & SP7021_FINISH_FLAG) ||
+		(SP7021_GET_TX_LEN(fd_status) == pspim->tx_cur_len)) {
+
+		while (SP7021_GET_LEN(fd_status) != pspim->rx_cur_len) {
+			fd_status = readl(pspim->mas_base + SP7021_SPI_STATUS_REG);
+			if (fd_status & SP7021_RX_FULL_FLAG)
+				rx_cnt = pspim->data_unit;
+			else
+				rx_cnt = SP7021_GET_RX_CNT(fd_status);
+
+			if (rx_cnt > 0)
+				sp7021_spi_mas_rb(pspim, rx_cnt);
+		}
+		writel(readl(pspim->mas_base + SP7021_INT_BUSY_REG)
+			| SP7021_CLR_MAS_INT, pspim->mas_base + SP7021_INT_BUSY_REG);
+		writel(SP7021_FINISH_FLAG, pspim->mas_base + SP7021_SPI_STATUS_REG);
+
+		isrdone = true;
+	}
+
+	if (isrdone)
+		complete(&pspim->isr_done);
+	spin_unlock_irqrestore(&pspim->lock, flags);
+	return IRQ_HANDLED;
+}
+
+// called in *controller_transfer_one*
+static void sp7021_prep_transfer(struct spi_controller *ctlr,
+			struct spi_device *spi)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	pspim->tx_cur_len = 0;
+	pspim->rx_cur_len = 0;
+	pspim->data_unit = SP7021_FIFO_DATA_BITS / spi->bits_per_word;
+}
+
+// called from *transfer* functions, set clock there
+static void sp7021_spi_setup_transfer(struct spi_device *spi,
+					struct spi_controller *ctlr, struct spi_transfer *xfer)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+	u32 rc = 0, rs = 0;
+	unsigned int clk_rate;
+	unsigned int div;
+	unsigned int clk_sel;
+
+	// set clock
+	clk_rate = clk_get_rate(pspim->spi_clk);
+	div = clk_rate / xfer->speed_hz;
+
+	clk_sel = (div / 2) - 1;
+	// set full duplex (bit 6) and fd freq (bits 31:16)
+	rc = SP7021_FD_SEL | (0xffff << 16);
+	rs = SP7021_FD_SEL | ((clk_sel & 0xffff) << 16);
+	writel((pspim->xfer_conf & ~rc) | rs, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+}
+
+// preliminary set CS, CPOL, CPHA and LSB
+static int sp7021_spi_controller_prepare_message(struct spi_controller *ctlr,
+				    struct spi_message *msg)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+	struct spi_device *s = msg->spi;
+	// reg clear bits and set bits
+	u32 rs = 0;
+
+	writel(readl(pspim->mas_base + SP7021_SPI_STATUS_REG) | SP7021_FD_SW_RST,
+					pspim->mas_base + SP7021_SPI_STATUS_REG);
+
+	//set up full duplex frequency and enable  full duplex
+	rs = SP7021_FD_SEL | ((0xffff) << 16);
+
+	if (s->mode & SPI_CPOL)
+		rs |= SP7021_CPOL_FD;
+
+	if (s->mode & SPI_LSB_FIRST)
+		rs |= SP7021_LSB_SEL;
+
+	if (s->mode & SPI_CS_HIGH)
+		rs |= SP7021_CS_POR;
+
+	if (s->mode & SPI_CPHA)
+		rs |=  SP7021_CPHA_R;
+	else
+		rs |=  SP7021_CPHA_W;
+
+	rs |= SP7021_WRITE_BYTE(0) | SP7021_READ_BYTE(0);  //set R/W fifo byte
+
+	pspim->xfer_conf = rs;
+
+	if (pspim->xfer_conf & SP7021_CPOL_FD)
+		writel(pspim->xfer_conf, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+
+	return 0;
+}
+
+static int sp7021_spi_mas_transfer_one(struct spi_controller *ctlr,
+		struct spi_device *spi, struct spi_transfer *xfer)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+	u32 reg_temp = 0;
+	unsigned long timeout = msecs_to_jiffies(1000);
+	unsigned int i;
+	int ret;
+	unsigned int xfer_cnt, xfer_len, last_len;
+
+	xfer_cnt = xfer->len / SP7021_SPI_DATA_SIZE;
+	last_len = xfer->len % SP7021_SPI_DATA_SIZE;
+
+	for (i = 0; i <= xfer_cnt; i++) {
+
+		mutex_lock(&pspim->buf_lock);
+
+		sp7021_prep_transfer(ctlr, spi);
+		sp7021_spi_setup_transfer(spi, ctlr, xfer);
+
+		reinit_completion(&pspim->isr_done);
+
+		if (i == xfer_cnt)
+			xfer_len = last_len;
+		else
+			xfer_len = SP7021_SPI_DATA_SIZE;
+
+		pspim->tx_buf = xfer->tx_buf+i*SP7021_SPI_DATA_SIZE;
+		pspim->rx_buf = xfer->rx_buf+i*SP7021_SPI_DATA_SIZE;
+
+		if (pspim->tx_cur_len < xfer_len) {
+			reg_temp = min(pspim->data_unit, xfer_len);
+			sp7021_spi_mas_wb(pspim, reg_temp);
+		}
+
+		// initial SPI master config and change to Full-Duplex mode 91.15
+		reg_temp = readl(pspim->mas_base + SP7021_SPI_CONFIG_REG);
+		reg_temp &= SP7021_CLEAN_RW_BYTE;
+		reg_temp &= SP7021_CLEAN_FLUG_MASK;
+		reg_temp |= SP7021_FD_SEL;
+		// set SPI master config for full duplex (SPI_FD_CONFIG)  91.15
+		reg_temp |= SP7021_FINISH_FLAG_MASK |
+				SP7021_TX_EMP_FLAG_MASK | SP7021_RX_FULL_FLAG_MASK;
+		reg_temp |= SP7021_WRITE_BYTE(0) | SP7021_READ_BYTE(0);
+		writel(reg_temp, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+
+		writel(SP7021_TOTAL_LENGTH(xfer_len) | SP7021_TX_LENGTH(xfer_len)
+			| SP7021_SPI_START_FD, pspim->mas_base + SP7021_SPI_STATUS_REG);
+
+		if (!wait_for_completion_interruptible_timeout(&pspim->isr_done,
+							       timeout)){
+			dev_err(&(spi->dev), "wait_for_completion err\n");
+			ret = -ETIMEDOUT;
+			goto free_maste_xfer;
+		}
+
+		reg_temp = readl(pspim->mas_base + SP7021_SPI_STATUS_REG);
+		if (reg_temp & SP7021_FINISH_FLAG) {
+			writel(SP7021_FINISH_FLAG, pspim->mas_base + SP7021_SPI_STATUS_REG);
+			writel(readl(pspim->mas_base + SP7021_SPI_CONFIG_REG) &
+				SP7021_CLEAN_FLUG_MASK, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+		}
+
+		ret = 0;
+
+		if (pspim->xfer_conf & SP7021_CPOL_FD)
+			writel(pspim->xfer_conf, pspim->mas_base + SP7021_SPI_CONFIG_REG);
+
+		mutex_unlock(&pspim->buf_lock);
+	}
+
+free_maste_xfer:
+	return ret;
+}
+
+// SPI-slave R/W
+static int sp7021_spi_sla_transfer_one(struct spi_controller *ctlr,
+		struct spi_device *spi, struct spi_transfer *xfer)
+{
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+	struct device *dev = pspim->dev;
+	int mode = SP7021_SPI_IDLE, ret = 0;
+
+	if (spi_controller_is_slave(ctlr)) {
+
+		if ((xfer->tx_buf) && (xfer->rx_buf)) {
+			dev_dbg(&ctlr->dev, "%s() wrong command\n", __func__);
+			ret = -EINVAL;
+		} else if (xfer->tx_buf) {
+			xfer->tx_dma = dma_map_single(dev, (void *)xfer->tx_buf,
+						xfer->len, DMA_TO_DEVICE);
+
+			if (dma_mapping_error(dev, xfer->tx_dma))
+				return -ENOMEM;
+
+			mode = SP7021_SLA_WRITE;
+		} else if (xfer->rx_buf) {
+			xfer->rx_dma = dma_map_single(dev, xfer->rx_buf,
+				xfer->len, DMA_FROM_DEVICE);
+
+			if (dma_mapping_error(dev, xfer->rx_dma))
+				return -ENOMEM;
+
+			mode = SP7021_SLA_READ;
+		}
+
+		switch (mode) {
+		case SP7021_SLA_WRITE:
+			sp7021_spi_sla_tx(spi, xfer);
+			break;
+		case SP7021_SLA_READ:
+			sp7021_spi_sla_rx(spi, xfer);
+			break;
+		default:
+			break;
+		}
+
+		if (xfer->tx_buf)
+			dma_unmap_single(dev, xfer->tx_dma,
+				xfer->len, DMA_TO_DEVICE);
+
+		if (xfer->rx_buf)
+			dma_unmap_single(dev, xfer->rx_dma,
+				xfer->len, DMA_FROM_DEVICE);
+
+	}
+
+	spi_finalize_current_transfer(ctlr);
+
+	return ret;
+
+}
+
+static int sp7021_spi_controller_probe(struct platform_device *pdev)
+{
+	int ret;
+	int mode;
+	struct spi_controller *ctlr;
+	struct sp7021_spi_ctlr *pspim;
+
+	pdev->id = 0;
+	mode = SP7021_MASTER_MODE;
+	if (pdev->dev.of_node) {
+		pdev->id = of_alias_get_id(pdev->dev.of_node, "sp_spi");
+		if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
+			mode = SP7021_SLAVE_MODE;
+	}
+	dev_dbg(&pdev->dev, "pdev->id = %d\n", pdev->id);
+
+	if (mode == SP7021_SLAVE_MODE)
+		ctlr = devm_spi_alloc_slave(&pdev->dev, sizeof(*pspim));
+	else
+		ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(*pspim));
+	if (!ctlr)
+		return -ENOMEM;
+
+	ctlr->dev.of_node = pdev->dev.of_node;
+	ctlr->bus_num = pdev->id;
+	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
+	ctlr->auto_runtime_pm = true;
+	ctlr->prepare_message = sp7021_spi_controller_prepare_message;
+
+	if (mode == SP7021_SLAVE_MODE) {
+		ctlr->transfer_one = sp7021_spi_sla_transfer_one;
+		ctlr->slave_abort = sp7021_spi_sla_abort;
+		ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX;
+	} else {
+		ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
+		ctlr->min_speed_hz = 40000;
+		ctlr->max_speed_hz = 25000000;
+		ctlr->use_gpio_descriptors = true;
+		ctlr->flags = SPI_CONTROLLER_MUST_RX | SPI_CONTROLLER_MUST_TX;
+		ctlr->transfer_one = sp7021_spi_mas_transfer_one;
+	}
+
+	platform_set_drvdata(pdev, ctlr);
+	pspim = spi_controller_get_devdata(ctlr);
+
+	pspim->ctlr = ctlr;
+	pspim->dev = &pdev->dev;
+
+	spin_lock_init(&pspim->lock);
+	mutex_init(&pspim->buf_lock);
+	init_completion(&pspim->isr_done);
+	init_completion(&pspim->sla_isr);
+
+	pspim->mas_base = devm_platform_ioremap_resource_byname
+		(pdev, SP7021_MAS_REG_NAME);
+	pspim->sla_base = devm_platform_ioremap_resource_byname
+		(pdev, SP7021_SLA_REG_NAME);
+
+	dev_dbg(&pdev->dev, "mas_base 0x%x\n", (unsigned int)pspim->mas_base);
+
+	pspim->mas_irq = platform_get_irq_byname(pdev, SP7021_MAS_IRQ_NAME);
+	if (pspim->mas_irq < 0) {
+		dev_err(&pdev->dev, "failed to get %s\n", SP7021_MAS_IRQ_NAME);
+		return pspim->mas_irq;
+	}
+
+	pspim->sla_irq = platform_get_irq_byname(pdev, SP7021_SLA_IRQ_NAME);
+	if (pspim->sla_irq < 0) {
+		dev_err(&pdev->dev, "failed to get %s\n", SP7021_SLA_IRQ_NAME);
+		return pspim->sla_irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, pspim->mas_irq, sp7021_spi_mas_irq
+						, IRQF_TRIGGER_RISING, pdev->name, pspim);
+	if (ret) {
+		dev_err(&pdev->dev, "%s devm_request_irq fail\n", SP7021_MAS_IRQ_NAME);
+		return ret;
+	}
+
+	ret = devm_request_irq(&pdev->dev, pspim->sla_irq, sp7021_spi_sla_irq
+						, IRQF_TRIGGER_RISING, pdev->name, pspim);
+	if (ret) {
+		dev_err(&pdev->dev, "%s devm_request_irq fail\n", SP7021_SLA_IRQ_NAME);
+		return ret;
+	}
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = devm_spi_register_controller(&pdev->dev, ctlr);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "spi_register_master fail\n");
+		goto disable_runtime_pm;
+	}
+
+	// clk
+	pspim->spi_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pspim->spi_clk)) {
+		return dev_err_probe(&pdev->dev, PTR_ERR(pspim->spi_clk),
+				     "devm_clk_get fail\n");
+	}
+
+	// reset
+	pspim->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	dev_dbg(&pdev->dev, "pspim->rstc : 0x%x\n", (unsigned int)pspim->rstc);
+	if (IS_ERR(pspim->rstc)) {
+		return dev_err_probe(&pdev->dev, PTR_ERR(pspim->rstc),
+				     "devm_rst_get fail\n");
+	}
+
+	ret = clk_prepare_enable(pspim->spi_clk);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+			"failed to enable clk\n");
+
+	ret = reset_control_deassert(pspim->rstc);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret,
+			"failed to deassert reset\n");
+		goto free_reset_assert;
+
+	}
+
+	dev_dbg(&pdev->dev, "pm init done\n");
+
+	return ret;
+
+free_reset_assert:
+	reset_control_assert(pspim->rstc);
+	clk_disable_unprepare(pspim->spi_clk);
+disable_runtime_pm:
+	pm_runtime_disable(&pdev->dev);
+
+	dev_dbg(&pdev->dev, "spi_master_probe done\n");
+	return ret;
+}
+
+static int sp7021_spi_controller_remove(struct platform_device *pdev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+
+	spi_unregister_controller(pspim->ctlr);
+	clk_disable_unprepare(pspim->spi_clk);
+	reset_control_assert(pspim->rstc);
+
+	return 0;
+}
+
+static int __maybe_unused sp7021_spi_controller_suspend(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	return reset_control_assert(pspim->rstc);
+}
+
+static int __maybe_unused sp7021_spi_controller_resume(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	reset_control_deassert(pspim->rstc);
+
+	return clk_prepare_enable(pspim->spi_clk);
+}
+
+static int sp7021_spi_runtime_suspend(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	dev_dbg(dev, "devid:%d\n", dev->id);
+	return reset_control_assert(pspim->rstc);
+}
+
+static int sp7021_spi_runtime_resume(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
+
+	dev_dbg(dev, "devid:%d\n", dev->id);
+	return reset_control_deassert(pspim->rstc);
+}
+
+static const struct dev_pm_ops sp7021_spi_pm_ops = {
+	SET_RUNTIME_PM_OPS(sp7021_spi_runtime_suspend,
+				sp7021_spi_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(sp7021_spi_controller_suspend,
+				sp7021_spi_controller_resume)
+};
+
+static const struct of_device_id sp7021_spi_controller_ids[] = {
+	{ .compatible = "sunplus,sp7021-spi-controller" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sp7021_spi_controller_ids);
+
+static struct platform_driver sp7021_spi_controller_driver = {
+	.probe = sp7021_spi_controller_probe,
+	.remove = sp7021_spi_controller_remove,
+	.driver = {
+		.name = "sunplus,sp7021-spi-controller",
+		.of_match_table = sp7021_spi_controller_ids,
+		.pm     = &sp7021_spi_pm_ops,
+	},
+};
+module_platform_driver(sp7021_spi_controller_driver);
+
+MODULE_AUTHOR("lH Kuo <lh.kuo@sunplus.com>");
+MODULE_DESCRIPTION("Sunplus SPI controller driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Andy Shevchenko]

On Mon, Nov 22, 2021 at 4:34 AM LH.Kuo <lhjeff911@gmail.com> wrote:
>
> Add SPI driver for Sunplus SP7021.

Much better, but needs more work to be good enough, see my comments below.

...

> +config SPI_SUNPLUS_SP7021
> +       tristate "Sunplus SP7021 SPI controller"
> +       depends on SOC_SP7021
> +       help
> +         This enable Sunplus SP7021 spi controller driver on the SP7021 SoCs.

enables
SPI

> +         This driver can also be built as a module. If so, the module will be
> +         called as spi-sunplus-sp7021.
> +
> +         If you have a  Sunplus SP7021 platform say Y here.
> +         If unsure, say N.

...

> +// SPDX-License-Identifier: GPL-2.0-only

> +//

Do you need this line?

> +// Copyright (c) 2021 Sunplus Inc.
> +// Author: LH Kuo <lh.kuo@sunplus.com>

...

> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/spi/spi.h>
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/delay.h>

Sort them, please.

...

> +#define SLAVE_INT_IN

What's this?

...

> +#define SP7021_MAS_REG_NAME "spi_master"
> +#define SP7021_SLA_REG_NAME "spi_slave"
> +
> +#define SP7021_MAS_IRQ_NAME "mas_risc_intr"
> +#define SP7021_SLA_IRQ_NAME "slave_risc_intr"

Why do you need these?

...

> +#define SP7021_CLR_MAS_INT (1<<6)

Make use of BIT() and GENMASK() here and below.

> +#define SP7021_SLA_DMA_READ (0xd)
> +#define SP7021_SLA_SW_RST (1<<1)
> +#define SP7021_SLA_DMA_WRITE (0x4d)
> +#define SP7021_SLA_DMA_W_INT (1<<8)
> +#define SP7021_SLA_CLR_INT (1<<8)
> +#define SP7021_SLA_DATA_RDY (1<<0)

This is a mess. Make sure they are sorted by the value.
Also it's visible that bit 6 defines READ vs. WRITE (at least for DMA).

> +#define SP7021_CLR_MAS_W_INT (1<<7)
> +
> +#define SP7021_TOTAL_LENGTH(x) (x<<24)
> +#define SP7021_TX_LENGTH(x) (x<<16)
> +#define SP7021_GET_LEN(x)     ((x>>24)&0xFF)
> +#define SP7021_GET_TX_LEN(x)  ((x>>16)&0xFF)
> +#define SP7021_GET_RX_CNT(x)  ((x>>12)&0x0F)
> +#define SP7021_GET_TX_CNT(x)  ((x>>8)&0x0F)
> +
> +#define SP7021_FINISH_FLAG (1<<6)
> +#define SP7021_FINISH_FLAG_MASK (1<<15)
> +#define SP7021_RX_FULL_FLAG (1<<5)
> +#define SP7021_RX_FULL_FLAG_MASK (1<<14)
> +#define SP7021_RX_EMP_FLAG (1<<4)
> +#define SP7021_TX_EMP_FLAG (1<<2)
> +#define SP7021_TX_EMP_FLAG_MASK (1<<11)
> +#define SP7021_SPI_START_FD (1<<0)
> +#define SP7021_FD_SEL (1<<6)
> +#define SP7021_LSB_SEL (1<<4)
> +#define SP7021_WRITE_BYTE(x) (x<<9)
> +#define SP7021_READ_BYTE(x) (x<<7)
> +#define SP7021_CLEAN_RW_BYTE (~0x780)
> +#define SP7021_CLEAN_FLUG_MASK (~0xF800)
> +
> +#define SP7021_CPOL_FD (1<<0)
> +#define SP7021_CPHA_R (1<<1)
> +#define SP7021_CPHA_W (1<<2)
> +#define SP7021_CS_POR (1<<5)
> +
> +#define SP7021_FD_SW_RST (1<<1)
> +#define SP7021_FIFO_DATA_BITS (16*8)    // 16 BYTES
> +#define SP7021_INT_BYPASS (1<<3)
> +
> +#define SP7021_FIFO_REG 0x0034
> +#define SP7021_SPI_STATUS_REG 0x0038
> +#define SP7021_SPI_CONFIG_REG 0x003c
> +#define SP7021_INT_BUSY_REG 0x004c
> +#define SP7021_DMA_CTRL_REG 0x0050
> +
> +#define SP7021_DATA_RDY_REG 0x0044
> +#define SP7021_SLV_DMA_CTRL_REG 0x0048
> +#define SP7021_SLV_DMA_LENGTH_REG 0x004c
> +#define SP7021_SLV_DMA_ADDR_REG 0x004c

...

> +enum SPI_MODE {

Besides unneeded names, which may collide with generic definitions...

> +       SP7021_SLA_READ = 0,
> +       SP7021_SLA_WRITE = 1,
> +       SP7021_SPI_IDLE = 2

...add a comma here, since it doesn't look like a terminator.

> +};

...

> +enum {
> +       SP7021_MASTER_MODE,
> +       SP7021_SLAVE_MODE,
> +};

Is it related to hardware? Then assign proper values explicitly.

...

> +struct sp7021_spi_ctlr {

> +

Redundant blank line.

> +       struct device *dev;
> +       int mode;
> +       struct spi_controller *ctlr;

> +       void __iomem *mas_base;
> +       void __iomem *sla_base;

Why do you need this to be separated?

> +       u32 xfer_conf;

> +       int mas_irq;
> +       int sla_irq;

Ditto.

> +       struct clk *spi_clk;
> +       struct reset_control *rstc;
> +
> +       spinlock_t lock;
> +       struct mutex buf_lock;
> +
> +       struct completion isr_done;
> +       struct completion sla_isr;

Ditto.

> +       unsigned int  rx_cur_len;
> +       unsigned int  tx_cur_len;
> +
> +       const u8 *tx_buf;
> +       u8 *rx_buf;
> +
> +       unsigned int  data_unit;
> +};

...

> +// spi slave irq handler

Useless comments.

...

> +// slave only. usually called on driver remove

Why is it so?
Also find use of proper English grammar (capitalization, periods, etc.
Ditto for all your comments.

...

> +// slave R/W, called from S_transfer_one() only

Ditto here and for all similar comments. If you point out that
something is called from something either explain why or drop useless
comments since anybody can see what function called from which
function (even indirectly).

...

> +int sp7021_spi_sla_tx(struct spi_device *spi, struct spi_transfer *xfer)
> +{
> +       struct sp7021_spi_ctlr *pspim = spi_controller_get_devdata(spi->controller);

> +       struct device *devp = &(spi->dev);

Here and everywhere else, first of all we are using dev for struct
device pointers, second there are too many parentheses.

> +       int err = 0;

What's the use? See below...

> +       mutex_lock(&pspim->buf_lock);
> +
> +       reinit_completion(&pspim->sla_isr);
> +
> +       writel_relaxed(SP7021_SLA_DMA_WRITE, pspim->sla_base + SP7021_SLV_DMA_CTRL_REG);
> +       writel_relaxed(xfer->len, pspim->sla_base + SP7021_SLV_DMA_LENGTH_REG);
> +       writel_relaxed(xfer->tx_dma, pspim->sla_base + SP7021_SLV_DMA_ADDR_REG);
> +       writel(readl(pspim->sla_base + SP7021_DATA_RDY_REG) | SP7021_SLA_DATA_RDY,
> +                       pspim->sla_base + SP7021_DATA_RDY_REG);

> +       if (wait_for_completion_interruptible(&pspim->sla_isr))
> +               dev_err(devp, "%s() wait_for_completion timeout\n", __func__);

...seems you missed to assign proper error code.

> +       mutex_unlock(&pspim->buf_lock);
> +       return err;
> +}

...

> +exit_spi_slave_rw:

Make names of goto labels meaningful, what does above mean? What it
should mean is what will be done when goto to it, i.e. out_unlock: in
this case.

> +       mutex_unlock(&pspim->buf_lock);
> +       return err;

> +

You need to clean up your code before submission.
So, let's see -50 LOCs next time, I see it's achievable.

> +}

...

> +void sp7021_spi_mas_rb(struct sp7021_spi_ctlr *pspim, u8 len)
> +{
> +       int i;
> +
> +       for (i = 0; i < len; i++) {
> +               pspim->rx_buf[pspim->rx_cur_len] =
> +                       readl(pspim->mas_base + SP7021_FIFO_REG);
> +               pspim->rx_cur_len++;
> +       }
> +}
> +
> +void sp7021_spi_mas_wb(struct sp7021_spi_ctlr *pspim, u8 len)
> +{
> +       int i;
> +
> +       for (i = 0; i < len; i++) {
> +               writel(pspim->tx_buf[pspim->tx_cur_len],
> +                       pspim->mas_base + SP7021_FIFO_REG);
> +               pspim->tx_cur_len++;
> +       }
> +}

Are these NIH of readsl() / writesl()?

...

> +       unsigned long flags;
> +       struct sp7021_spi_ctlr *pspim = dev;
> +       u32 fd_status = 0;
> +       unsigned int tx_len, rx_cnt, tx_cnt, total_len;
> +       bool isrdone = false;

Reversed xmas tree order here and everywhere else.

...

> +               writel(readl(pspim->mas_base + SP7021_INT_BUSY_REG)
> +                       | SP7021_CLR_MAS_INT, pspim->mas_base + SP7021_INT_BUSY_REG);

Use a temporary variable instead of this mess.

...

> +static void sp7021_prep_transfer(struct spi_controller *ctlr,
> +                       struct spi_device *spi)

One line?

...

> +       // set clock
> +       clk_rate = clk_get_rate(pspim->spi_clk);
> +       div = clk_rate / xfer->speed_hz;
> +
> +       clk_sel = (div / 2) - 1;

When div == 0 is it okay to have this value for clk_sel?

...

> +       // set full duplex (bit 6) and fd freq (bits 31:16)

Useless, and use GENMASK()

> +       rc = SP7021_FD_SEL | (0xffff << 16);
> +       rs = SP7021_FD_SEL | ((clk_sel & 0xffff) << 16);

What' the point of having SP7021_FD_SEL in rc and rs simultaneously?

> +       writel((pspim->xfer_conf & ~rc) | rs, pspim->mas_base + SP7021_SPI_CONFIG_REG);

...

> +       writel(readl(pspim->mas_base + SP7021_SPI_STATUS_REG) | SP7021_FD_SW_RST,
> +                                       pspim->mas_base + SP7021_SPI_STATUS_REG);

Introduce proper IO accessors as other drivers do.

> +       //set up full duplex frequency and enable  full duplex
> +       rs = SP7021_FD_SEL | ((0xffff) << 16);

Seems like déjà-vu to me. Perhaps it makes sense to have a dedicated definition.

...

> +       unsigned long timeout = msecs_to_jiffies(1000);
> +       unsigned int i;
> +       int ret;
> +       unsigned int xfer_cnt, xfer_len, last_len;

...

> +       for (i = 0; i <= xfer_cnt; i++) {

> +

Redundant. As I said you have a lot of this kind of blank lines sparse
over the code.

> +               mutex_lock(&pspim->buf_lock);
> +
> +               sp7021_prep_transfer(ctlr, spi);
> +               sp7021_spi_setup_transfer(spi, ctlr, xfer);
> +
> +               reinit_completion(&pspim->isr_done);
> +
> +               if (i == xfer_cnt)
> +                       xfer_len = last_len;
> +               else
> +                       xfer_len = SP7021_SPI_DATA_SIZE;

If xfer_len == 0 does it make any sense to go via the entire loop?

...

> +               if (!wait_for_completion_interruptible_timeout(&pspim->isr_done,
> +                                                              timeout)){

One line? Also check wrong spacing.

...

> +free_maste_xfer:
> +       return ret;

Useless label. You may return directly. Actually the entire function
needs a bit of care.

...

> +                       dma_unmap_single(dev, xfer->tx_dma,
> +                               xfer->len, DMA_TO_DEVICE);

One line

...

> +                       dma_unmap_single(dev, xfer->rx_dma,
> +                               xfer->len, DMA_FROM_DEVICE);

Ditto.

...

> +       pdev->id = 0;

Why?

...

> +       if (pdev->dev.of_node) {
> +               pdev->id = of_alias_get_id(pdev->dev.of_node, "sp_spi");

Ditto.

...

> +               if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
> +                       mode = SP7021_SLAVE_MODE;

There is no need to check of_node for this call.

...

> +       dev_dbg(&pdev->dev, "pdev->id = %d\n", pdev->id);

Useless.

...

> +       ctlr->dev.of_node = pdev->dev.of_node;

Use device_set_node().

...

> +       pspim->mas_base = devm_platform_ioremap_resource_byname
> +               (pdev, SP7021_MAS_REG_NAME);
> +       pspim->sla_base = devm_platform_ioremap_resource_byname
> +               (pdev, SP7021_SLA_REG_NAME);

Something is wrong with the indentation.

...

> +       dev_dbg(&pdev->dev, "mas_base 0x%x\n", (unsigned int)pspim->mas_base);

Redundant.

...

> +       pspim->mas_irq = platform_get_irq_byname(pdev, SP7021_MAS_IRQ_NAME);
> +       if (pspim->mas_irq < 0) {

> +               dev_err(&pdev->dev, "failed to get %s\n", SP7021_MAS_IRQ_NAME);

Duplicate message printing.

> +               return pspim->mas_irq;
> +       }
> +
> +       pspim->sla_irq = platform_get_irq_byname(pdev, SP7021_SLA_IRQ_NAME);
> +       if (pspim->sla_irq < 0) {

> +               dev_err(&pdev->dev, "failed to get %s\n", SP7021_SLA_IRQ_NAME);

Ditto.

> +               return pspim->sla_irq;
> +       }

...

> +       // clk

Meaningless.

...

> +       dev_dbg(&pdev->dev, "pspim->rstc : 0x%x\n", (unsigned int)pspim->rstc);

Get rid of the debugging like this, it's not for production use at all.

...

> +               return dev_err_probe(&pdev->dev, PTR_ERR(pspim->rstc),
> +                                    "devm_rst_get fail\n");

One line.

...

> +               return dev_err_probe(&pdev->dev, ret,
> +                       "failed to enable clk\n");

Ditto. And so on...
To make lines shorter, utilize a temporary variable for struct device *dev.

...

> +       dev_dbg(&pdev->dev, "pm init done\n");

Redundant.

> +       dev_dbg(&pdev->dev, "spi_master_probe done\n");

Redundant.

...

> +       dev_dbg(dev, "devid:%d\n", dev->id);

Redundant.

...

> +       dev_dbg(dev, "devid:%d\n", dev->id);

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

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

On Tue, Nov 23, 2021 at 12:09:54AM +0200, Andy Shevchenko wrote:
> On Mon, Nov 22, 2021 at 4:34 AM LH.Kuo <lhjeff911@gmail.com> wrote:

> > +// slave only. usually called on driver remove

> Why is it so?
> Also find use of proper English grammar (capitalization, periods, etc.
> Ditto for all your comments.

Please don't go overboard on changes you're requesting, the important
thing with comments is that they're intelligible.  People have different
levels of skill with English and that's totally fine, it's much better
that people feel able to write comments than that they stop doing so
because they are concerned about issues with their foreign language
skills.  

> > +       unsigned long flags;
> > +       struct sp7021_spi_ctlr *pspim = dev;
> > +       u32 fd_status = 0;
> > +       unsigned int tx_len, rx_cnt, tx_cnt, total_len;
> > +       bool isrdone = false;

> Reversed xmas tree order here and everywhere else.

Again, please don't go overboard - this isn't a general requirement for
kernel code, some parts of the kernel do want it but outside of those
areas it's not something that we should be asking for (and TBH even when
it is required you should explain what it is, it's not as easy to search
for as it could be).  I certainly don't care.

> > +               if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
> > +                       mode = SP7021_SLAVE_MODE;

> There is no need to check of_node for this call.

OTOH if we are checking it anyway it doesn't hurt to have all the
property reads in the check for of_node.  Either way it doesn't
fundamentally matter.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Andy Shevchenko]

On Tue, Nov 23, 2021 at 4:03 PM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Nov 23, 2021 at 12:09:54AM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 22, 2021 at 4:34 AM LH.Kuo <lhjeff911@gmail.com> wrote:
>
> > > +// slave only. usually called on driver remove
>
> > Why is it so?
> > Also find use of proper English grammar (capitalization, periods, etc.
> > Ditto for all your comments.
>
> Please don't go overboard on changes you're requesting, the important
> thing with comments is that they're intelligible.  People have different
> levels of skill with English and that's totally fine, it's much better
> that people feel able to write comments than that they stop doing so
> because they are concerned about issues with their foreign language
> skills.

Shame on me! I'm also bad at English (okay, sometimes).

...

> > > +       unsigned long flags;
> > > +       struct sp7021_spi_ctlr *pspim = dev;
> > > +       u32 fd_status = 0;
> > > +       unsigned int tx_len, rx_cnt, tx_cnt, total_len;
> > > +       bool isrdone = false;
>
> > Reversed xmas tree order here and everywhere else.
>
> Again, please don't go overboard - this isn't a general requirement for
> kernel code, some parts of the kernel do want it but outside of those
> areas it's not something that we should be asking for (and TBH even when
> it is required you should explain what it is, it's not as easy to search
> for as it could be).  I certainly don't care.

Here it is. The "reversed xmas tree order" implies that longer lines
in the definition block, where one puts all variables that are being
used locally in the given function, are going first followed by
shorter ones. This makes it a bit easier to read the code. There are
also additional rules that may be applied, such as defines with
assignments _usually_ go before anything else, error code variable
separately and last. That said, the above might look better in the
following form:

       struct sp7021_spi_ctlr *pspim = dev;
       unsigned int tx_len, total_len;
       unsigned int rx_cnt, tx_cnt;
       unsigned long flags;
       bool isrdone = false;
       u32 fd_status = 0;

...

> > > +               if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
> > > +                       mode = SP7021_SLAVE_MODE;
>
> > There is no need to check of_node for this call.
>
> OTOH if we are checking it anyway it doesn't hurt to have all the
> property reads in the check for of_node.

I assumed that previous comment against SPI ID potentially will be
addressed by removing that code which in the result gives unnecessary
use of the of_node check above. So that's why I pointed this out.

>  Either way it doesn't
> fundamentally matter.

True!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Andy Shevchenko]

On Thu, Nov 25, 2021 at 11:47 AM Lh Kuo 郭力豪 <lh.Kuo@sunplus.com> wrote:

>     I have already modified most of the driver. And the probe function is as follows. Is it okay?

Can be done a bit better, see below (seems you missed few of the comments)

> static int sp7021_spi_controller_probe(struct platform_device *pdev)
> {
>         int ret;
>         int mode;
>         struct spi_controller *ctlr;
>         struct sp7021_spi_ctlr *pspim;
>         struct device *dev = &pdev->dev;

Other way around, or i.o.w. "reversed tree".

>         mode = SP7021_MASTER_MODE;
>         pdev->id = of_alias_get_id(pdev->dev.of_node, "sp_spi");
>
>         if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
>                 mode = SP7021_SLAVE_MODE;

         pdev->id = of_alias_get_id(pdev->dev.of_node, "sp_spi");

         mode = SP7021_MASTER_MODE;
         if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
                 mode = SP7021_SLAVE_MODE;

>         if (mode == SP7021_SLAVE_MODE)
>                 ctlr = devm_spi_alloc_slave(dev, sizeof(*pspim));
>         else
>                 ctlr = devm_spi_alloc_master(dev, sizeof(*pspim));
>         if (!ctlr)
>                 return -ENOMEM;
>
>         ctlr->dev.of_node = pdev->dev.of_node;
>         ctlr->bus_num = pdev->id;
>         ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
>         ctlr->auto_runtime_pm = true;
>         ctlr->prepare_message = sp7021_spi_controller_prepare_message;
>         if (mode == SP7021_SLAVE_MODE) {
>                 ctlr->transfer_one = sp7021_spi_sla_transfer_one;
>                 ctlr->slave_abort = sp7021_spi_sla_abort;
>                 ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX;
>         } else {
>                 ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
>                 ctlr->min_speed_hz = 40000;
>                 ctlr->max_speed_hz = 25000000;
>                 ctlr->use_gpio_descriptors = true;
>                 ctlr->flags = SPI_CONTROLLER_MUST_RX | SPI_CONTROLLER_MUST_TX;
>                 ctlr->transfer_one = sp7021_spi_mas_transfer_one;
>         }
>         platform_set_drvdata(pdev, ctlr);
>         pspim = spi_controller_get_devdata(ctlr);
>         pspim->ctlr = ctlr;
>         pspim->dev = dev;
>         spin_lock_init(&pspim->lock);
>         mutex_init(&pspim->buf_lock);
>         init_completion(&pspim->isr_done);
>         init_completion(&pspim->sla_isr);
>         pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, "spi_master");
>         pspim->sla_base = devm_platform_ioremap_resource_byname(pdev, "spi_slave");
>
>         pspim->mas_irq = platform_get_irq_byname(pdev, "mas_risc_intr");
>         if (pspim->mas_irq < 0) {

>                 dev_err(dev, "failed to get mas intr\n");

Dup. No need to repeat what's done by platform core.

>                 return pspim->mas_irq;
>         }
>
>         pspim->sla_irq = platform_get_irq_byname(pdev, "slave_risc_intr");
>         if (pspim->sla_irq < 0) {

>                 dev_err(dev, "failed to get sla intr\n");

Dup.

>                 return pspim->sla_irq;
>         }
>
>         ret = devm_request_irq(dev, pspim->mas_irq, sp7021_spi_mas_irq
>                                                 , IRQF_TRIGGER_RISING, pdev->name, pspim);

Ugly indentation.

>         if (ret) {
>                 dev_err(dev, "mas intr devm_request_irq fail\n");
>                 return ret;
>         }
>
>         ret = devm_request_irq(dev, pspim->sla_irq, sp7021_spi_sla_irq
>                                                 , IRQF_TRIGGER_RISING, pdev->name, pspim);

Ditto.

>         if (ret) {
>                 dev_err(dev, "slave intr devm_request_irq fail\n");
>                 return ret;
>         }
>
>         pspim->spi_clk = devm_clk_get(dev, NULL);

>         if (IS_ERR(pspim->spi_clk)) {
>                 return dev_err_probe(dev, PTR_ERR(pspim->spi_clk), "clk get fail\n");
>         }

Does checkpatch compains on this?
Hint: {} around a single statement shouldn't be added.

>         pspim->rstc = devm_reset_control_get_exclusive(dev, NULL);
>         if (IS_ERR(pspim->rstc)) {
>                 return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst get fail\n");

Ditto.

>         }
>
>         ret = clk_prepare_enable(pspim->spi_clk);
>         if (ret)

>                 return dev_err_probe(dev, ret,
>                         "failed to enable clk\n");

One line

>         ret = reset_control_deassert(pspim->rstc);
>         if (ret) {
>                 dev_err_probe(dev, ret,
>                         "failed to deassert reset\n");

One line.

>                 goto free_reset_assert;

>

Really, pay attention to a stray blank line here and there.

>         }
>
>         pm_runtime_enable(dev);
>
>         ret = devm_spi_register_controller(dev, ctlr);

You can't mix non-devm with devm APIs. Either all non-devm, or devm
followed by non-devm.

>         if (ret) {
>                 dev_err(dev, "spi_register_master fail\n");
>                 goto err_disable_pm_runtime;
>         }
>
>         return ret;
>
> err_disable_pm_runtime:
>         pm_runtime_disable(dev);
> free_reset_assert:
>         reset_control_assert(pspim->rstc);
>         clk_disable_unprepare(pspim->spi_clk);
>
>         return ret;
> }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Andy Shevchenko]

On Fri, Nov 26, 2021 at 9:49 AM Lh Kuo 郭力豪 <lh.Kuo@sunplus.com> wrote:

(Uncommented is okay)

...

> > >         ret = devm_request_irq(dev, pspim->mas_irq, sp7021_spi_mas_irq
> > >                                                 , IRQF_TRIGGER_RISING,
> > > pdev->name, pspim);
> >
> > Ugly indentation.
> >
>
> Amended as follows, is it okay?
>
>         ret = devm_request_irq(dev, pspim->mas_irq, sp7021_spi_mas_irq
>                         , IRQF_TRIGGER_RISING, pdev->name, pspim);
>         if (ret)
>                 return ret;

Still not okay. Have you seen this style somewhere in the kernel?
Hint: something is really wrong with comma's location.

...

> > >         pm_runtime_enable(dev);
> > >
> > >         ret = devm_spi_register_controller(dev, ctlr);
> >
> > You can't mix non-devm with devm APIs. Either all non-devm, or devm followed by non-devm.

>   I don't understand so I need to change to spi_register_controller(ctlr)?   why?

I haven't told you that. What I'm saying is this:
1) all calls are devm_*() - OK!
2) all calls are non-devm_*() OK!
3) devm_*() followed by non-devm_*() OK!
4) non-devm_*() call followed by devm_*() call  NOT okay!

You need to fulfil your homework (see plenty of the examples in the
Linux kernel source tree on how to proceed).

> I modified the remove-function as follows. I think devm_spi_register_controller(dev, ctlr); should be no problem in the probe funciton.

It has ordering issues. That's why 4) above is not okay.

> static int sp7021_spi_controller_remove(struct platform_device *pdev)
> {
>         struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
>         struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
>
>         pm_runtime_disable(&pdev->dev);
>         pm_runtime_set_suspended(&pdev->dev);
>         reset_control_assert(pspim->rstc);
>         clk_disable_unprepare(pspim->spi_clk);
>
>         return 0;
> }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Philipp Zabel]

Hi LH,

On Fri, 2021-11-26 at 07:40 +0000, Lh Kuo 郭力豪 wrote:
[...]
> Amended as follows, is it okay?
> 
> 	ret = devm_request_irq(dev, pspim->mas_irq, sp7021_spi_mas_irq
> 			, IRQF_TRIGGER_RISING, pdev->name, pspim);
> 	if (ret)
> 		return ret;

Comma at the end of the line and align the next line with the opening
parenthesis:

	ret = devm_request_irq(dev, pspim->mas_irq, sp7021_spi_mas_irq,
			       IRQF_TRIGGER_RISING, pdev->name, pspim);

You can use scripts/checkpatch --strict to find these issues before
review.

> > >         pspim->rstc = devm_reset_control_get_exclusive(dev, NULL);
> > >         if (IS_ERR(pspim->rstc)) {
> > >                 return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst
> > > get fail\n");
> > 
> 
> Amended as follows, is it okay?
> 
> 	pspim->rstc = devm_reset_control_get_exclusive(dev, NULL);
> 	if (IS_ERR(pspim->rstc))
> 		return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst get fail\n");

Yes.
> > 

> > >         ret = devm_spi_register_controller(dev, ctlr);
> > 
> > You can't mix non-devm with devm APIs. Either all non-devm, or devm followed by non-devm.
> > 
> 
>   I don't understand so I need to change to spi_register_controller(ctlr)?   why?

devm_spi_register_controller() shouldn't be called after
pm_runtime_enable().

You could either switch to devm_pm_runtime_enable() or move the
pm_runtime_enable() after the devm_spi_register_controller() call if
possible, or switch to spi_register_controller().

> I modified the remove-function as follows. I think devm_spi_register_controller(dev, ctlr); should be no problem in the probe funciton.
> static int sp7021_spi_controller_remove(struct platform_device *pdev)
> {
> 	struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
> 	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
> 
> 	pm_runtime_disable(&pdev->dev);

I'm not sure if the SPI framework requires the spi_controller to be
unregistered before hardware is powered off, maybe it is enough to call
spi_controller_suspend() in the right place?

> 	pm_runtime_set_suspended(&pdev->dev);
> 	reset_control_assert(pspim->rstc);
> 	clk_disable_unprepare(pspim->spi_clk);
> 
> 	return 0;
> }

regards
Philipp

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Mark Brown]

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

On Fri, Nov 26, 2021 at 11:36:29AM +0100, Philipp Zabel wrote:

> > 	pm_runtime_disable(&pdev->dev);

> I'm not sure if the SPI framework requires the spi_controller to be
> unregistered before hardware is powered off, maybe it is enough to call
> spi_controller_suspend() in the right place?

It would *probably* do the right thing but the expectation really is
that you'll unregister before making the controller stop working, that
should be much more robust..

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Andy Shevchenko]

On Mon, Nov 29, 2021 at 8:20 AM Lh Kuo 郭力豪 <lh.Kuo@sunplus.com> wrote:

>    Feel sorry. I haven't found any devm PM API use in the SPI driver, and I didn't realize that PM function also has devm API. So I was confused before. I will move the pm_runtime_enable() after the devm_spi_register_controller() . I have rewritten the Probe and Remove functions as shown below.

Neither you found APIs for clock and reset, Try to grep for
devm_add_action_or_reset().

So, for PM it is probably good to leave it last, but you still have the issue.

>    And sp7021_spi_controller driver is modified and the code cleaned more than -50 LOCs. If the Probe and Remove functions is OK I will start next submission.

No, it's not okay.. yet, but we are closer. See my comments above and below.

>    Thanks for all comments
>
> static int sp7021_spi_controller_probe(struct platform_device *pdev)
> {
>         struct device *dev = &pdev->dev;
>         struct sp7021_spi_ctlr *pspim;
>         struct spi_controller *ctlr;
>         int mode;
>         int ret;
>
>         dev_info(dev, "sp7021_spi_controller_probe\n");
>
>         mode = SP7021_MASTER_MODE;
>         pdev->id = of_alias_get_id(pdev->dev.of_node, "sp_spi");
>
>         if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
>                 mode = SP7021_SLAVE_MODE;
>
>         if (mode == SP7021_SLAVE_MODE)
>                 ctlr = devm_spi_alloc_slave(dev, sizeof(*pspim));
>         else
>                 ctlr = devm_spi_alloc_master(dev, sizeof(*pspim));
>         if (!ctlr)
>                 return -ENOMEM;
>

>         ctlr->dev.of_node = pdev->dev.of_node;

device_set_node()

>         ctlr->bus_num = pdev->id;
>         ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
>         ctlr->auto_runtime_pm = true;
>         ctlr->prepare_message = sp7021_spi_controller_prepare_message;
>         if (mode == SP7021_SLAVE_MODE) {
>                 ctlr->transfer_one = sp7021_spi_sla_transfer_one;
>                 ctlr->slave_abort = sp7021_spi_sla_abort;
>                 ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX;
>         } else {
>                 ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
>                 ctlr->min_speed_hz = 40000;
>                 ctlr->max_speed_hz = 25000000;
>                 ctlr->use_gpio_descriptors = true;
>                 ctlr->flags = SPI_CONTROLLER_MUST_RX | SPI_CONTROLLER_MUST_TX;
>                 ctlr->transfer_one = sp7021_spi_mas_transfer_one;
>         }
>         platform_set_drvdata(pdev, ctlr);
>         pspim = spi_controller_get_devdata(ctlr);
>         pspim->ctlr = ctlr;
>         pspim->dev = dev;
>         spin_lock_init(&pspim->lock);
>         mutex_init(&pspim->buf_lock);
>         init_completion(&pspim->isr_done);
>         init_completion(&pspim->sla_isr);

>         pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, "master");
>         pspim->sla_base = devm_platform_ioremap_resource_byname(pdev, "slave");

Where are the error checks?

>         pspim->mas_irq = platform_get_irq_byname(pdev, "mas_risc");
>         if (pspim->mas_irq < 0)
>                 return pspim->mas_irq;
>
>         pspim->sla_irq = platform_get_irq_byname(pdev, "slave_risc");
>         if (pspim->sla_irq < 0)
>                 return pspim->sla_irq;
>
>         ret = devm_request_irq(dev, pspim->mas_irq, sp7021_spi_mas_irq,
>                                                 IRQF_TRIGGER_RISING, pdev->name, pspim);
>         if (ret)
>                 return ret;
>
>         ret = devm_request_irq(dev, pspim->sla_irq, sp7021_spi_sla_irq,
>                                                 IRQF_TRIGGER_RISING, pdev->name, pspim);
>         if (ret)
>                 return ret;
>
>         pspim->spi_clk = devm_clk_get(dev, NULL);
>         if (IS_ERR(pspim->spi_clk))
>                 return dev_err_probe(dev, PTR_ERR(pspim->spi_clk), "clk get fail\n");
>
>         pspim->rstc = devm_reset_control_get_exclusive(dev, NULL);
>         if (IS_ERR(pspim->rstc))
>                 return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst get fail\n");

>         ret = clk_prepare_enable(pspim->spi_clk);
>         if (ret)
>                 return dev_err_probe(dev, ret, "failed to enable clk\n");
>
>         ret = reset_control_deassert(pspim->rstc);
>         if (ret) {
>                 dev_err_probe(dev, ret, "failed to deassert reset\n");
>                 goto err_free_reset;
>         }

These two need to be wrapped as I explained above.

>         ret = devm_spi_register_controller(dev, ctlr);
>         pm_runtime_enable(dev);
>         if (ret) {


>                 dev_err(dev, "spi_register_master fail\n");
>                 goto err_disable_pm_runtime;

  pm_runtime_disable();
  return dev_err_probe();

>         }
>
>         return ret;
>
> err_disable_pm_runtime:
>         pm_runtime_disable(dev);
> err_free_reset:
>         reset_control_assert(pspim->rstc);
>         clk_disable_unprepare(pspim->spi_clk);
>
>         return ret;
> }
>
> static int sp7021_spi_controller_remove(struct platform_device *pdev)
> {
>         struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
>         struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);

>         spi_unregister_controller(ctlr);

Lukas already explained to you why this is wrong.

>         pm_runtime_disable(&pdev->dev);
>         pm_runtime_set_suspended(&pdev->dev);

>         reset_control_assert(pspim->rstc);
>         clk_disable_unprepare(pspim->spi_clk);

This has the same ordering issue as already discussed.

>         return 0;
> }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Andy Shevchenko]

On Tue, Nov 30, 2021 at 10:32 AM Lh Kuo 郭力豪 <lh.Kuo@sunplus.com> wrote:

> > >         ctlr->dev.of_node = pdev->dev.of_node;
> >
> > device_set_node()
>
> Is this funciton set as follows?
>         device_set_node(&ctlr->dev, of_fwnode_handle(pdev->dev.fwnode));

Can you please do something?
For example, figuring out yourself (Elixir is a very good service for
that): https://elixir.bootlin.com/linux/latest/A/ident/device_set_node

...

> > >         pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, "master");
> > >         pspim->sla_base = devm_platform_ioremap_resource_byname(pdev,
> > > "slave");
> >
> > Where are the error checks?

> The changes are as follows? is this correct?

Almost, but not enough. Please run checkpatch.

>         pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, "master");
>         if (IS_ERR(pspim->mas_base)) {
>                 return dev_err_probe(dev, PTR_ERR(pspim->mas_base), "mas_base get fail\n");
>         }
>
>         pspim->sla_base = devm_platform_ioremap_resource_byname(pdev, "slave");
>         if (IS_ERR(pspim->sla_base)) {
>                 return dev_err_probe(dev, PTR_ERR(pspim->sla_base), "sla_base get fail\n");
>         }

...

> > >         if (ret) {
> > >                 dev_err_probe(dev, ret, "failed to deassert reset\n");
> > >                 goto err_free_reset;
> > >         }
> >
> > These two need to be wrapped as I explained above.
>
> I think these changes are depend on remove-function.

No, it's the other way around: ->remove() implementation depends on
these changes.

> These settings are as follows? is this correct?

No.

>         pspim->spi_clk = devm_clk_get(dev, NULL);
>         if (IS_ERR(pspim->spi_clk))
>                 return dev_err_probe(dev, PTR_ERR(pspim->spi_clk), "clk get fail\n");
>
>         pspim->rstc = devm_reset_control_get_exclusive(dev, NULL);
>         if (IS_ERR(pspim->rstc))
>                 return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst get fail\n");
>
>         ret = clk_prepare_enable(pspim->spi_clk);
>         if (ret)
>                 return dev_err_probe(dev, ret, "failed to enable clk\n");
>
>         devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare,
>                         pspim->spi_clk);

Please, find other drivers as examples of how to do that and take care
about possible errors.

>         ret = reset_control_deassert(pspim->rstc);
>         if (ret)
>                 return dev_err_probe(dev, ret, "failed to deassert reset\n");
>
>         devm_add_action_or_reset(dev, (void(*)(void *))reset_control_assert,
>                         pspim->rstc);

Ditto.

>         ret = spi_register_controller(ctlr);

Read what Lukas said.

>         pm_runtime_enable(dev);
>         if (ret) {
>                 pm_runtime_disable(dev);
>                 return dev_err_probe(dev, ret, "spi_register_master fail\n");
>         }
>
>         return ret;
>
> }
>
> static int sp7021_spi_controller_remove(struct platform_device *pdev)
> {
>         struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
>
>         pm_runtime_disable(&pdev->dev);
>         pm_runtime_set_suspended(&pdev->dev);
>
>         return 0;
> }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Lukas Wunner]

On Mon, Nov 22, 2021 at 10:33:32AM +0800, LH.Kuo wrote:
> +static int sp7021_spi_controller_probe(struct platform_device *pdev)
> +{
[...]
> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret != 0) {
> +		dev_err(&pdev->dev, "spi_register_master fail\n");
> +		goto disable_runtime_pm;
> +	}

You need to use spi_register_controller() here (*not* the devm_ variant)
because you're using spi_unregister_controller() in
sp7021_spi_controller_remove().

> +
> +	// clk
> +	pspim->spi_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pspim->spi_clk)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pspim->spi_clk),
> +				     "devm_clk_get fail\n");
> +	}
> +
> +	// reset
> +	pspim->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	dev_dbg(&pdev->dev, "pspim->rstc : 0x%x\n", (unsigned int)pspim->rstc);
> +	if (IS_ERR(pspim->rstc)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pspim->rstc),
> +				     "devm_rst_get fail\n");
> +	}
> +
> +	ret = clk_prepare_enable(pspim->spi_clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +			"failed to enable clk\n");
> +
> +	ret = reset_control_deassert(pspim->rstc);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret,
> +			"failed to deassert reset\n");
> +		goto free_reset_assert;
> +
> +	}

You need to move the above steps *before* the call to
spi_register_controller().  Once spi_register_controller() returns,
it must be able to perform SPI transactions.  So you have to enable
all required clocks before calling it.  You also have to perform the
reset step before registration to avoid interfering with an ongoing
transaction.  The order of these steps must mirror the order in
sp7021_spi_controller_remove():  There you're unregistering the
controller *before* disabling the clock and asserting reset,
so the order must be inverted here.


> +static int sp7021_spi_controller_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
> +	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +
> +	spi_unregister_controller(pspim->ctlr);
> +	clk_disable_unprepare(pspim->spi_clk);
> +	reset_control_assert(pspim->rstc);
> +
> +	return 0;
> +}

I think the two calls to pm_runtime_* should be moved after
spi_unregister_controller() but that's probably not critical.

Thanks,

Lukas

[PATCH v3 2/2] devicetree: bindings SPI Add bindings doc for Sunplus SP7021

[LH.Kuo]

Add devicetree bindings SPI Add bindings doc for Sunplus SP7021

Signed-off-by: LH.Kuo <lh.kuo@sunplus.com>
---
Changes in v3:
 - Addressed all comments from Mr. Mark Brown
 - Addressed all comments from Mr. Philipp Zabel
 - Addressed all comments from Mr. Rob Herring
 - remove spi transfer_one_message

 .../bindings/spi/spi-sunplus-sp7021.yaml           | 95 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 96 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml

diff --git a/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
new file mode 100644
index 0000000..5502f15
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd. 2021
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/spi-sunplus-sp7021.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sunplus sp7021 SPI controller
+
+allOf:
+  - $ref: "spi-controller.yaml"
+
+maintainers:
+  - lh.kuo <lh.kuo@sunplus.com>
+
+properties:
+  compatible:
+    enum:
+      - sunplus,sp7021-spi-controller
+
+  reg:
+    items:
+      - description: Base address and length of the SPI master registers
+      - description: Base address and length of the SPI slave registers
+
+  reg-names:
+    items:
+      - const: spi_master
+      - const: spi_slave
+
+  interrupt-names:
+    items:
+      - const: dma_w_intr
+      - const: mas_risc_intr
+      - const: slave_risc_intr
+
+  interrupts:
+    minItems: 3
+
+  clocks:
+    maxItems: 1
+
+  clocks-names:
+    items:
+      - const: sys_pll
+
+  resets:
+    maxItems: 1
+
+  pinctrl-names:
+    description:
+      A pinctrl state named "default" must be defined.
+    const: default
+
+  pinctrl-0:
+    description:
+      A phandle to the default pinctrl state.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clocks-names
+  - resets
+  - pinctrl-names
+  - pinctrl-0
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sp-sp7021.h>
+    #include <dt-bindings/reset/sp-sp7021.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi@9C002D80 {
+        compatible = "sunplus,sp7021-spi-controller";
+        reg = <0x9C002D80 0x80>, <0x9C002E00 0x80>;
+        reg-names = "spi_master", "spi_slave";
+        interrupt-parent = <&intc>;
+        interrupt-names = "dma_w_intr",
+                          "mas_risc_intr",
+                          "slave_risc_intr";
+        interrupts = <144 IRQ_TYPE_LEVEL_HIGH>,
+                     <146 IRQ_TYPE_LEVEL_HIGH>,
+                     <145 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clkc SPI_COMBO_0>;
+        clocks-names = "sys_pll";
+        resets = <&rstc RST_SPI_COMBO_0>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pins_spi0>;
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 75fa7d5..88f3747 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18220,6 +18220,7 @@ SUNPLUS SPI CONTROLLER INTERFACE DRIVER
 M:	LH Kuo <lh.kuo@sunplus.com>
 L:	linux-spi@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
 F:	drivers/spi/spi-sunplus-sp7021.c
 
 SUPERH
-- 
2.7.4

Re: [PATCH v3 2/2] devicetree: bindings SPI Add bindings doc for Sunplus SP7021

[Rob Herring]

On Mon, Nov 22, 2021 at 10:33:33AM +0800, LH.Kuo wrote:
> Add devicetree bindings SPI Add bindings doc for Sunplus SP7021
> 
> Signed-off-by: LH.Kuo <lh.kuo@sunplus.com>

Again, From and S-o-b must match.

> ---
> Changes in v3:
>  - Addressed all comments from Mr. Mark Brown
>  - Addressed all comments from Mr. Philipp Zabel
>  - Addressed all comments from Mr. Rob Herring
>  - remove spi transfer_one_message
> 
>  .../bindings/spi/spi-sunplus-sp7021.yaml           | 95 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
> new file mode 100644
> index 0000000..5502f15
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (C) Sunplus Co., Ltd. 2021
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/spi-sunplus-sp7021.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sunplus sp7021 SPI controller
> +
> +allOf:
> +  - $ref: "spi-controller.yaml"
> +
> +maintainers:
> +  - lh.kuo <lh.kuo@sunplus.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sunplus,sp7021-spi-controller

I think you can drop '-controller'.

> +
> +  reg:
> +    items:
> +      - description: Base address and length of the SPI master registers
> +      - description: Base address and length of the SPI slave registers

Drop 'Base address and length of '. The rest is sufficient.

> +
> +  reg-names:
> +    items:
> +      - const: spi_master
> +      - const: spi_slave

Drop 'spi_'.

> +
> +  interrupt-names:
> +    items:
> +      - const: dma_w_intr
> +      - const: mas_risc_intr
> +      - const: slave_risc_intr

Drop '_intr', it's redundant.

> +
> +  interrupts:
> +    minItems: 3
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clocks-names:
> +    items:
> +      - const: sys_pll
> +
> +  resets:
> +    maxItems: 1
> +
> +  pinctrl-names:
> +    description:
> +      A pinctrl state named "default" must be defined.
> +    const: default
> +
> +  pinctrl-0:
> +    description:
> +      A phandle to the default pinctrl state.
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clocks-names
> +  - resets
> +  - pinctrl-names
> +  - pinctrl-0
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/sp-sp7021.h>
> +    #include <dt-bindings/reset/sp-sp7021.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi@9C002D80 {
> +        compatible = "sunplus,sp7021-spi-controller";
> +        reg = <0x9C002D80 0x80>, <0x9C002E00 0x80>;
> +        reg-names = "spi_master", "spi_slave";
> +        interrupt-parent = <&intc>;
> +        interrupt-names = "dma_w_intr",
> +                          "mas_risc_intr",
> +                          "slave_risc_intr";
> +        interrupts = <144 IRQ_TYPE_LEVEL_HIGH>,
> +                     <146 IRQ_TYPE_LEVEL_HIGH>,
> +                     <145 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clkc SPI_COMBO_0>;
> +        clocks-names = "sys_pll";
> +        resets = <&rstc RST_SPI_COMBO_0>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pins_spi0>;
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 75fa7d5..88f3747 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18220,6 +18220,7 @@ SUNPLUS SPI CONTROLLER INTERFACE DRIVER
>  M:	LH Kuo <lh.kuo@sunplus.com>
>  L:	linux-spi@vger.kernel.org
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
>  F:	drivers/spi/spi-sunplus-sp7021.c
>  
>  SUPERH
> -- 
> 2.7.4
> 
> 

Re: [PATCH v3 0/2] Add SPI control driver for Sunplus SP7021 SoC

[Mark Brown]

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

On Mon, Nov 22, 2021 at 10:33:31AM +0800, LH.Kuo wrote:
> This is a patch series for SPI driver for Sunplus SP7021 SoC.
> 
> Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
> many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and
> etc.) into a single chip. It is designed for industrial control.

The structure of the driver now looks good, but there are some smaller
issues remaining - I don't think I saw anything that Andy and Lukas
didn't already raise.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]