This bus driver supports the Loongson spi hardware controller in the
Loongson platforms and supports to use DTS and PCI framework to
register spi device resources.
Signed-off-by: Yinbo Zhu <[email protected]>
---
MAINTAINERS | 4 +
drivers/spi/Kconfig | 31 ++++
drivers/spi/Makefile | 3 +
drivers/spi/spi-loongson-core.c | 296 ++++++++++++++++++++++++++++++++
drivers/spi/spi-loongson-pci.c | 72 ++++++++
drivers/spi/spi-loongson-plat.c | 47 +++++
drivers/spi/spi-loongson.h | 41 +++++
7 files changed, 494 insertions(+)
create mode 100644 drivers/spi/spi-loongson-core.c
create mode 100644 drivers/spi/spi-loongson-pci.c
create mode 100644 drivers/spi/spi-loongson-plat.c
create mode 100644 drivers/spi/spi-loongson.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 0dcf4cfbe178..4a629b9b5a4e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12162,6 +12162,10 @@ M: Yinbo Zhu <[email protected]>
L: [email protected]
S: Maintained
F: Documentation/devicetree/bindings/spi/loongson,ls2k-spi.yaml
+F: drivers/spi/spi-loongson-core.c
+F: drivers/spi/spi-loongson-pci.c
+F: drivers/spi/spi-loongson-plat.c
+F: drivers/spi/spi-loongson.h
LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
M: Sathya Prakash <[email protected]>
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index cbf60b6a931c..87f87ee35d27 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -509,6 +509,37 @@ config SPI_LM70_LLP
which interfaces to an LM70 temperature sensor using
a parallel port.
+config SPI_LOONGSON_CORE
+ tristate "Loongson SPI Controller Core Driver Support"
+ depends on LOONGARCH || COMPILE_TEST
+ help
+ This core driver supports the Loongson spi hardware controller in
+ the Loongson platforms.
+ Say Y or M here if you want to use the SPI controller on
+ Loongson platform.
+
+config SPI_LOONGSON_PCI
+ tristate "Loongson SPI Controller PCI Driver Support"
+ select SPI_LOONGSON_CORE
+ depends on PCI && (LOONGARCH || COMPILE_TEST)
+ help
+ This bus driver supports the Loongson spi hardware controller in
+ the Loongson platforms and supports to use PCI framework to
+ register spi device resources.
+ Say Y or M here if you want to use the SPI controller on
+ Loongson platform.
+
+config SPI_LOONGSON_PLATFORM
+ tristate "Loongson SPI Controller Platform Driver Support"
+ select SPI_LOONGSON_CORE
+ depends on OF && (LOONGARCH || COMPILE_TEST)
+ help
+ This bus driver supports the Loongson spi hardware controller in
+ the Loongson platforms and supports to use DTS framework to
+ register spi device resources.
+ Say Y or M here if you want to use the SPI controller on
+ Loongson platform.
+
config SPI_LP8841_RTC
tristate "ICP DAS LP-8841 SPI Controller for RTC"
depends on MACH_PXA27X_DT || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d87cf75bee6a..a4931aa64476 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -70,6 +70,9 @@ obj-$(CONFIG_SPI_INTEL_PLATFORM) += spi-intel-platform.o
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_LOONGSON_CORE) += spi-loongson-core.o
+obj-$(CONFIG_SPI_LOONGSON_PCI) += spi-loongson-pci.o
+obj-$(CONFIG_SPI_LOONGSON_PLATFORM) += spi-loongson-plat.o
obj-$(CONFIG_SPI_LP8841_RTC) += spi-lp8841-rtc.o
obj-$(CONFIG_SPI_MESON_SPICC) += spi-meson-spicc.o
obj-$(CONFIG_SPI_MESON_SPIFC) += spi-meson-spifc.o
diff --git a/drivers/spi/spi-loongson-core.c b/drivers/spi/spi-loongson-core.c
new file mode 100644
index 000000000000..4c0b5c5fcab4
--- /dev/null
+++ b/drivers/spi/spi-loongson-core.c
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Loongson SPI Support
+// Copyright (C) 2023 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/err.h>
+#include <linux/spi/spi.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+
+#include "spi-loongson.h"
+
+static inline void loongson_spi_write_reg(struct loongson_spi *spi, unsigned char reg,
+ unsigned char data)
+{
+ writeb(data, spi->base + reg);
+}
+
+static inline char loongson_spi_read_reg(struct loongson_spi *spi, unsigned char reg)
+{
+ return readb(spi->base + reg);
+}
+
+static void loongson_spi_set_cs(struct spi_device *spi, bool val)
+{
+ int cs;
+ struct loongson_spi *loongson_spi = spi_master_get_devdata(spi->master);
+
+ if (loongson_spi->mode & SPI_NO_CS)
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG, 0);
+ else {
+ cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG)
+ & ~(0x11 << spi->chip_select);
+ loongson_spi_write_reg(loongson_spi,
+ LOONGSON_SPI_SFCS_REG,
+ (val ? (0x11 << spi->chip_select) :
+ (0x1 << spi->chip_select)) | cs);
+ }
+}
+
+static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
+ struct spi_device *spi, struct spi_transfer *t)
+{
+ unsigned int hz;
+ unsigned int div, div_tmp;
+ unsigned int bit;
+ 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 && loongson_spi->hz != hz) ||
+ ((spi->mode ^ loongson_spi->mode) & (SPI_CPOL | SPI_CPHA))) {
+ div = DIV_ROUND_UP_ULL(loongson_spi->clk_rate, 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_rate = %llu hz = %d div_tmp = %d bit = %d\n",
+ loongson_spi->clk_rate, hz, div_tmp, bit);
+
+ loongson_spi->hz = hz;
+ loongson_spi->spcr = div_tmp & 3;
+ loongson_spi->sper = (div_tmp >> 2) & 3;
+ val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
+ val &= ~0xc;
+ if (spi->mode & SPI_CPOL)
+ val |= 8;
+ if (spi->mode & SPI_CPHA)
+ val |= 4;
+
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, (val & ~3) |
+ loongson_spi->spcr);
+ val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, (val & ~3) |
+ loongson_spi->sper);
+ loongson_spi->mode &= SPI_NO_CS;
+ loongson_spi->mode |= spi->mode;
+ }
+
+ return 0;
+}
+
+static int loongson_spi_setup(struct spi_device *spi)
+{
+ struct loongson_spi *loongson_spi;
+
+ loongson_spi = spi_master_get_devdata(spi->master);
+ if (spi->bits_per_word % 8)
+ return -EINVAL;
+
+ if (spi->chip_select >= spi->master->num_chipselect)
+ return -EINVAL;
+
+ loongson_spi->hz = 0;
+ loongson_spi->mode &= SPI_NO_CS;
+ loongson_spi_set_cs(spi, 1);
+
+ return 0;
+}
+
+static int loongson_spi_write_read_8bit(struct spi_device *spi, const u8 **tx_buf,
+ u8 **rx_buf, unsigned int num)
+{
+ struct loongson_spi *loongson_spi = spi_master_get_devdata(spi->master);
+ unsigned long timeout = jiffies + SPI_COMPLETION_TIMEOUT;
+
+ if (tx_buf && *tx_buf) {
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, *((*tx_buf)++));
+ while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1 &&
+ time_after(timeout, jiffies))
+ cpu_relax();
+ } else {
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, 0);
+ while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1 &&
+ time_after(timeout, jiffies))
+ cpu_relax();
+ }
+
+ if (rx_buf && *rx_buf)
+ *(*rx_buf)++ = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_FIFO_REG);
+ else
+ loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_FIFO_REG);
+
+ return 0;
+}
+
+static unsigned int loongson_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
+{
+ unsigned int count;
+ const u8 *tx = xfer->tx_buf;
+ u8 *rx = xfer->rx_buf;
+
+ count = xfer->len;
+
+ do {
+ if (loongson_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
+ goto out;
+ count--;
+ } while (count);
+
+out:
+ return xfer->len - count;
+}
+
+static int loongson_spi_prepare_message(struct spi_controller *ctlr, struct spi_message *m)
+{
+ struct loongson_spi *loongson_spi = spi_controller_get_devdata(ctlr);
+
+ loongson_spi->para = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG);
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, loongson_spi->para & ~1);
+
+ return 0;
+}
+
+static int loongson_spi_transfer_one(struct spi_controller *ctrl, struct spi_device *spi,
+ struct spi_transfer *xfer)
+{
+ struct loongson_spi *loongson_spi = spi_master_get_devdata(spi->master);
+
+ loongson_spi_update_state(loongson_spi, spi, xfer);
+ if (xfer->len)
+ xfer->len = loongson_spi_write_read(spi, xfer);
+
+ return 0;
+}
+
+static int loongson_spi_unprepare_message(struct spi_controller *ctrl, struct spi_message *m)
+{
+ struct loongson_spi *loongson_spi = spi_controller_get_devdata(ctrl);
+
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, loongson_spi->para);
+
+ return 0;
+}
+
+static void loongson_spi_reginit(struct loongson_spi *loongson_spi_dev)
+{
+ unsigned char val;
+
+ val = loongson_spi_read_reg(loongson_spi_dev, LOONGSON_SPI_SPCR_REG);
+ val &= ~LOONGSON_SPI_SPCR_SPE;
+ loongson_spi_write_reg(loongson_spi_dev, LOONGSON_SPI_SPCR_REG, val);
+
+ loongson_spi_write_reg(loongson_spi_dev, LOONGSON_SPI_SPSR_REG,
+ (LOONGSON_SPI_SPSR_SPIF | LOONGSON_SPI_SPSR_WCOL));
+
+ val = loongson_spi_read_reg(loongson_spi_dev, LOONGSON_SPI_SPCR_REG);
+ val |= LOONGSON_SPI_SPCR_SPE;
+ loongson_spi_write_reg(loongson_spi_dev, LOONGSON_SPI_SPCR_REG, val);
+}
+
+int loongson_spi_init_master(struct device *dev, struct resource *res)
+{
+ struct spi_master *master;
+ struct loongson_spi *spi;
+ struct clk *clk;
+
+ master = devm_spi_alloc_master(dev, sizeof(struct loongson_spi));
+ if (master == NULL) {
+ dev_info(dev, "master allocation failed\n");
+ return -ENOMEM;
+ }
+
+ master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
+ master->setup = loongson_spi_setup;
+ master->prepare_message = loongson_spi_prepare_message;
+ master->transfer_one = loongson_spi_transfer_one;
+ master->unprepare_message = loongson_spi_unprepare_message;
+ master->set_cs = loongson_spi_set_cs;
+ master->num_chipselect = 4;
+ master->dev.of_node = of_node_get(dev->of_node);
+ dev_set_drvdata(dev, master);
+
+ spi = spi_master_get_devdata(master);
+
+ spi->master = master;
+
+ spi->base = devm_ioremap(dev, res->start, resource_size(res));
+ if (spi->base == NULL) {
+ dev_err(dev, "cannot map io\n");
+ return -ENXIO;
+ }
+
+ clk = devm_clk_get(dev, NULL);
+ if (!IS_ERR(clk))
+ spi->clk_rate = clk_get_rate(clk);
+
+ loongson_spi_reginit(spi);
+
+ spi->mode = 0;
+ if (of_get_property(dev->of_node, "spi-nocs", NULL))
+ spi->mode |= SPI_NO_CS;
+
+ return devm_spi_register_master(dev, master);
+}
+EXPORT_SYMBOL_GPL(loongson_spi_init_master);
+
+static int __maybe_unused loongson_spi_suspend(struct device *dev)
+{
+ struct loongson_spi *loongson_spi;
+ struct spi_master *master;
+
+ master = dev_get_drvdata(dev);
+ spi_master_suspend(master);
+
+ loongson_spi = spi_master_get_devdata(master);
+
+ loongson_spi->spcr = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
+ loongson_spi->sper = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);
+ loongson_spi->spsr = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG);
+ loongson_spi->para = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG);
+ loongson_spi->sfcs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG);
+ loongson_spi->timi = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_TIMI_REG);
+
+ return 0;
+}
+
+static int __maybe_unused loongson_spi_resume(struct device *dev)
+{
+ struct loongson_spi *loongson_spi;
+ struct spi_master *master;
+
+ master = dev_get_drvdata(dev);
+ loongson_spi = spi_master_get_devdata(master);
+
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, loongson_spi->spcr);
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, loongson_spi->sper);
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPSR_REG, loongson_spi->spsr);
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, loongson_spi->para);
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG, loongson_spi->sfcs);
+ loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_TIMI_REG, loongson_spi->timi);
+
+ spi_master_resume(master);
+
+ return 0;
+}
+
+const struct dev_pm_ops loongson_spi_dev_pm_ops = {
+ .suspend = loongson_spi_suspend,
+ .resume = loongson_spi_resume,
+};
+EXPORT_SYMBOL_GPL(loongson_spi_dev_pm_ops);
+
+MODULE_DESCRIPTION("Loongson spi core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/spi/spi-loongson-pci.c b/drivers/spi/spi-loongson-pci.c
new file mode 100644
index 000000000000..f8aaad6577bd
--- /dev/null
+++ b/drivers/spi/spi-loongson-pci.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0+
+// PCI interface driver for Loongson SPI Support
+// Copyright (C) 2023 Loongson Technology Corporation Limited
+
+#include <linux/pci.h>
+
+#include "spi-loongson.h"
+
+static int loongson_spi_pci_register(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ int ret;
+ unsigned char v8;
+ struct resource res[2];
+ struct device *dev = &pdev->dev;
+
+ ret = pci_enable_device(pdev);
+ if (ret < 0) {
+ dev_err(dev, "cannot enable pci device\n");
+ goto err_out;
+ }
+
+ ret = pci_request_region(pdev, 0, "loongson-spi io");
+ if (ret < 0) {
+ dev_err(dev, "cannot request region 0.\n");
+ goto err_out;
+ }
+
+ res[0].start = pci_resource_start(pdev, 0);
+ res[0].end = pci_resource_end(pdev, 0);
+ ret = pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &v8);
+
+ if (ret == PCIBIOS_SUCCESSFUL) {
+ res[1].start = v8;
+ res[1].end = v8;
+ }
+
+ ret = loongson_spi_init_master(dev, res);
+ if (ret)
+ dev_err(dev, "failed to initialize master\n");
+
+err_out:
+ return ret;
+}
+
+static void loongson_spi_pci_unregister(struct pci_dev *pdev)
+{
+ pci_release_region(pdev, 0);
+ pci_disable_device(pdev);
+}
+
+static struct pci_device_id loongson_spi_devices[] = {
+ {PCI_DEVICE(0x14, 0x7a0b)},
+ {PCI_DEVICE(0x14, 0x7a1b)},
+ {0, 0, 0, 0, 0, 0, 0}
+};
+MODULE_DEVICE_TABLE(pci, loongson_spi_devices);
+
+static struct pci_driver loongson_spi_pci_driver = {
+ .name = "loongson-spi-pci",
+ .id_table = loongson_spi_devices,
+ .probe = loongson_spi_pci_register,
+ .remove = loongson_spi_pci_unregister,
+ .driver = {
+ .bus = &pci_bus_type,
+ .pm = &loongson_spi_dev_pm_ops,
+ },
+};
+module_pci_driver(loongson_spi_pci_driver);
+
+MODULE_DESCRIPTION("Loongson spi pci driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/spi/spi-loongson-plat.c b/drivers/spi/spi-loongson-plat.c
new file mode 100644
index 000000000000..acf0fa38ce4b
--- /dev/null
+++ b/drivers/spi/spi-loongson-plat.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Platform driver for Loongson SPI Support
+// Copyright (C) 2023 Loongson Technology Corporation Limited
+
+#include <linux/platform_device.h>
+#include <linux/of.h>
+
+#include "spi-loongson.h"
+
+static int loongson_spi_platform_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ int ret;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res == NULL) {
+ dev_err(dev, "cannot get io resource memory\n");
+ return -ENOENT;
+ }
+
+ ret = loongson_spi_init_master(dev, res);
+ if (ret)
+ dev_err(dev, "failed to initialize master\n");
+
+ return ret;
+}
+
+static const struct of_device_id loongson_spi_id_table[] = {
+ { .compatible = "loongson,ls2k-spi", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, loongson_spi_id_table);
+
+static struct platform_driver loongson_spi_plat_driver = {
+ .probe = loongson_spi_platform_probe,
+ .driver = {
+ .name = "loongson-spi",
+ .bus = &platform_bus_type,
+ .pm = &loongson_spi_dev_pm_ops,
+ .of_match_table = loongson_spi_id_table,
+ },
+};
+module_platform_driver(loongson_spi_plat_driver);
+
+MODULE_DESCRIPTION("Loongson spi platform driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/spi/spi-loongson.h b/drivers/spi/spi-loongson.h
new file mode 100644
index 000000000000..44818340188d
--- /dev/null
+++ b/drivers/spi/spi-loongson.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* Header File for Loongson SPI Driver. */
+/* Copyright (C) 2023 Loongson Technology Corporation Limited */
+
+#ifndef __LINUX_SPI_LOONGSON_H
+#define __LINUX_SPI_LOONGSON_H
+
+#define LOONGSON_SPI_SPCR_REG 0x00
+#define LOONGSON_SPI_SPSR_REG 0x01
+#define LOONGSON_SPI_FIFO_REG 0x02
+#define LOONGSON_SPI_SPER_REG 0x03
+#define LOONGSON_SPI_PARA_REG 0x04
+#define LOONGSON_SPI_SFCS_REG 0x05
+#define LOONGSON_SPI_TIMI_REG 0x06
+
+/* Bits definition for Loongson SPI register */
+#define LOONGSON_SPI_PARA_MEM_EN BIT(0)
+#define LOONGSON_SPI_SPSR_SPIF BIT(7)
+#define LOONGSON_SPI_SPSR_WCOL BIT(6)
+#define LOONGSON_SPI_SPCR_SPE BIT(6)
+
+#define SPI_COMPLETION_TIMEOUT msecs_to_jiffies(2000)
+
+struct loongson_spi {
+ struct spi_master *master;
+ void __iomem *base;
+ int cs_active;
+ unsigned int hz;
+ unsigned char spcr;
+ unsigned char sper;
+ unsigned char spsr;
+ unsigned char para;
+ unsigned char sfcs;
+ unsigned char timi;
+ unsigned int mode;
+ u64 clk_rate;
+};
+
+extern int loongson_spi_init_master(struct device *dev, struct resource *res);
+extern const struct dev_pm_ops loongson_spi_dev_pm_ops;
+#endif /* __LINUX_SPI_LOONGSON_H */
--
2.20.1
On Wed, Apr 26, 2023 at 03:10:45PM +0800, Yinbo Zhu wrote:
> This bus driver supports the Loongson spi hardware controller in the
> Loongson platforms and supports to use DTS and PCI framework to
> register spi device resources.
This breaks an x86 allmodconfig build:
/build/stage/linux/drivers/spi/spi-loongson-core.c: In function ‘loongson_spi_init_master’:
/build/stage/linux/drivers/spi/spi-loongson-core.c:222:31: error: implicit declaration of function ‘of_node_get’ [-Werror=implicit-function-declaration]
222 | master->dev.of_node = of_node_get(dev->of_node);
| ^~~~~~~~~~~
/build/stage/linux/drivers/spi/spi-loongson-core.c:222:29: error: assignment to ‘struct device_node *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
222 | master->dev.of_node = of_node_get(dev->of_node);
| ^
/build/stage/linux/drivers/spi/spi-loongson-core.c:242:13: error: implicit declaration of function ‘of_get_property’ [-Werror=implicit-function-declaration]
242 | if (of_get_property(dev->of_node, "spi-nocs", NULL))
| ^~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Wed, Apr 26, 2023 at 03:10:45PM +0800, Yinbo Zhu kirjoitti:
> This bus driver supports the Loongson spi hardware controller in the
> Loongson platforms and supports to use DTS and PCI framework to
> register spi device resources.
SPI
...
> +config SPI_LOONGSON_CORE
> + tristate "Loongson SPI Controller Core Driver Support"
Does it need to be visible to the user?
> + depends on LOONGARCH || COMPILE_TEST
> + help
> + This core driver supports the Loongson spi hardware controller in
> + the Loongson platforms.
> + Say Y or M here if you want to use the SPI controller on
> + Loongson platform.
...
> +config SPI_LOONGSON_PLATFORM
> + tristate "Loongson SPI Controller Platform Driver Support"
> + select SPI_LOONGSON_CORE
> + depends on OF && (LOONGARCH || COMPILE_TEST)
Is it really dependent to OF? Why?
> + help
> + This bus driver supports the Loongson spi hardware controller in
> + the Loongson platforms and supports to use DTS framework to
> + register spi device resources.
> + Say Y or M here if you want to use the SPI controller on
> + Loongson platform.
...
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/spi/spi.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
Ordered?
...
> + if (loongson_spi->mode & SPI_NO_CS)
> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG, 0);
Missing {}
> + else {
> + cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG)
> + & ~(0x11 << spi->chip_select);
> + loongson_spi_write_reg(loongson_spi,
> + LOONGSON_SPI_SFCS_REG,
> + (val ? (0x11 << spi->chip_select) :
> + (0x1 << spi->chip_select)) | cs);
Too many parentheses.
> + }
...
> + const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
Oh, why?!
...
> + if ((hz && loongson_spi->hz != hz) ||
> + ((spi->mode ^ loongson_spi->mode) & (SPI_CPOL | SPI_CPHA))) {
> + div = DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz);
> + if (div < 2)
> + div = 2;
> + if (div > 4096)
> + div = 4096;
NIH clamp_val()
> + bit = fls(div) - 1;
> + if ((1<<bit) == div)
> + bit--;
> + div_tmp = rdiv[bit];
I believe this can be optimized.
> + dev_dbg(&spi->dev, "clk_rate = %llu hz = %d div_tmp = %d bit = %d\n",
> + loongson_spi->clk_rate, hz, div_tmp, bit);
> +
> + loongson_spi->hz = hz;
> + loongson_spi->spcr = div_tmp & 3;
> + loongson_spi->sper = (div_tmp >> 2) & 3;
> + val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
> + val &= ~0xc;
GENMASK()
> + if (spi->mode & SPI_CPOL)
> + val |= 8;
BIT()
> + if (spi->mode & SPI_CPHA)
> + val |= 4;
> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, (val & ~3) |
> + loongson_spi->spcr);
> + val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);
> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, (val & ~3) |
> + loongson_spi->sper);
> + loongson_spi->mode &= SPI_NO_CS;
> + loongson_spi->mode |= spi->mode;
> + }
...
> + while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1 &&
> + time_after(timeout, jiffies))
> + cpu_relax();
iopoll.h has a suitable macro for this.
...
> + while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1 &&
> + time_after(timeout, jiffies))
> + cpu_relax();
Ditto.
...
> + master = devm_spi_alloc_master(dev, sizeof(struct loongson_spi));
> + if (master == NULL) {
> + dev_info(dev, "master allocation failed\n");
We do not issue a message for ENOMEM
> + return -ENOMEM;
> + }
...
> + master->dev.of_node = of_node_get(dev->of_node);
device_set_node()
...
> + spi->base = devm_ioremap(dev, res->start, resource_size(res));
Why not devm_ioremap_resource()?
> + if (spi->base == NULL) {
> + dev_err(dev, "cannot map io\n");
> + return -ENXIO;
return dev_err_probe();
> + }
...
> + clk = devm_clk_get(dev, NULL);
Can we hav
> + if (!IS_ERR(clk))
Use _optional variant above instead of this.
Do not forget about deferred probe.
> + spi->clk_rate = clk_get_rate(clk);
...
> + if (of_get_property(dev->of_node, "spi-nocs", NULL))
> + spi->mode |= SPI_NO_CS;
Don't we have something in the SPI core to handle this in a generic way?
...
> +EXPORT_SYMBOL_GPL(loongson_spi_init_master);
Please, use _NS variant.
...
> +MODULE_DESCRIPTION("Loongson spi core driver");
SPI
...
> + struct resource res[2];
> + struct device *dev = &pdev->dev;
> +
> + ret = pci_enable_device(pdev);
pcim_enable_device()
> + if (ret < 0) {
> + dev_err(dev, "cannot enable pci device\n");
> + goto err_out;
return dev_err_probe();
> + }
> +
> + ret = pci_request_region(pdev, 0, "loongson-spi io");
> + if (ret < 0) {
> + dev_err(dev, "cannot request region 0.\n");
> + goto err_out;
> + }
> +
> + res[0].start = pci_resource_start(pdev, 0);
> + res[0].end = pci_resource_end(pdev, 0);
What's wrong with pcim_iomap_regions()?
...
> + ret = pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &v8);
What?!
What's wrong with pci_alloc_irq_vectors()?
> +
> + if (ret == PCIBIOS_SUCCESSFUL) {
> + res[1].start = v8;
> + res[1].end = v8;
> + }
> +
> + ret = loongson_spi_init_master(dev, res);
Why not passing the remapped address and IRQ number instead?
> + if (ret)
> + dev_err(dev, "failed to initialize master\n");
return dev_err_probe();
> +
> +err_out:
Completely useless. Return in-line.
> + return ret;
> +}
...
> +static struct pci_device_id loongson_spi_devices[] = {
> + {PCI_DEVICE(0x14, 0x7a0b)},
> + {PCI_DEVICE(0x14, 0x7a1b)},
Can you define vendor ID in pci_ids.h?
> + {0, 0, 0, 0, 0, 0, 0}
What is this? Why {} is not working for you?
> +};
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
Why not using devm_platform_ioremap_resource()?
> + dev_err(dev, "cannot get io resource memory\n");
> + return -ENOENT;
return dev_err_probe();
> + }
> +
> + ret = loongson_spi_init_master(dev, res);
> + if (ret)
> + dev_err(dev, "failed to initialize master\n");
Ditto.
...
> +static const struct of_device_id loongson_spi_id_table[] = {
> + { .compatible = "loongson,ls2k-spi", },
Inned comma is redundant.
> + { }
> +};
...
> +#ifndef __LINUX_SPI_LOONGSON_H
> +#define __LINUX_SPI_LOONGSON_H
Missing bits.h
Missing types.h
Missing declaration for msecs_to_jiffies()
Missing forward declarations for struct spi_master and struct device.
MIssing declaration for dev_pm_ops.
> +#define LOONGSON_SPI_SPCR_REG 0x00
> +#define LOONGSON_SPI_SPSR_REG 0x01
> +#define LOONGSON_SPI_FIFO_REG 0x02
> +#define LOONGSON_SPI_SPER_REG 0x03
> +#define LOONGSON_SPI_PARA_REG 0x04
> +#define LOONGSON_SPI_SFCS_REG 0x05
> +#define LOONGSON_SPI_TIMI_REG 0x06
> +
> +/* Bits definition for Loongson SPI register */
> +#define LOONGSON_SPI_PARA_MEM_EN BIT(0)
> +#define LOONGSON_SPI_SPSR_SPIF BIT(7)
> +#define LOONGSON_SPI_SPSR_WCOL BIT(6)
> +#define LOONGSON_SPI_SPCR_SPE BIT(6)
> +
> +#define SPI_COMPLETION_TIMEOUT msecs_to_jiffies(2000)
> +
> +struct loongson_spi {
> + struct spi_master *master;
> + void __iomem *base;
> + int cs_active;
> + unsigned int hz;
> + unsigned char spcr;
> + unsigned char sper;
> + unsigned char spsr;
> + unsigned char para;
> + unsigned char sfcs;
> + unsigned char timi;
> + unsigned int mode;
> + u64 clk_rate;
> +};
> +
> +extern int loongson_spi_init_master(struct device *dev, struct resource *res);
No extern for the function declarations.
> +extern const struct dev_pm_ops loongson_spi_dev_pm_ops;
> +#endif /* __LINUX_SPI_LOONGSON_H */
--
With Best Regards,
Andy Shevchenko
在 2023/5/8 下午9:20, Mark Brown 写道:
> On Wed, Apr 26, 2023 at 03:10:45PM +0800, Yinbo Zhu wrote:
>> This bus driver supports the Loongson spi hardware controller in the
>> Loongson platforms and supports to use DTS and PCI framework to
>> register spi device resources.
>
> This breaks an x86 allmodconfig build:
>
> /build/stage/linux/drivers/spi/spi-loongson-core.c: In function ‘loongson_spi_init_master’:
> /build/stage/linux/drivers/spi/spi-loongson-core.c:222:31: error: implicit declaration of function ‘of_node_get’ [-Werror=implicit-function-declaration]
> 222 | master->dev.of_node = of_node_get(dev->of_node);
> | ^~~~~~~~~~~
> /build/stage/linux/drivers/spi/spi-loongson-core.c:222:29: error: assignment to ‘struct device_node *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> 222 | master->dev.of_node = of_node_get(dev->of_node);
> | ^
> /build/stage/linux/drivers/spi/spi-loongson-core.c:242:13: error: implicit declaration of function ‘of_get_property’ [-Werror=implicit-function-declaration]
> 242 | if (of_get_property(dev->of_node, "spi-nocs", NULL))
> | ^~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
These errors was due to the function of_node_get and of_get_property
loss a declaration and I will add "#include <linux/of.h>" in
spi-loongson-core.c to fix it.
Thanks.
>
On Mon, May 08, 2023 at 06:04:06PM +0300, [email protected] wrote:
> Wed, Apr 26, 2023 at 03:10:45PM +0800, Yinbo Zhu kirjoitti:
> > + loongson_spi_write_reg(loongson_spi,
> > + LOONGSON_SPI_SFCS_REG,
> > + (val ? (0x11 << spi->chip_select) :
> > + (0x1 << spi->chip_select)) | cs);
> Too many parentheses.
The code is absolutely fine, there is nothing wrong with adding explicit
parentheses even where not strictly needed if it helps to make things
clear (which is obviously always a problem wiht ternery operator use).
Please, stop this sort of nitpicking. It is at times actively unhelpful.
> > + bit = fls(div) - 1;
> > + if ((1<<bit) == div)
> > + bit--;
> > + div_tmp = rdiv[bit];
> I believe this can be optimized.
This isn't constructive feedback, if there is a concrete optimisation
you want to suggest please just suggest it.
> > +EXPORT_SYMBOL_GPL(loongson_spi_init_master);
> Please, use _NS variant.
It really does not matter, the chances of any collisions is pretty much
zero.
On Tue, May 9, 2023 at 7:39 AM Mark Brown <[email protected]> wrote:
> On Mon, May 08, 2023 at 06:04:06PM +0300, [email protected] wrote:
> > Wed, Apr 26, 2023 at 03:10:45PM +0800, Yinbo Zhu kirjoitti:
...
> > > + loongson_spi_write_reg(loongson_spi,
> > > + LOONGSON_SPI_SFCS_REG,
> > > + (val ? (0x11 << spi->chip_select) :
> > > + (0x1 << spi->chip_select)) | cs);
>
> > Too many parentheses.
>
> The code is absolutely fine, there is nothing wrong with adding explicit
> parentheses even where not strictly needed if it helps to make things
> clear (which is obviously always a problem wiht ternery operator use).
> Please, stop this sort of nitpicking. It is at times actively unhelpful.
Okay, sorry for the noise.
...
> > > + bit = fls(div) - 1;
> > > + if ((1<<bit) == div)
> > > + bit--;
> > > + div_tmp = rdiv[bit];
>
> > I believe this can be optimized.
>
> This isn't constructive feedback, if there is a concrete optimisation
> you want to suggest please just suggest it.
It goes together with the other questions in this function. But if you
think about some code proposal, here we are:
_original_
const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
div = DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz);
if (div < 2)
div = 2;
if (div > 4096)
div = 4096;
bit = fls(div) - 1;
if ((1<<bit) == div)
bit--;
div_tmp = rdiv[bit];
loongson_spi->spcr = div_tmp & 3;
loongson_spi->sper = (div_tmp >> 2) & 3;
_proposed_
const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
// as far as I understood the above table
00 00 [0] <= 2
00 01 [1] <= 4
01 00 [2] <= 8
00 10 [3] <= 16
00 11 [4] <= 32
01 01 [5] <= 64
01 10 [6] <= 128
01 11 [7] <= 256
10 00 [8] <= 512
10 01 [9] <= 1024
10 10 [10] <= 2048
10 11 [11] <= 4096
div =
clamp_val(DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz), 2, 4096);
div_tmp = rdiv[fls(div - 1)];
loongson_spi->spcr = (div_tmp & GENMASK(1, 0)) >> 0;
loongson_spi->sper = (div_tmp & GENMASK(3, 2)) >> 2;
But TBH I would expect the author to think about it more.
Also the check can be modified to have less indented code:
if (hz && loongson_spi->hz == hz)
return 0;
if (!((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK))
return 0;
...
> > > +EXPORT_SYMBOL_GPL(loongson_spi_init_master);
>
> > Please, use _NS variant.
>
> It really does not matter, the chances of any collisions is pretty much
> zero.
The point is in preventing us from using those symbols outside of the scope.
--
With Best Regards,
Andy Shevchenko
?? 2023/5/8 ????11:04, [email protected] д??:
> Wed, Apr 26, 2023 at 03:10:45PM +0800, Yinbo Zhu kirjoitti:
>> This bus driver supports the Loongson spi hardware controller in the
>> Loongson platforms and supports to use DTS and PCI framework to
>> register spi device resources.
>
> SPI
okay I got it.
>
> ...
>
>> +config SPI_LOONGSON_CORE
>> + tristate "Loongson SPI Controller Core Driver Support"
>
> Does it need to be visible to the user?
okay, I will set it invisible.
>
>> + depends on LOONGARCH || COMPILE_TEST
>> + help
>> + This core driver supports the Loongson spi hardware controller in
>> + the Loongson platforms.
>> + Say Y or M here if you want to use the SPI controller on
>> + Loongson platform.
>
> ...
>
>> +config SPI_LOONGSON_PLATFORM
>> + tristate "Loongson SPI Controller Platform Driver Support"
>> + select SPI_LOONGSON_CORE
>> + depends on OF && (LOONGARCH || COMPILE_TEST)
>
> Is it really dependent to OF? Why?
Yes, because this driver need depend on device tree.
>
>> + help
>> + This bus driver supports the Loongson spi hardware controller in
>> + the Loongson platforms and supports to use DTS framework to
>> + register spi device resources.
>> + Say Y or M here if you want to use the SPI controller on
>> + Loongson platform.
>
> ...
>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>
> Ordered?
okay, I got it.
>
> ...
>
>> + if (loongson_spi->mode & SPI_NO_CS)
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG, 0);
>
> Missing {}
okay, I got it.
>
>> + else {
>> + cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG)
>> + & ~(0x11 << spi->chip_select);
>> + loongson_spi_write_reg(loongson_spi,
>> + LOONGSON_SPI_SFCS_REG,
>> + (val ? (0x11 << spi->chip_select) :
>> + (0x1 << spi->chip_select)) | cs);
>
> Too many parentheses.
>
>> + }
>
> ...
>
>> + const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
>
> Oh, why?!
As you understand in another mail, you are right!
00 00 [0] <= 2
00 01 [1] <= 4
01 00 [2] <= 8
00 10 [3] <= 16
00 11 [4] <= 32
01 01 [5] <= 64
01 10 [6] <= 128
01 11 [7] <= 256
10 00 [8] <= 512
10 01 [9] <= 1024
10 10 [10] <= 2048
10 11 [11] <= 4096
>
> ...
>
>> + if ((hz && loongson_spi->hz != hz) ||
>> + ((spi->mode ^ loongson_spi->mode) & (SPI_CPOL | SPI_CPHA))) {
>> + div = DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz);
>
>> + if (div < 2)
>> + div = 2;
>> + if (div > 4096)
>> + div = 4096;
>
> NIH clamp_val()
okay, I will use it.
>
>> + bit = fls(div) - 1;
>> + if ((1<<bit) == div)
>> + bit--;
>> + div_tmp = rdiv[bit];
>
> I believe this can be optimized.
okay, I will apply your optimization advice in another mail for my
patch.
>
>> + dev_dbg(&spi->dev, "clk_rate = %llu hz = %d div_tmp = %d bit = %d\n",
>> + loongson_spi->clk_rate, hz, div_tmp, bit);
>> +
>> + loongson_spi->hz = hz;
>> + loongson_spi->spcr = div_tmp & 3;
>> + loongson_spi->sper = (div_tmp >> 2) & 3;
>> + val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
>> + val &= ~0xc;
>
> GENMASK()
okay, I got it.
>
>> + if (spi->mode & SPI_CPOL)
>> + val |= 8;
>
> BIT()
okay, I got it.
>
>> + if (spi->mode & SPI_CPHA)
>> + val |= 4;
>
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, (val & ~3) |
>> + loongson_spi->spcr);
>> + val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, (val & ~3) |
>> + loongson_spi->sper);
>> + loongson_spi->mode &= SPI_NO_CS;
>> + loongson_spi->mode |= spi->mode;
>> + }
>
> ...
>
>> + while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1 &&
>> + time_after(timeout, jiffies))
>> + cpu_relax();
>
> iopoll.h has a suitable macro for this.
okay, I will use read_poll_timeout or similar api in iopoll.h for this.
>
> ...
>
>> + while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1 &&
>> + time_after(timeout, jiffies))
>> + cpu_relax();
>
> Ditto.
okay, I got it.
>
> ...
>
>> + master = devm_spi_alloc_master(dev, sizeof(struct loongson_spi));
>> + if (master == NULL) {
>
>> + dev_info(dev, "master allocation failed\n");
>
> We do not issue a message for ENOMEM
I will remove the dmesg.
>
>> + return -ENOMEM;
>> + }
>
> ...
>
>> + master->dev.of_node = of_node_get(dev->of_node);
>
> device_set_node()
okay, I got it.
>
> ...
>
>> + spi->base = devm_ioremap(dev, res->start, resource_size(res));
>
> Why not devm_ioremap_resource()?
I will use devm_platform_ioremap_resource as your low text advice and
there will be no needed for devm_ioremap_resource.
>
>
>> + if (spi->base == NULL) {
>> + dev_err(dev, "cannot map io\n");
>> + return -ENXIO;
>
> return dev_err_probe();
okay, I got it.
>
>> + }
>
> ...
>
>> + clk = devm_clk_get(dev, NULL);
>
> Can we hav
>
>> + if (!IS_ERR(clk))
>
> Use _optional variant above instead of this.
> Do not forget about deferred probe.
I will use devm_clk_get_optional and dev_err_probe to replace above.
>
>> + spi->clk_rate = clk_get_rate(clk);
>
> ...
>
>> + if (of_get_property(dev->of_node, "spi-nocs", NULL))
>> + spi->mode |= SPI_NO_CS;
>
> Don't we have something in the SPI core to handle this in a generic way?
The 2k500 has two type spi controller. one type was different other
loongson-2 common spi. and other loongson-2 SoCs was only a common type
spi.
loongson-2 common type spi cs:
bit0: spi:csn_en0
bit4: spi_csn0
bit1: spi_csn_en1
bit5: spi_csn1
bit2: spi_csn_en2
bit6: spi_csn2
bit3: spi_csn_en3
bit7: spi_csn3
loongson-2 special type spi one cs:
bit0: spi_csn
bit1: spi_csn_en
The difference between the special type spi and common type spi not only
cs number but has register definition struct. The spi core seems unable
to use generic way to cover it.
and I found it has still has some issues that current code is compatible
special type spi, so I will drop special type spi support and leave only
support for common type spi. Support special type spi as needed in the
future.
>
> ...
>
>
>> +EXPORT_SYMBOL_GPL(loongson_spi_init_master);
>
> Please, use _NS variant.
>
> ...
>
>> +MODULE_DESCRIPTION("Loongson spi core driver");
>
> SPI
okay, I got it.
>
> ...
>
>> + struct resource res[2];
>> + struct device *dev = &pdev->dev;
>> +
>> + ret = pci_enable_device(pdev);
>
> pcim_enable_device()
okay, I got it.
>
>> + if (ret < 0) {
>> + dev_err(dev, "cannot enable pci device\n");
>> + goto err_out;
>
> return dev_err_probe();
okay, I got it.
>
>> + }
>> +
>> + ret = pci_request_region(pdev, 0, "loongson-spi io");
>> + if (ret < 0) {
>> + dev_err(dev, "cannot request region 0.\n");
>> + goto err_out;
>> + }
>> +
>> + res[0].start = pci_resource_start(pdev, 0);
>> + res[0].end = pci_resource_end(pdev, 0);
>
> What's wrong with pcim_iomap_regions()?
No wrong, I don't notice it and I will use pcim_iomap_regions.
>
> ...
>
>> + ret = pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &v8);
>
> What?!
>
> What's wrong with pci_alloc_irq_vectors()?
If the interupt is needed that I will use pci_alloc_irq_vectors.
currently, the driver is not using irq and I may disacard
the parsing of interrupts.
>
>> +
>> + if (ret == PCIBIOS_SUCCESSFUL) {
>> + res[1].start = v8;
>> + res[1].end = v8;
>> + }
>> +
>> + ret = loongson_spi_init_master(dev, res);
>
> Why not passing the remapped address and IRQ number instead?
okay I got it.
>
>> + if (ret)
>> + dev_err(dev, "failed to initialize master\n");
>
> return dev_err_probe();
>
>> +
>> +err_out:
>
> Completely useless. Return in-line.
I will remove the "err_out".
>
>> + return ret;
>> +}
>
> ...
>
>> +static struct pci_device_id loongson_spi_devices[] = {
>> + {PCI_DEVICE(0x14, 0x7a0b)},
>> + {PCI_DEVICE(0x14, 0x7a1b)},
>
> Can you define vendor ID in pci_ids.h?
okay, I will add it into pci_ids.h.
>
>
>> + {0, 0, 0, 0, 0, 0, 0}
>
> What is this? Why {} is not working for you?
I will remove that.
>
>> +};
>
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (res == NULL) {
>
> Why not using devm_platform_ioremap_resource()?
okay, I will use it.
>
>> + dev_err(dev, "cannot get io resource memory\n");
>> + return -ENOENT;
>
> return dev_err_probe();
okay, I got it.
>
>> + }
>> +
>> + ret = loongson_spi_init_master(dev, res);
>> + if (ret)
>> + dev_err(dev, "failed to initialize master\n");
>
> Ditto.
okay, I got it.
>
> ...
>
>> +static const struct of_device_id loongson_spi_id_table[] = {
>> + { .compatible = "loongson,ls2k-spi", },
>
> Inned comma is redundant.
okay, I got it.
>
>> + { }
>> +};
>
> ...
>
>> +#ifndef __LINUX_SPI_LOONGSON_H
>> +#define __LINUX_SPI_LOONGSON_H
>
> Missing bits.h
> Missing types.h
> Missing declaration for msecs_to_jiffies()
> Missing forward declarations for struct spi_master and struct device.
> MIssing declaration for dev_pm_ops.
okay, I will add it.
>
>
>> +#define LOONGSON_SPI_SPCR_REG 0x00
>> +#define LOONGSON_SPI_SPSR_REG 0x01
>> +#define LOONGSON_SPI_FIFO_REG 0x02
>> +#define LOONGSON_SPI_SPER_REG 0x03
>> +#define LOONGSON_SPI_PARA_REG 0x04
>> +#define LOONGSON_SPI_SFCS_REG 0x05
>> +#define LOONGSON_SPI_TIMI_REG 0x06
>> +
>> +/* Bits definition for Loongson SPI register */
>> +#define LOONGSON_SPI_PARA_MEM_EN BIT(0)
>> +#define LOONGSON_SPI_SPSR_SPIF BIT(7)
>> +#define LOONGSON_SPI_SPSR_WCOL BIT(6)
>> +#define LOONGSON_SPI_SPCR_SPE BIT(6)
>> +
>> +#define SPI_COMPLETION_TIMEOUT msecs_to_jiffies(2000)
>> +
>> +struct loongson_spi {
>> + struct spi_master *master;
>> + void __iomem *base;
>> + int cs_active;
>> + unsigned int hz;
>> + unsigned char spcr;
>> + unsigned char sper;
>> + unsigned char spsr;
>> + unsigned char para;
>> + unsigned char sfcs;
>> + unsigned char timi;
>> + unsigned int mode;
>> + u64 clk_rate;
>> +};
>> +
>> +extern int loongson_spi_init_master(struct device *dev, struct resource *res);
>
> No extern for the function declarations.
okay, I got it.
>
>> +extern const struct dev_pm_ops loongson_spi_dev_pm_ops;
>
>> +#endif /* __LINUX_SPI_LOONGSON_H */
>
在 2023/5/10 下午3:03, Andy Shevchenko 写道:
>
> _original_
>
> const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
>
> div = DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz);
> if (div < 2)
> div = 2;
> if (div > 4096)
> div = 4096;
>
> bit = fls(div) - 1;
> if ((1<<bit) == div)
> bit--;
> div_tmp = rdiv[bit];
>
> loongson_spi->spcr = div_tmp & 3;
> loongson_spi->sper = (div_tmp >> 2) & 3;
>
> _proposed_
>
> const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
>
> // as far as I understood the above table
> 00 00 [0] <= 2
> 00 01 [1] <= 4
> 01 00 [2] <= 8
> 00 10 [3] <= 16
> 00 11 [4] <= 32
> 01 01 [5] <= 64
> 01 10 [6] <= 128
> 01 11 [7] <= 256
> 10 00 [8] <= 512
> 10 01 [9] <= 1024
> 10 10 [10] <= 2048
> 10 11 [11] <= 4096
>
> div =
> clamp_val(DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz), 2, 4096);
> div_tmp = rdiv[fls(div - 1)];
>
> loongson_spi->spcr = (div_tmp & GENMASK(1, 0)) >> 0;
> loongson_spi->sper = (div_tmp & GENMASK(3, 2)) >> 2;
>
> But TBH I would expect the author to think about it more.
In previous code, if mode need to be updated but clk not to be updated
then the clk settting code will be also run. so, I think it is better
to confiure clk and mode separately.
if (hz && loongson_spi->hz != hz) {
div = clamp_val(DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz), 2,
4096);
div_tmp = rdiv[fls(div - 1)];
loongson_spi->spcr = (div_tmp & GENMASK(1, 0)) >> 0;
loongson_spi->sper = (div_tmp & GENMASK(3, 2)) >> 2;
val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG,
(val & ~3) | loongson_spi->spcr);
val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);
loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG,
(val & ~3) |loongson_spi->sper);
loongson_spi->hz = hz;
}
if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK) {
val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
val &= ~(LOONGSON_SPI_SPCR_CPOL | LOONGSON_SPI_SPCR_CPHA);
if (spi->mode & SPI_CPOL)
val |= LOONGSON_SPI_SPCR_CPOL;
if (spi->mode & SPI_CPHA)
val |= LOONGSON_SPI_SPCR_CPHA;
loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, val);
loongson_spi->mode |= spi->mode;
}
>
> Also the check can be modified to have less indented code:
>
> if (hz && loongson_spi->hz == hz)
> return 0;
>
> if (!((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK))
> return 0;
The setting register about clk and mode was the same SPCR register, so
only the clk and mode don't need to be updated in the same, then return
0, so the code seems to be as follows. But setting clk and mode
separately doesn't require follows judgement.
if ((hz && loongson_spi->hz == hz) &&
!((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK))
return 0;
Thanks.
>
>
>
On Fri, May 12, 2023 at 11:12 AM zhuyinbo <[email protected]> wrote:
> 在 2023/5/10 下午3:03, Andy Shevchenko 写道:
...
> so, I think it is better
> to confiure clk and mode separately.
Agree, but at the same time you can split the conditional bodies to
the separate functions. In this case the indentation of each of them
can be decreased.
...
> > Also the check can be modified to have less indented code:
> >
> > if (hz && loongson_spi->hz == hz)
> > return 0;
> >
> > if (!((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK))
> > return 0;
>
> The setting register about clk and mode was the same SPCR register, so
> only the clk and mode don't need to be updated in the same, then return
> 0, so the code seems to be as follows. But setting clk and mode
> separately doesn't require follows judgement.
>
> if ((hz && loongson_spi->hz == hz) &&
> !((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK))
> return 0;
Correct, sorry for the mistake I made.
--
With Best Regards,
Andy Shevchenko
在 2023/5/12 下午5:40, Andy Shevchenko 写道:
> On Fri, May 12, 2023 at 11:12 AM zhuyinbo <[email protected]> wrote:
>> 在 2023/5/10 下午3:03, Andy Shevchenko 写道:
>
> ...
>
>> so, I think it is better
>> to confiure clk and mode separately.
>
> Agree, but at the same time you can split the conditional bodies to
> the separate functions. In this case the indentation of each of them
> can be decreased.
okay, I got it.
Thanks.
>
在 2023/5/11 下午3:18, zhuyinbo 写道:
>
>
> 在 2023/5/8 下午11:04, [email protected] 写道:
>>
>>> +config SPI_LOONGSON_CORE
>>> + tristate "Loongson SPI Controller Core Driver Support"
>>
>> Does it need to be visible to the user?
>
I try to set it invisible that by removing the SPI_LOONGSON_CORE Kconfig
or removing "tristate "Loongson SPI Controller Core Driver Support" that
will cause spi-core driver doesn't be compiled or compiled fail issue,
so I will still set it visible to the user.
>>
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + if (res == NULL) {
>>
>> Why not using devm_platform_ioremap_resource()?
> okay, I will use it.
>>
>>> + dev_err(dev, "cannot get io resource memory\n");
>>> + return -ENOENT;
>>
>> return dev_err_probe();
It seems that there is no need to print memory log when use
devm_platform_ioremap_resource because this function had contained
the this memory log print thus I will return PTR_ERR(reg_base).
reg_base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(reg_base))
return PTR_ERR(reg_base);
Thanks.
>>
>>
Mon, May 15, 2023 at 04:14:00PM +0800, zhuyinbo kirjoitti:
> 在 2023/5/11 下午3:18, zhuyinbo 写道:
> > 在 2023/5/8 下午11:04, [email protected] 写道:
...
> > > > +config SPI_LOONGSON_CORE
> > > > + tristate "Loongson SPI Controller Core Driver Support"
> > >
> > > Does it need to be visible to the user?
>
> I try to set it invisible that by removing the SPI_LOONGSON_CORE Kconfig
> or removing "tristate "Loongson SPI Controller Core Driver Support" that
> will cause spi-core driver doesn't be compiled or compiled fail issue,
> so I will still set it visible to the user.
Making a symbol selectable only can be achieved by removing the description
(near to tristate directive), have you tried that?
...
> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > + if (res == NULL) {
> > >
> > > Why not using devm_platform_ioremap_resource()?
> > okay, I will use it.
> > >
> > > > + dev_err(dev, "cannot get io resource memory\n");
> > > > + return -ENOENT;
> > >
> > > return dev_err_probe();
>
> It seems that there is no need to print memory log when use
> devm_platform_ioremap_resource because this function had contained
> the this memory log print thus I will return PTR_ERR(reg_base).
>
> reg_base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(reg_base))
> return PTR_ERR(reg_base);
Good catch! Sure, go with this.
--
With Best Regards,
Andy Shevchenko
在 2023/5/15 下午5:15, [email protected] 写道:
> Mon, May 15, 2023 at 04:14:00PM +0800, zhuyinbo kirjoitti:
>> 在 2023/5/11 下午3:18, zhuyinbo 写道:
>>> 在 2023/5/8 下午11:04, [email protected] 写道:
>
> ...
>
>>>>> +config SPI_LOONGSON_CORE
>>>>> + tristate "Loongson SPI Controller Core Driver Support"
>>>>
>>>> Does it need to be visible to the user?
>>
>> I try to set it invisible that by removing the SPI_LOONGSON_CORE Kconfig
>> or removing "tristate "Loongson SPI Controller Core Driver Support" that
>> will cause spi-core driver doesn't be compiled or compiled fail issue,
>> so I will still set it visible to the user.
>
> Making a symbol selectable only can be achieved by removing the description
> (near to tristate directive), have you tried that?
Is it like this ?
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -517,7 +517,7 @@ config SPI_LM70_LLP
a parallel port.
config SPI_LOONGSON_CORE
- tristate "Loongson SPI Controller Core Driver Support"
+ tristate
depends on LOONGARCH || COMPILE_TEST
Thanks.
>
> ...
>
>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> + if (res == NULL) {
>>>>
>>>> Why not using devm_platform_ioremap_resource()?
>>> okay, I will use it.
>>>>
>>>>> + dev_err(dev, "cannot get io resource memory\n");
>>>>> + return -ENOENT;
>>>>
>>>> return dev_err_probe();
>>
>> It seems that there is no need to print memory log when use
>> devm_platform_ioremap_resource because this function had contained
>> the this memory log print thus I will return PTR_ERR(reg_base).
>>
>> reg_base = devm_platform_ioremap_resource(pdev, 0);
>> if (IS_ERR(reg_base))
>> return PTR_ERR(reg_base);
>
> Good catch! Sure, go with this.
>
On Mon, May 15, 2023 at 3:01 PM zhuyinbo <[email protected]> wrote:
> 在 2023/5/15 下午5:15, [email protected] 写道:
> > Mon, May 15, 2023 at 04:14:00PM +0800, zhuyinbo kirjoitti:
> >> 在 2023/5/11 下午3:18, zhuyinbo 写道:
> >>> 在 2023/5/8 下午11:04, [email protected] 写道:
...
> >>>>> +config SPI_LOONGSON_CORE
> >>>>> + tristate "Loongson SPI Controller Core Driver Support"
> >>>>
> >>>> Does it need to be visible to the user?
> >>
> >> I try to set it invisible that by removing the SPI_LOONGSON_CORE Kconfig
> >> or removing "tristate "Loongson SPI Controller Core Driver Support" that
> >> will cause spi-core driver doesn't be compiled or compiled fail issue,
> >> so I will still set it visible to the user.
> >
> > Making a symbol selectable only can be achieved by removing the description
> > (near to tristate directive), have you tried that?
>
> Is it like this ?
Yes.
--
With Best Regards,
Andy Shevchenko
在 2023/5/15 下午9:54, Andy Shevchenko 写道:
> On Mon, May 15, 2023 at 3:01 PM zhuyinbo <[email protected]> wrote:
>> 在 2023/5/15 下午5:15, [email protected] 写道:
>>> Mon, May 15, 2023 at 04:14:00PM +0800, zhuyinbo kirjoitti:
>>>> 在 2023/5/11 下午3:18, zhuyinbo 写道:
>>>>> 在 2023/5/8 下午11:04, [email protected] 写道:
>
> ...
>
>>>>>>> +config SPI_LOONGSON_CORE
>>>>>>> + tristate "Loongson SPI Controller Core Driver Support"
>>>>>>
>>>>>> Does it need to be visible to the user?
>>>>
>>>> I try to set it invisible that by removing the SPI_LOONGSON_CORE Kconfig
>>>> or removing "tristate "Loongson SPI Controller Core Driver Support" that
>>>> will cause spi-core driver doesn't be compiled or compiled fail issue,
>>>> so I will still set it visible to the user.
>>>
>>> Making a symbol selectable only can be achieved by removing the description
>>> (near to tristate directive), have you tried that?
>>
>> Is it like this ?
>
> Yes.
okay, I got it.
Thanks.
>