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.
okay, I got it.
+ 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 will use read_poll_timeout or similar api in iopoll.h for this.
+ 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 got it.
...
+ 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()?
okay, I got it.
+ 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();
No wrong, I don't notice it and I will use pcim_iomap_regions.
+ }
+
+ 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?
I will remove that.
+ {0, 0, 0, 0, 0, 0, 0}
What is this? Why {} is not working for you?
okay, I will use it.
+};
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res == NULL) {
Why not using devm_platform_ioremap_resource()?
okay, I got 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.
+ { }
+};
...
+#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 */