Add SPI control driver for Sunplus SP7021 SoC

Add SPI driver for Sunplus SP7021.

drivers/spi/spi-sunplus-sp7021.c new file +10 -0

+#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"
+

drivers/spi/spi-sunplus-sp7021.c new file +7 -0

+	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);
+

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

drivers/spi/spi-sunplus-sp7021.c new file +6 -0

+	FUNC_DBG();
+
+	reset_control_assert(pspim->rstc);
+
+	return 0;
+}

Quoted 9 lines

drivers/spi/spi-sunplus-sp7021.c line 5

Quoted 7 lines

+++ b/drivers/spi/spi-sunplus-sp7021.c

drivers/spi/spi-sunplus-sp7021.c new file +5 -0

@@ -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?

Quoted 2 lines

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.

Quoted 6 lines

drivers/spi/spi-sunplus-sp7021.c line 326

Quoted 3 lines

drivers/spi/spi-sunplus-sp7021.c new file +2 -0

+	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.

Quoted 6 lines

drivers/spi/spi-sunplus-sp7021.c line 615

Quoted 4 lines

drivers/spi/spi-sunplus-sp7021.c new file +4 -0

+	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.

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 1 line · LH.Kuo

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.

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

drivers/spi/spi-sunplus-sp7021.c new file +5 -0

+// 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;

Quoted 3 lines

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?

Quoted 5 lines

drivers/spi/spi-sunplus-sp7021.c line 163

Quoted 3 lines

drivers/spi/spi-sunplus-sp7021.c new file +3 -0

+// 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 ??

Quoted 4 lines

drivers/spi/spi-sunplus-sp7021.c line 194

Quoted 1 line

drivers/spi/spi-sunplus-sp7021.c new file +1 -0

+	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

Quoted 7 lines

drivers/spi/spi-sunplus-sp7021.c line 198

Quoted 2 lines

drivers/spi/spi-sunplus-sp7021.c new file +2 -0

+		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.

Quoted 7 lines

drivers/spi/spi-sunplus-sp7021.c line 238

Quoted 5 lines

drivers/spi/spi-sunplus-sp7021.c new file +4 -0

+// 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

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	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

Quoted 1 line

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.

Quoted 3 lines

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 ?

Quoted 4 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+// 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);

Quoted 14 lines

drivers/spi/spi-sunplus-sp7021.c line 545

Quoted 11 lines

drivers/spi/spi-sunplus-sp7021.c new file +11 -0

+	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 ?

Quoted 6 lines

drivers/spi/spi-sunplus-sp7021.c line 604

Quoted 3 lines

drivers/spi/spi-sunplus-sp7021.c new file +3 -0

+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

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	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 ?

Quoted 7 lines

drivers/spi/spi-sunplus-sp7021.c line 696

Quoted 4 lines

drivers/spi/spi-sunplus-sp7021.c new file +4 -0

+	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.

#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.

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>,

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?

This is my concern, so I use Transfer_One_message

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.

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);

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;

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.

drivers/spi/Kconfig +1 -0

+	  If unsure, say N.

Hi

Quoted 35 lines

-----Original Message-----
From: Randy Dunlap <[email protected]>
Sent: Wednesday, November 10, 2021 12:55 AM
To: LH.Kuo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; Wells Lu 呂芳騰 <[email protected]>; Lh Kuo 郭力豪 <[email protected]>
Subject: Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

drivers/spi/Kconfig config SPI_SUN6I · line 873

Quoted 13 lines · LH.Kuo

drivers/spi/Kconfig index renamed from drivers/spi/Kconfig +0 -0

596705d..30ce0ed 100644

drivers/spi/Kconfig +5 -0

@@ -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

drivers/spi/Kconfig config SPI_SUN6I · line 877

Quoted 4 lines

drivers/spi/Kconfig +4 -0

+	  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)

Quoted 3 lines

drivers/spi/Kconfig config SPI_SUN6I · line 878

Quoted 1 line

drivers/spi/Kconfig +1 -0

+	  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.

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.

Quoted 2 lines

--
~Randy

Add devicetree bindings SPI Add bindings doc for Sunplus SP7021

Quoted 7 lines

Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml line 29

Quoted 4 lines

Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml new file +4 -0

+  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.

Add SPI driver for Sunplus SP7021.

drivers/spi/Kconfig +5 -0

+         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.

drivers/spi/spi-sunplus-sp7021.c new file +2 -0

+// Copyright (c) 2021 Sunplus Inc.
+// Author: LH Kuo <[email protected]>

+#define SLAVE_INT_IN

+#define SP7021_CLR_MAS_INT (1<<6)

drivers/spi/spi-sunplus-sp7021.c new file +43 -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 {

+};

drivers/spi/spi-sunplus-sp7021.c new file +8 -0

+       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

+// slave only. usually called on driver remove

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

+ int err = 0;

drivers/spi/spi-sunplus-sp7021.c new file +3 -0

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

+exit_spi_slave_rw:

+}

drivers/spi/spi-sunplus-sp7021.c new file +11 -0

+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++;
+       }
+}
+

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

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

drivers/spi/spi-sunplus-sp7021.c new file +4 -0

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

+ pdev->id = 0;

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

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

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

(quoted diff) +2 -0

+               return pspim->sla_irq;
+       }

+ // clk

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

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

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

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

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

Quoted 1 line · LH.Kuo

+// 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.

drivers/spi/spi-sunplus-sp7021.c line 230

Quoted 5 lines

drivers/spi/spi-sunplus-sp7021.c new file +5 -0

+       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.

drivers/spi/spi-sunplus-sp7021.c line 505

Quoted 2 lines

drivers/spi/spi-sunplus-sp7021.c new file +2 -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.

Quoted 7 lines · Andy Shevchenko

Quoted 1 line · LH.Kuo

+// 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.

Quoted 5 lines

Quoted 5 lines

drivers/spi/spi-sunplus-sp7021.c new file +5 -0

+       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;

Quoted 1 line

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.

Quoted 4 lines

drivers/spi/spi-sunplus-sp7021.c line 505

Quoted 2 lines

drivers/spi/spi-sunplus-sp7021.c new file +2 -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.

Either way it doesn't
fundamentally matter.

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

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;

        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;
        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");

                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");

                return pspim->sla_irq;
        }

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

        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);

        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");
        }

        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 free_reset_assert;

        }

        pm_runtime_enable(dev);

        ret = devm_spi_register_controller(dev, ctlr);

        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;
}

Quoted 6 lines

Quoted 3 lines

        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;

Quoted 5 lines

Quoted 3 lines

        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 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);
        pm_runtime_set_suspended(&pdev->dev);
        reset_control_assert(pspim->rstc);
        clk_disable_unprepare(pspim->spi_clk);

        return 0;
}

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;

Quoted 5 lines

Quoted 4 lines

        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");

Quoted 6 lines

Quoted 3 lines

        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 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);

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

	return 0;
}

Quoted 1 line

	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?

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.

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.

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;

        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");

        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;
        }

        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;

        }

        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);

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

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

        return 0;
}

Quoted 3 lines

Quoted 1 line

        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));

Quoted 5 lines

Quoted 3 lines

        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?

        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");
        }

Quoted 6 lines

Quoted 4 lines

        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.

These settings are as follows? is this correct?

        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);

        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);

        ret = spi_register_controller(ctlr);

        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;
}

drivers/spi/spi-sunplus-sp7021.c new file +2 -0

+static int sp7021_spi_controller_probe(struct platform_device *pdev)
+{

drivers/spi/spi-sunplus-sp7021.c new file +18 -0

+
+	// 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)

drivers/spi/spi-sunplus-sp7021.c new file +4 -0

+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);

Add devicetree bindings SPI Add bindings doc for Sunplus SP7021

Signed-off-by LH.Kuo <[email protected]>

---
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

Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml new file +9 -0

new file mode 100644
index 0000000..5502f15
@@ -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
+

Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml new file +60 -0

+
+  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>;
+    };
+...

MAINTAINERS +1 -0

index 75fa7d5..88f3747 100644
@@ -18220,6 +18220,7 @@ SUNPLUS SPI CONTROLLER INTERFACE DRIVER
 M:	LH Kuo <[email protected]>
 L:	[email protected]
 S:	Maintained
+F:	Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
 F:	drivers/spi/spi-sunplus-sp7021.c
 
 SUPERH

Signature · 1 line

2.7.4

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.