[PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

[PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

[Qing Zhang]

The SPI controller has the following characteristics:

- Full-duplex synchronous serial data transmission
- Support up to 4 variable length byte transmission
- Main mode support
- Mode failure generates an error flag and issues an interrupt request
- Double buffer receiver
- Serial clock with programmable polarity and phase
- SPI can be controlled in wait mode
- Support boot from SPI

Use mtd_debug tool to earse/write/read /dev/mtd0 on development.

eg:

[root@linux mtd-utils-1.0.0]# mtd_debug erase /dev/mtd0 0x20000 0x40000
Erased 262144 bytes from address 0x00020000 in flash
[root@linux mtd-utils-1.0.0]# mtd_debug write /dev/mtd0 0x20000 13 1.img
Copied 13 bytes from 1.img to address 0x00020000 in flash
[root@linux mtd-utils-1.0.0]# mtd_debug read /dev/mtd0 0x20000 13 2.img
Copied 13 bytes from address 0x00020000 in flash to 2.img
[root@linux mtd-utils-1.0.0]# cmp -l 1.img 2.img

Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
---

v2:
- keep Kconfig and Makefile sorted
- make the entire comment a C++ one so things look more intentional
- Fix unclear indentation
- make conditional statements to improve legibility
- Don't use static inline
- the core handle message queue
- Add a new binding document
- Fix probe part mixed pdev and PCI
---
 drivers/spi/Kconfig    |   7 ++
 drivers/spi/Makefile   |   1 +
 drivers/spi/spi-ls7a.c | 324 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 332 insertions(+)
 create mode 100644 drivers/spi/spi-ls7a.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index aadaea0..af7c0d4 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -413,6 +413,13 @@ config SPI_LP8841_RTC
 	  Say N here unless you plan to run the kernel on an ICP DAS
 	  LP-8x4x industrial computer.
 
+config SPI_LS7A
+	tristate "Loongson LS7A SPI Controller Support"
+	depends on CPU_LOONGSON64 || COMPILE_TEST
+	help
+	  This drivers supports the Loongson LS7A SPI controller in master
+	  SPI mode.
+
 config SPI_MPC52xx
 	tristate "Freescale MPC52xx SPI (non-PSC) controller support"
 	depends on PPC_MPC52xx
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 6fea582..d015cf2 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_SPI_LANTIQ_SSC)		+= spi-lantiq-ssc.o
 obj-$(CONFIG_SPI_JCORE)			+= spi-jcore.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
 obj-$(CONFIG_SPI_LP8841_RTC)		+= spi-lp8841-rtc.o
+obj-$(CONFIG_SPI_LS7A)			+= spi-ls7a.o
 obj-$(CONFIG_SPI_MESON_SPICC)		+= spi-meson-spicc.o
 obj-$(CONFIG_SPI_MESON_SPIFC)		+= spi-meson-spifc.o
 obj-$(CONFIG_SPI_MPC512x_PSC)		+= spi-mpc512x-psc.o
diff --git a/drivers/spi/spi-ls7a.c b/drivers/spi/spi-ls7a.c
new file mode 100644
index 0000000..21ca1ab
--- /dev/null
+++ b/drivers/spi/spi-ls7a.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Loongson LS7A SPI Controller driver
+ *
+ * Copyright (C) 2020 Loongson Technology Corporation Limited
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/spi/spi.h>
+#include <linux/pci.h>
+#include <linux/of.h>
+
+/* define spi register */
+#define	SPCR	0x00
+#define	SPSR	0x01
+#define	FIFO	0x02
+#define	SPER	0x03
+#define	PARA	0x04
+#define	SFCS	0x05
+#define	TIMI	0x06
+
+struct ls7a_spi {
+	spinlock_t lock;
+	struct spi_master *master;
+	void __iomem *base;
+	int cs_active;
+	unsigned int hz;
+	unsigned char spcr, sper;
+	unsigned int mode;
+};
+
+static void ls7a_spi_write_reg(struct ls7a_spi *spi,
+			       unsigned char reg,
+			       unsigned char data)
+{
+	writeb(data, spi->base + reg);
+}
+
+static char ls7a_spi_read_reg(struct ls7a_spi *spi,
+			      unsigned char reg)
+{
+	return readb(spi->base + reg);
+}
+
+static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device  *spi, int val)
+{
+	int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select);
+
+	if (spi->mode  & SPI_CS_HIGH)
+		val = !val;
+	ls7a_spi_write_reg(ls7a_spi, SFCS,
+		(val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs);
+
+	return 0;
+}
+
+static int ls7a_spi_do_transfer(struct ls7a_spi *ls7a_spi,
+				struct spi_device *spi,
+				struct spi_transfer *t)
+{
+	unsigned int hz;
+	unsigned int div, div_tmp;
+	unsigned int bit;
+	unsigned long clk;
+	unsigned char val;
+	const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
+
+	if (t) {
+		hz = t->speed_hz;
+		if (!hz)
+			hz = spi->max_speed_hz;
+	} else
+		hz = spi->max_speed_hz;
+
+	if (((spi->mode ^ ls7a_spi->mode) & (SPI_CPOL | SPI_CPHA))
+		|| (hz && ls7a_spi->hz != hz)) {
+		clk = 100000000;
+
+		div = DIV_ROUND_UP(clk, hz);
+		if (div < 2)
+			div = 2;
+		if (div > 4096)
+			div = 4096;
+
+		bit = fls(div) - 1;
+		if ((1<<bit) == div)
+			bit--;
+		div_tmp = rdiv[bit];
+
+		dev_dbg(&spi->dev, "clk = %ld hz = %d div_tmp = %d bit = %d\n",
+			clk, hz, div_tmp, bit);
+
+		ls7a_spi->hz = hz;
+		ls7a_spi->spcr = div_tmp & 3;
+		ls7a_spi->sper = (div_tmp >> 2) & 3;
+
+		val = ls7a_spi_read_reg(ls7a_spi, SPCR);
+		val &= ~0xc;
+		if (spi->mode & SPI_CPOL)
+			val |= 8;
+		if (spi->mode & SPI_CPHA)
+			val |= 4;
+		ls7a_spi_write_reg(ls7a_spi, SPCR, (val & ~3) | ls7a_spi->spcr);
+		val = ls7a_spi_read_reg(ls7a_spi, SPER);
+		ls7a_spi_write_reg(ls7a_spi, SPER, (val & ~3) | ls7a_spi->sper);
+		ls7a_spi->mode = spi->mode;
+	}
+	return 0;
+}
+
+static int ls7a_spi_setup(struct spi_device *spi)
+{
+	struct ls7a_spi *ls7a_spi;
+
+	ls7a_spi = spi_master_get_devdata(spi->master);
+
+	if (spi->chip_select >= spi->master->num_chipselect)
+		return -EINVAL;
+
+	set_cs(ls7a_spi, spi, 1);
+
+	return 0;
+}
+
+static int ls7a_spi_write_read_8bit(struct spi_device *spi,
+				    const u8 **tx_buf, u8 **rx_buf,
+				    unsigned int num)
+{
+	struct ls7a_spi *ls7a_spi;
+
+	ls7a_spi = spi_master_get_devdata(spi->master);
+
+	if (tx_buf && *tx_buf) {
+		ls7a_spi_write_reg(ls7a_spi, FIFO, *((*tx_buf)++));
+
+		while ((ls7a_spi_read_reg(ls7a_spi, SPSR) & 0x1) == 1)
+			;
+	} else {
+		ls7a_spi_write_reg(ls7a_spi, FIFO, 0);
+
+		while ((ls7a_spi_read_reg(ls7a_spi, SPSR) & 0x1) == 1)
+			;
+	}
+
+	if (rx_buf && *rx_buf)
+		*(*rx_buf)++ = ls7a_spi_read_reg(ls7a_spi, FIFO);
+	else
+		ls7a_spi_read_reg(ls7a_spi, FIFO);
+
+	return 1;
+}
+
+static unsigned int ls7a_spi_write_read(struct spi_device *spi,
+					struct spi_transfer *xfer)
+{
+	struct ls7a_spi *ls7a_spi;
+	unsigned int count;
+	const u8 *tx = xfer->tx_buf;
+	u8 *rx = xfer->rx_buf;
+
+	ls7a_spi = spi_master_get_devdata(spi->master);
+	count = xfer->len;
+
+	do {
+		if (ls7a_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
+			goto out;
+		count--;
+	} while (count);
+
+out:
+	return xfer->len - count;
+}
+
+static int  ls7a_spi_transfer_one_message(struct spi_master *master,
+					  struct spi_message *m)
+{
+	struct ls7a_spi *ls7a_spi;
+	struct spi_device *spi;
+	struct spi_transfer *t = NULL;
+	int param, status, r;
+	unsigned int total_len = 0;
+	unsigned int cs_change;
+	const int nsecs = 50;
+
+	ls7a_spi = spi_master_get_devdata(master);
+	spi = m->spi;
+
+	cs_change = 1;
+
+	spin_lock(&ls7a_spi->lock);
+	param = ls7a_spi_read_reg(ls7a_spi, PARA);
+	ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
+	spin_unlock(&ls7a_spi->lock);
+	list_for_each_entry(t, &m->transfers, transfer_list) {
+		if (cs_change) {
+			set_cs(ls7a_spi, spi, 0);
+			ls7a_spi_do_transfer(ls7a_spi, spi, t);
+			ndelay(nsecs);
+		}
+		cs_change = t->cs_change;
+
+		r = ls7a_spi_write_read(spi, t);
+		if (r < 0) {
+			status = r;
+			goto error;
+			}
+		total_len += r;
+
+		spi_transfer_delay_exec(t);
+
+		if (cs_change) {
+			set_cs(ls7a_spi, spi, 1);
+			ndelay(nsecs);
+		}
+	}
+
+	spin_lock(&ls7a_spi->lock);
+	ls7a_spi_write_reg(ls7a_spi, PARA, param);
+	spin_unlock(&ls7a_spi->lock);
+
+	if (!cs_change) {
+		ndelay(nsecs);
+		set_cs(ls7a_spi, spi, 1);
+	}
+
+error:
+	m->status = status;
+	m->actual_length = total_len;
+	spi_finalize_current_message(master);
+	return status;
+}
+
+static int ls7a_spi_pci_probe(struct pci_dev *pdev,
+			      const struct pci_device_id *ent)
+{
+	struct device *dev = &pdev->dev;
+	struct spi_master *master;
+	struct ls7a_spi *spi;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
+	if (!master)
+		return -ENOMEM;
+
+	spi = spi_master_get_devdata(master);
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		goto err_free_master;
+
+	ret = pci_request_regions(pdev, "ls7a-spi");
+	if (ret)
+		goto err_free_master;
+
+	spi->base = pcim_iomap(pdev, 0, pci_resource_len(pdev, 0));
+	if (!spi->base) {
+		ret = -EINVAL;
+		goto err_free_master;
+	}
+	ls7a_spi_write_reg(spi, SPCR, 0x51);
+	ls7a_spi_write_reg(spi, SPER, 0x00);
+	ls7a_spi_write_reg(spi, TIMI, 0x01);
+	ls7a_spi_write_reg(spi, PARA, 0x40);
+	spi->mode = 0;
+
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
+	master->setup = ls7a_spi_setup;
+	master->transfer_one_message  = ls7a_spi_transfer_one_message;
+	master->bits_per_word_mask = SPI_BPW_MASK(8);
+	master->num_chipselect = 4;
+	master->dev.of_node = of_node_get(pdev->dev.of_node);
+
+	spi->master = master;
+
+	pci_set_drvdata(pdev, master);
+
+	ret = devm_spi_register_master(dev, master);
+	if (ret)
+		goto err_free_master;
+
+	return 0;
+
+err_free_master:
+	pci_release_regions(pdev);
+	spi_master_put(master);
+	return ret;
+}
+
+static void ls7a_spi_pci_remove(struct pci_dev *pdev)
+{
+	struct spi_master *master = pci_get_drvdata(pdev);
+	struct ls7a_spi *spi;
+
+	spi = spi_master_get_devdata(master);
+	if (!spi)
+		return;
+
+	pci_release_regions(pdev);
+}
+
+static const struct pci_device_id ls7a_spi_pci_id_table[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a0b) },
+	{ 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, ls7a_spi_pci_id_table);
+
+static struct pci_driver ls7a_spi_pci_driver = {
+	.name		= "ls7a-spi",
+	.id_table	= ls7a_spi_pci_id_table,
+	.probe		= ls7a_spi_pci_probe,
+	.remove		= ls7a_spi_pci_remove,
+};
+
+module_pci_driver(ls7a_spi_pci_driver);
+
+MODULE_AUTHOR("Juxin Gao <gaojuxin@loongson.cn>");
+MODULE_AUTHOR("Qing Zhang <zhangqing@loongson.cn>");
+MODULE_DESCRIPTION("Loongson LS7A SPI controller driver");
-- 
2.1.0

[PATCH v2 2/4] spi: Add devicetree bindings documentation for Loongson SPI

[Qing Zhang]

Add spi-ls7a binding documentation.

Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
---
 Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
new file mode 100644
index 0000000..56247b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
@@ -0,0 +1,31 @@
+Binding for LOONGSON LS7A SPI controller
+
+Required properties:
+- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
+- reg: reference IEEE Std 1275-1994.
+- #address-cells: <1>, as required by generic SPI binding.
+- #size-cells: <0>, also as required by generic SPI binding.
+- #interrupts: No hardware interrupt.
+
+Child nodes as per the generic SPI binding.
+
+Example:
+
+			spi@16,0 {
+				compatible = "pci0014,7a0b.0",
+						"pci0014,7a0b",
+						"pciclass088000",
+						"pciclass0880";
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0xb000 0x0 0x0 0x0 0x0>;
+				num-chipselects = <0>;
+				spiflash: s25fl016k@0 {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				compatible ="spansion,s25fl016k","jedec,spi-nor";
+				spi-max-frequency=<50000000>;
+				reg=<0>;
+				};
+			};
-- 
2.1.0

[PATCH v2 3/4] MIPS: Loongson64: DTS: Add SPI support to LS7A

[Qing Zhang]

add spi and amd node support.

Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
---

v2:
- Add spi about pci device DT
---
 arch/mips/boot/dts/loongson/ls7a-pch.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/mips/boot/dts/loongson/ls7a-pch.dtsi b/arch/mips/boot/dts/loongson/ls7a-pch.dtsi
index f99a7a1..ab8836b 100644
--- a/arch/mips/boot/dts/loongson/ls7a-pch.dtsi
+++ b/arch/mips/boot/dts/loongson/ls7a-pch.dtsi
@@ -405,6 +405,26 @@
 				interrupt-map-mask = <0 0 0 0>;
 				interrupt-map = <0 0 0 0 &pic 39 IRQ_TYPE_LEVEL_HIGH>;
 			};
+
+			spi@16,0 {
+				compatible = "pci0014,7a0b.0",
+						"pci0014,7a0b",
+						"pciclass088000",
+						"pciclass0880";
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				reg = <0xb000 0x0 0x0 0x0 0x0>;
+				num-chipselects = <0>;
+				spiflash: s25fl016k@0 {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				compatible ="spansion,s25fl016k","jedec,spi-nor";
+				spi-max-frequency=<50000000>;
+				reg=<0>;
+				};
+			};
 		};
 
 		isa {
-- 
2.1.0

[PATCH v2 4/4] MIPS: Loongson: Enable Loongson LS7A SPI in loongson3_defconfig

[Qing Zhang]

This is now supported, enable for Loongson systems.

Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
---

v2:
- Modify CONFIG_SPI_LOONGSON to CONFIG_SPI_LS7A
---
 arch/mips/configs/loongson3_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/configs/loongson3_defconfig b/arch/mips/configs/loongson3_defconfig
index 38a817e..28784cb 100644
--- a/arch/mips/configs/loongson3_defconfig
+++ b/arch/mips/configs/loongson3_defconfig
@@ -271,6 +271,9 @@ CONFIG_HW_RANDOM=y
 CONFIG_RAW_DRIVER=m
 CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_PIIX4=y
+CONFIG_SPI=y
+CONFIG_SPI_MASTER=y
+CONFIG_SPI_LS7A=y
 CONFIG_GPIO_LOONGSON=y
 CONFIG_SENSORS_LM75=m
 CONFIG_SENSORS_LM93=m
-- 
2.1.0

Re: [PATCH v2 2/4] spi: Add devicetree bindings documentation for Loongson SPI

[Jiaxun Yang]

在 2020/12/8 15:44, Qing Zhang 写道:
> Add spi-ls7a binding documentation.
>
> Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
> ---
>   Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt


Hi Qing,

Please use dt schema instead.

Thanks.

- Jiaxun


> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
> new file mode 100644
> index 0000000..56247b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
> @@ -0,0 +1,31 @@
> +Binding for LOONGSON LS7A SPI controller
> +
> +Required properties:
> +- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
> +- reg: reference IEEE Std 1275-1994.
> +- #address-cells: <1>, as required by generic SPI binding.
> +- #size-cells: <0>, also as required by generic SPI binding.
> +- #interrupts: No hardware interrupt.
> +
> +Child nodes as per the generic SPI binding.
> +
> +Example:
> +
> +			spi@16,0 {
> +				compatible = "pci0014,7a0b.0",
> +						"pci0014,7a0b",
> +						"pciclass088000",
> +						"pciclass0880";
> +
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <0xb000 0x0 0x0 0x0 0x0>;
> +				num-chipselects = <0>;
> +				spiflash: s25fl016k@0 {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				compatible ="spansion,s25fl016k","jedec,spi-nor";
> +				spi-max-frequency=<50000000>;
> +				reg=<0>;
> +				};
> +			};

Re: [PATCH v2 3/4] MIPS: Loongson64: DTS: Add SPI support to LS7A

[Jiaxun Yang]

在 2020/12/8 15:44, Qing Zhang 写道:
> add spi and amd node support.

Hi Qing,


Thanks for your patch.

What is AMD node?

Also given that different boards may have different flash, is it a wise

idea to hardcode here?

Thanks.

- Jiaxun


>
> Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
> ---
>
> v2:
> - Add spi about pci device DT
> ---
>   arch/mips/boot/dts/loongson/ls7a-pch.dtsi | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/arch/mips/boot/dts/loongson/ls7a-pch.dtsi b/arch/mips/boot/dts/loongson/ls7a-pch.dtsi
> index f99a7a1..ab8836b 100644
> --- a/arch/mips/boot/dts/loongson/ls7a-pch.dtsi
> +++ b/arch/mips/boot/dts/loongson/ls7a-pch.dtsi
> @@ -405,6 +405,26 @@
>   				interrupt-map-mask = <0 0 0 0>;
>   				interrupt-map = <0 0 0 0 &pic 39 IRQ_TYPE_LEVEL_HIGH>;
>   			};
> +
> +			spi@16,0 {
> +				compatible = "pci0014,7a0b.0",
> +						"pci0014,7a0b",
> +						"pciclass088000",
> +						"pciclass0880";
> +
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				reg = <0xb000 0x0 0x0 0x0 0x0>;
> +				num-chipselects = <0>;
> +				spiflash: s25fl016k@0 {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				compatible ="spansion,s25fl016k","jedec,spi-nor";
> +				spi-max-frequency=<50000000>;
> +				reg=<0>;
> +				};
> +			};
>   		};
>   
>   		isa {

Re: [PATCH v2 2/4] spi: Add devicetree bindings documentation for Loongson SPI

[Sergei Shtylyov]

Hello!

On 08.12.2020 10:44, Qing Zhang wrote:

> Add spi-ls7a binding documentation.
> 
> Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
> ---
>   Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
> new file mode 100644
> index 0000000..56247b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
> @@ -0,0 +1,31 @@
> +Binding for LOONGSON LS7A SPI controller
> +
> +Required properties:
> +- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
> +- reg: reference IEEE Std 1275-1994.
> +- #address-cells: <1>, as required by generic SPI binding.
> +- #size-cells: <0>, also as required by generic SPI binding.
> +- #interrupts: No hardware interrupt.

    You say it's a required prop, yet yuoe example doesn't have it...

> +
> +Child nodes as per the generic SPI binding.
> +
> +Example:
> +
> +			spi@16,0 {
> +				compatible = "pci0014,7a0b.0",
> +						"pci0014,7a0b",
> +						"pciclass088000",
> +						"pciclass0880";
> +
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <0xb000 0x0 0x0 0x0 0x0>;
> +				num-chipselects = <0>;
> +				spiflash: s25fl016k@0 {
> +				#address-cells = <1>;
> +				#size-cells = <1>;

    Once more?

> +				compatible ="spansion,s25fl016k","jedec,spi-nor";

    Once more?

> +				spi-max-frequency=<50000000>;
> +				reg=<0>;

    Once more? Did you mean this for a child node?

> +				};
> +			};

MBR, Sergei

Re: [PATCH v2 2/4] spi: Add devicetree bindings documentation for Loongson SPI

[zhangqing]

Hi Sergei Shtylyov,


On 12/08/2020 04:40 PM, Sergei Shtylyov wrote:
> Hello!
>
> On 08.12.2020 10:44, Qing Zhang wrote:
>
>> Add spi-ls7a binding documentation.
>>
>> Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
>> ---
>>   Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 
>> ++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt 
>> b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>> new file mode 100644
>> index 0000000..56247b5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>> @@ -0,0 +1,31 @@
>> +Binding for LOONGSON LS7A SPI controller
>> +
>> +Required properties:
>> +- compatible: should be 
>> "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
>> +- reg: reference IEEE Std 1275-1994.
>> +- #address-cells: <1>, as required by generic SPI binding.
>> +- #size-cells: <0>, also as required by generic SPI binding.
>> +- #interrupts: No hardware interrupt.
>
>    You say it's a required prop, yet yuoe example doesn't have it...
         I want to emphasize here that LS7A SPI has no hardware 
interrupts, and DT is not actually used.
>
>> +
>> +Child nodes as per the generic SPI binding.
>> +
>> +Example:
>> +
>> +            spi@16,0 {
>> +                compatible = "pci0014,7a0b.0",
>> +                        "pci0014,7a0b",
>> +                        "pciclass088000",
>> +                        "pciclass0880";
>> +
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +                reg = <0xb000 0x0 0x0 0x0 0x0>;
>> +                num-chipselects = <0>;
>> +                spiflash: s25fl016k@0 {
>> +                #address-cells = <1>;
>> +                #size-cells = <1>;
>
>    Once more?
>
>> +                compatible ="spansion,s25fl016k","jedec,spi-nor";
>
>    Once more?
>
>> + spi-max-frequency=<50000000>;
>> +                reg=<0>;
>
>    Once more? Did you mean this for a child node?
        Yes, these are child node attributes, the child node splash is 
not necessary.
>
>> +                };
>> +            };
>
      Thanks

      -Qing
> MBR, Sergei

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

[kernel test robot]

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

Hi Qing,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on spi/for-next]
[also build test WARNING on robh/for-next linus/master v5.10-rc7 next-20201207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
        git checkout 6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/spi/spi-ls7a.c: In function 'ls7a_spi_write_read':
>> drivers/spi/spi-ls7a.c:162:19: warning: variable 'ls7a_spi' set but not used [-Wunused-but-set-variable]
     162 |  struct ls7a_spi *ls7a_spi;
         |                   ^~~~~~~~

vim +/ls7a_spi +162 drivers/spi/spi-ls7a.c

   158	
   159	static unsigned int ls7a_spi_write_read(struct spi_device *spi,
   160						struct spi_transfer *xfer)
   161	{
 > 162		struct ls7a_spi *ls7a_spi;
   163		unsigned int count;
   164		const u8 *tx = xfer->tx_buf;
   165		u8 *rx = xfer->rx_buf;
   166	
   167		ls7a_spi = spi_master_get_devdata(spi->master);
   168		count = xfer->len;
   169	
   170		do {
   171			if (ls7a_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
   172				goto out;
   173			count--;
   174		} while (count);
   175	
   176	out:
   177		return xfer->len - count;
   178	}
   179	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 77047 bytes --]

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

[kernel test robot]

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

Hi Qing,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on spi/for-next]
[also build test WARNING on robh/for-next linus/master v5.10-rc7 next-20201207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
        git checkout 6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/spi/spi-ls7a.c: In function 'ls7a_spi_write_read':
>> drivers/spi/spi-ls7a.c:162:19: warning: variable 'ls7a_spi' set but not used [-Wunused-but-set-variable]
     162 |  struct ls7a_spi *ls7a_spi;
         |                   ^~~~~~~~

vim +/ls7a_spi +162 drivers/spi/spi-ls7a.c

   158	
   159	static unsigned int ls7a_spi_write_read(struct spi_device *spi,
   160						struct spi_transfer *xfer)
   161	{
 > 162		struct ls7a_spi *ls7a_spi;
   163		unsigned int count;
   164		const u8 *tx = xfer->tx_buf;
   165		u8 *rx = xfer->rx_buf;
   166	
   167		ls7a_spi = spi_master_get_devdata(spi->master);
   168		count = xfer->len;
   169	
   170		do {
   171			if (ls7a_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
   172				goto out;
   173			count--;
   174		} while (count);
   175	
   176	out:
   177		return xfer->len - count;
   178	}
   179	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 77047 bytes --]

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

[Mark Brown]

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

On Tue, Dec 08, 2020 at 03:44:24PM +0800, Qing Zhang wrote:

> v2:
> - keep Kconfig and Makefile sorted
> - make the entire comment a C++ one so things look more intentional

You say this but...

> +++ b/drivers/spi/spi-ls7a.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Loongson LS7A SPI Controller driver
> + *
> + * Copyright (C) 2020 Loongson Technology Corporation Limited
> + */

...this is still a mix of C and C++ comments?

> +static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device  *spi, int val)
> +{
> +	int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select);
> +
> +	if (spi->mode  & SPI_CS_HIGH)
> +		val = !val;
> +	ls7a_spi_write_reg(ls7a_spi, SFCS,
> +		(val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs);
> +
> +	return 0;
> +}

Why not just expose this to the core and let it handle things?  

Please also write normal conditional statements to improve legibility.
There's quite a lot of coding style issues in this with things like
missing spaces 

> +	if (t) {
> +		hz = t->speed_hz;
> +		if (!hz)
> +			hz = spi->max_speed_hz;
> +	} else
> +		hz = spi->max_speed_hz;

If one branch of the conditional has braces please use them on both to
improve legibility.

> +static int  ls7a_spi_transfer_one_message(struct spi_master *master,
> +                                         struct spi_message *m)

I don't understand why the driver is implementing transfer_one_message()
- it looks like this is just open coding the standard loop that the
framework provides and should just be using transfer_one().

> +		r = ls7a_spi_write_read(spi, t);
> +		if (r < 0) {
> +			status = r;
> +			goto error;
> +			}

The indentation here isn't following the kernel coding style.

> +	master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
> +	if (!master)
> +		return -ENOMEM;

Why not use devm_ here?

> +	ret = devm_spi_register_master(dev, master);
> +	if (ret)
> +		goto err_free_master;

The driver uses devm_spi_register_master() here but...

> +static void ls7a_spi_pci_remove(struct pci_dev *pdev)
> +{
> +	struct spi_master *master = pci_get_drvdata(pdev);
> +	struct ls7a_spi *spi;
> +
> +	spi = spi_master_get_devdata(master);
> +	if (!spi)
> +		return;
> +
> +	pci_release_regions(pdev);

...releases the PCI regions in the remove() function before the SPI
controller is freed so the controller could still be active.

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

Re: [PATCH v2 2/4] spi: Add devicetree bindings documentation for Loongson SPI

[Mark Brown]

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

On Tue, Dec 08, 2020 at 06:47:10PM +0800, zhangqing wrote:
> On 12/08/2020 04:40 PM, Sergei Shtylyov wrote:

> > > +Required properties:

> > > +- #interrupts: No hardware interrupt.

> >    You say it's a required prop, yet yuoe example doesn't have it...

>         I want to emphasize here that LS7A SPI has no hardware interrupts,
> and DT is not actually used.

There is no need to do this, and documenting the property as required
just makes things confusing here.

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

Re: [PATCH v2 2/4] spi: Add devicetree bindings documentation for Loongson SPI

[Sergei Shtylyov]

On 12/8/20 1:47 PM, zhangqing wrote:

>>> Add spi-ls7a binding documentation.
>>>
>>> Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
>>> ---
>>>   Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
>>>   1 file changed, 31 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>> new file mode 100644
>>> index 0000000..56247b5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>> @@ -0,0 +1,31 @@
>>> +Binding for LOONGSON LS7A SPI controller
>>> +
>>> +Required properties:
>>> +- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
>>> +- reg: reference IEEE Std 1275-1994.
>>> +- #address-cells: <1>, as required by generic SPI binding.
>>> +- #size-cells: <0>, also as required by generic SPI binding.
>>> +- #interrupts: No hardware interrupt.
>>
>>    You say it's a required prop, yet yuoe example doesn't have it...
>         I want to emphasize here that LS7A SPI has no hardware interrupts, and DT is not actually used.

   The why document the property at all?

>>> +
>>> +Child nodes as per the generic SPI binding.
>>> +
>>> +Example:
>>> +
>>> +            spi@16,0 {
>>> +                compatible = "pci0014,7a0b.0",
>>> +                        "pci0014,7a0b",
>>> +                        "pciclass088000",
>>> +                        "pciclass0880";
>>> +
>>> +                #address-cells = <1>;
>>> +                #size-cells = <0>;
>>> +                reg = <0xb000 0x0 0x0 0x0 0x0>;
>>> +                num-chipselects = <0>;
>>> +                spiflash: s25fl016k@0 {
>>> +                #address-cells = <1>;
>>> +                #size-cells = <1>;
>>
>>    Once more?
>>
>>> +                compatible ="spansion,s25fl016k","jedec,spi-nor";
>>
>>    Once more?
>>
>>> + spi-max-frequency=<50000000>;
>>> +                reg=<0>;
>>
>>    Once more? Did you mean this for a child node?
>        Yes, these are child node attributes, the child node splash is not necessary.

   You should indent the child nodes with 1 more tab...

>>
>>> +                };
>>> +            };
>>
>      Thanks
> 
>      -Qing

MBR, Sergei

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

[kernel test robot]

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

Hi Qing,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on robh/for-next linus/master v5.10-rc7 next-20201208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
        git checkout 6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/spi/spi-ls7a.c: In function 'ls7a_spi_write_read':
   drivers/spi/spi-ls7a.c:162:19: warning: variable 'ls7a_spi' set but not used [-Wunused-but-set-variable]
     162 |  struct ls7a_spi *ls7a_spi;
         |                   ^~~~~~~~
   drivers/spi/spi-ls7a.c: At top level:
>> drivers/spi/spi-ls7a.c:320:1: warning: data definition has no type or storage class
     320 | module_pci_driver(ls7a_spi_pci_driver);
         | ^~~~~~~~~~~~~~~~~
>> drivers/spi/spi-ls7a.c:320:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
>> drivers/spi/spi-ls7a.c:320:1: warning: parameter names (without types) in function declaration
   drivers/spi/spi-ls7a.c:313:26: warning: 'ls7a_spi_pci_driver' defined but not used [-Wunused-variable]
     313 | static struct pci_driver ls7a_spi_pci_driver = {
         |                          ^~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +320 drivers/spi/spi-ls7a.c

   319	
 > 320	module_pci_driver(ls7a_spi_pci_driver);
   321	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54914 bytes --]

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

[kernel test robot]

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

Hi Qing,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on robh/for-next linus/master v5.10-rc7 next-20201208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
        git checkout 6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/spi/spi-ls7a.c: In function 'ls7a_spi_write_read':
   drivers/spi/spi-ls7a.c:162:19: warning: variable 'ls7a_spi' set but not used [-Wunused-but-set-variable]
     162 |  struct ls7a_spi *ls7a_spi;
         |                   ^~~~~~~~
   drivers/spi/spi-ls7a.c: At top level:
>> drivers/spi/spi-ls7a.c:320:1: warning: data definition has no type or storage class
     320 | module_pci_driver(ls7a_spi_pci_driver);
         | ^~~~~~~~~~~~~~~~~
>> drivers/spi/spi-ls7a.c:320:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
>> drivers/spi/spi-ls7a.c:320:1: warning: parameter names (without types) in function declaration
   drivers/spi/spi-ls7a.c:313:26: warning: 'ls7a_spi_pci_driver' defined but not used [-Wunused-variable]
     313 | static struct pci_driver ls7a_spi_pci_driver = {
         |                          ^~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +320 drivers/spi/spi-ls7a.c

   319	
 > 320	module_pci_driver(ls7a_spi_pci_driver);
   321	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 54914 bytes --]

Re: [PATCH v2 2/4] spi: Add devicetree bindings documentation for Loongson SPI

[zhangqing]

On 12/08/2020 10:48 PM, Sergei Shtylyov wrote:
> On 12/8/20 1:47 PM, zhangqing wrote:
>
>>>> Add spi-ls7a binding documentation.
>>>>
>>>> Signed-off-by: Qing Zhang <zhangqing@loongson.cn>
>>>> ---
>>>>    Documentation/devicetree/bindings/spi/spi-ls7a.txt | 31 ++++++++++++++++++++++
>>>>    1 file changed, 31 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-ls7a.txt b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>>> new file mode 100644
>>>> index 0000000..56247b5
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/spi/spi-ls7a.txt
>>>> @@ -0,0 +1,31 @@
>>>> +Binding for LOONGSON LS7A SPI controller
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be "pci0014,7a0b.0","pci0014,7a0b","pciclass088000","pciclass0880".
>>>> +- reg: reference IEEE Std 1275-1994.
>>>> +- #address-cells: <1>, as required by generic SPI binding.
>>>> +- #size-cells: <0>, also as required by generic SPI binding.
>>>> +- #interrupts: No hardware interrupt.
>>>     You say it's a required prop, yet yuoe example doesn't have it...
>>          I want to emphasize here that LS7A SPI has no hardware interrupts, and DT is not actually used.
>     The why document the property at all?
           Thank you for your reply again,

           I will remove the #interrupt attribute in the third edition.
>
>>>> +
>>>> +Child nodes as per the generic SPI binding.
>>>> +
>>>> +Example:
>>>> +
>>>> +            spi@16,0 {
>>>> +                compatible = "pci0014,7a0b.0",
>>>> +                        "pci0014,7a0b",
>>>> +                        "pciclass088000",
>>>> +                        "pciclass0880";
>>>> +
>>>> +                #address-cells = <1>;
>>>> +                #size-cells = <0>;
>>>> +                reg = <0xb000 0x0 0x0 0x0 0x0>;
>>>> +                num-chipselects = <0>;
>>>> +                spiflash: s25fl016k@0 {
>>>> +                #address-cells = <1>;
>>>> +                #size-cells = <1>;
>>>     Once more?
>>>
>>>> +                compatible ="spansion,s25fl016k","jedec,spi-nor";
>>>     Once more?
>>>
>>>> + spi-max-frequency=<50000000>;
>>>> +                reg=<0>;
>>>     Once more? Did you mean this for a child node?
>>         Yes, these are child node attributes, the child node splash is not necessary.
>     You should indent the child nodes with 1 more tab...
         I will do it and send the v3 in the soon.
>
>>>> +                };
>>>> +            };
>>       Thanks
>>
>>       -Qing
> MBR, Sergei

Re: [PATCH v2 2/4] spi: Add devicetree bindings documentation for Loongson SPI

[zhangqing]

On 12/08/2020 09:58 PM, Mark Brown wrote:
> On Tue, Dec 08, 2020 at 06:47:10PM +0800, zhangqing wrote:
>> On 12/08/2020 04:40 PM, Sergei Shtylyov wrote:
>>>> +Required properties:
>>>> +- #interrupts: No hardware interrupt.
>>>     You say it's a required prop, yet yuoe example doesn't have it...
>>          I want to emphasize here that LS7A SPI has no hardware interrupts,
>> and DT is not actually used.
> There is no need to do this, and documenting the property as required
> just makes things confusing here.
      Thanks for your suggestion,

     I will remove the #interrupt attribute in the third edition.

     Thanks,

     -Qing

>
>

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

[zhangqing]

Hi Brown,

Thank you for your suggestions, these are achievable, I will send v3 in 
the soon.

Before sending v3, I would like to trouble you to see if this is 
correct. It has been tested locally.

On 12/08/2020 09:56 PM, Mark Brown wrote:
> On Tue, Dec 08, 2020 at 03:44:24PM +0800, Qing Zhang wrote:
>
>> v2:
>> - keep Kconfig and Makefile sorted
>> - make the entire comment a C++ one so things look more intentional
> You say this but...
>
>> +++ b/drivers/spi/spi-ls7a.c
>> @@ -0,0 +1,324 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Loongson LS7A SPI Controller driver
>> + *
>> + * Copyright (C) 2020 Loongson Technology Corporation Limited
>> + */
> ...this is still a mix of C and C++ comments?
       Replace all with //

>
>> +static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device  *spi, int val)
>> +{
>> +	int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select);
>> +
>> +	if (spi->mode  & SPI_CS_HIGH)
>> +		val = !val;
>> +	ls7a_spi_write_reg(ls7a_spi, SFCS,
>> +		(val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs);
>> +
>> +	return 0;
>> +}
> Why not just expose this to the core and let it handle things?
>
> Please also write normal conditional statements to improve legibility.
> There's quite a lot of coding style issues in this with things like
> missing spaces
     static void ls7a_spi_set_cs(struct spi_device *spi, bool enable)
{
         struct ls7a_spi *ls7a_spi;

         int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << 
spi->chip_select));

         ls7a_spi = spi_master_get_devdata(spi->master);

         if (!!(spi->mode & SPI_CS_HIGH) == enable)
                 val = (0x11 << spi->chip_select) | cs;
         else
                 val = (0x1 << spi->chip_select) | cs;

         ls7a_spi_write_reg(ls7a_spi, SFCS, val);
}

      static int ls7a_spi_pci_probe---->

      +master->set_cs = ls7a_spi_set_cs;

>
>> +	if (t) {
>> +		hz = t->speed_hz;
>> +		if (!hz)
>> +			hz = spi->max_speed_hz;
>> +	} else
>> +		hz = spi->max_speed_hz;
> If one branch of the conditional has braces please use them on both to
> improve legibility.
>
>> +static int  ls7a_spi_transfer_one_message(struct spi_master *master,
>> +                                         struct spi_message *m)
> I don't understand why the driver is implementing transfer_one_message()
> - it looks like this is just open coding the standard loop that the
> framework provides and should just be using transfer_one().

static int  ls7a_spi_transfer_one(struct spi_master *master,
                       struct spi_device *spi,
                                   struct spi_transfer *t)
{
     struct ls7a_spi *ls7a_spi;
     int param, status;

     ls7a_spi = spi_master_get_devdata(master);

     spin_lock(&ls7a_spi->lock);
     param = ls7a_spi_read_reg(ls7a_spi, PARA);
     ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
     spin_unlock(&ls7a_spi->lock);

         status = ls7a_spi_do_transfer(ls7a_spi, spi, t);
         if(status < 0)
                 return status;

         if(t->len)
         r = ls7a_spi_write_read(spi, t);

         spin_lock(&ls7a_spi->lock);
     ls7a_spi_write_reg(ls7a_spi, PARA, param);
     spin_unlock(&ls7a_spi->lock);

     return status;
}

   static int ls7a_spi_pci_probe---->

  - master->transfer_one_message = ls7a_spi_transfer_one_message;
  +master->transfer_one = ls7a_spi_transfer_one;
>
>> +		r = ls7a_spi_write_read(spi, t);
>> +		if (r < 0) {
>> +			status = r;
>> +			goto error;
>> +			}
> The indentation here isn't following the kernel coding style.
>
>> +	master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
>> +	if (!master)
>> +		return -ENOMEM;
> Why not use devm_ here?

- master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));

   error:
- spi_put_master(master);

+ master = devm_spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));

>
>> +	ret = devm_spi_register_master(dev, master);
>> +	if (ret)
>> +		goto err_free_master;
> The driver uses devm_spi_register_master() here but...
>
>> +static void ls7a_spi_pci_remove(struct pci_dev *pdev)
>> +{
>> +	struct spi_master *master = pci_get_drvdata(pdev);
>> +	struct ls7a_spi *spi;
>> +
>> +	spi = spi_master_get_devdata(master);
>> +	if (!spi)
>> +		return;
>> +
>> +	pci_release_regions(pdev);
> ...releases the PCI regions in the remove() function before the SPI
> controller is freed so the controller could still be active.

      static void ls7a_spi_pci_remove(struct pci_dev *pdev)
{
         struct spi_master *master = pci_get_drvdata(pdev);

      + spi_unregister_master(master);
         pci_release_regions(pdev);
}

Thanks,

-Qing

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

[Mark Brown]

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

On Wed, Dec 09, 2020 at 03:24:15PM +0800, zhangqing wrote:

> > > +static int  ls7a_spi_transfer_one_message(struct spi_master *master,
> > > +                                         struct spi_message *m)

> > I don't understand why the driver is implementing transfer_one_message()
> > - it looks like this is just open coding the standard loop that the
> > framework provides and should just be using transfer_one().

> static int  ls7a_spi_transfer_one(struct spi_master *master,
>                       struct spi_device *spi,
>                                   struct spi_transfer *t)
> {
>     struct ls7a_spi *ls7a_spi;
>     int param, status;
> 
>     ls7a_spi = spi_master_get_devdata(master);
> 
>     spin_lock(&ls7a_spi->lock);
>     param = ls7a_spi_read_reg(ls7a_spi, PARA);
>     ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
>     spin_unlock(&ls7a_spi->lock);

I don't know what this does but is it better split out into a
prepare_message()?  It was only done once per message in your previous
implementation.  Or possibly runtime PM would be even better if that's
what it's doing.

> > ...releases the PCI regions in the remove() function before the SPI
> > controller is freed so the controller could still be active.
> 
>      static void ls7a_spi_pci_remove(struct pci_dev *pdev)
> {
>         struct spi_master *master = pci_get_drvdata(pdev);
> 
>      + spi_unregister_master(master);
>         pci_release_regions(pdev);
> }

You also need to change to using plain spi_register_master() but yes.
Otherwise everything looked good.

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