Re: [PATCH v11 2/2] spi: loongson: add bus driver for the loongson spi controller
From: zhuyinbo
Date: Wed May 24 2023 - 03:52:27 EST
在 2023/5/23 下午8:54, andy.shevchenko@xxxxxxxxx 写道:
Mon, May 22, 2023 at 03:10:30PM +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.
It's polite to add reviewers of the previous versions to the Cc list.
okay, I got it.
...
+static void loongson_spi_set_clk(struct loongson_spi *loongson_spi, unsigned int hz)
+{
+ unsigned char val;
+ unsigned int div, div_tmp;
+ const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
static?
okay, I will define "static const char rdiv".
+
+ 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;
+}
...
+static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
+ struct spi_device *spi, struct spi_transfer *t)
+{
+ unsigned int hz;
+
+ if (t)
+ hz = t->speed_hz;
And if t is NULL? hz will be uninitialized. Don't you get a compiler warning?
(Always test your code with `make W=1 ...`)
I alwasy use `make W=1` and I don't find any warnig, but that what you
said was right and I will initial hz.
+ if (hz && loongson_spi->hz != hz)
+ loongson_spi_set_clk(loongson_spi, hz);
+
+ if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK)
+ loongson_spi_set_mode(loongson_spi, spi);
+
+ return 0;
+}
...
+ readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr,
+ (loongson_spi->spsr & 0x1) != 1, 1, MSEC_PER_SEC);
Wouldn't be better to use ' == 0' in the conditional? Or if you think your
approach is better (to show the exact expectation) the definition of the bit 0
might help
#define LOONGSON_... BIT(0)
okay, I got it.
readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr,
(loongson_spi->spsr & LOONGSON_...) != LOONGSON_...,
1, MSEC_PER_SEC);
...
+ do {
+ if (loongson_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
+ goto out;
break;
+ count--;
+ } while (count);
} while (--count);
?
okay, I got it.
+out:
+ return xfer->len - count;
Shouldn't you return an error code if the write failed?
okay, I got it. I will add a error code for return when write failed.
...
+ master = devm_spi_alloc_master(dev, sizeof(struct loongson_spi));
+ if (master == NULL)
if (!master)
+ return -ENOMEM;
Why do you use deprecated naming? Can you use spi_controller* instead of
spi_master* in all cases?
It seems was a personal code style issue and I don't find the
differences between spi_controller and spi_master, Using spi_controller*
is also acceptable to me and I will use spi_controller* instead of
spi_master* in all cases.
...
+ master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
= SPI_MODE_X_MASK | SPI_CS_HIGH;
...
+ clk = devm_clk_get_optional(dev, NULL);
+ if (!IS_ERR(clk))
+ spi->clk_rate = clk_get_rate(clk);
+ else
Redundant. Just check for the error first as it's very usual pattern in the
Linux kernel.
Like below ?
clk = devm_clk_get_optional(dev, NULL);
- if (!IS_ERR(clk))
- spi->clk_rate = clk_get_rate(clk);
- else
+ if (IS_ERR(clk))
return dev_err_probe(dev, PTR_ERR(clk), "unable to get
clock\n");
+ spi->clk_rate = clk_get_rate(clk);
loongson_spi_reginit(spi);
+ return dev_err_probe(dev, PTR_ERR(clk), "unable to get clock\n");
...
+static void loongson_spi_pci_unregister(struct pci_dev *pdev)
+{
+ pcim_iounmap_regions(pdev, BIT(0));
Not needed due to 'm' in the API name, which means "managed".
+ pci_disable_device(pdev);
This is simply wrong. We don't do explicit clean up for managed resources.
+}
That said, drop the ->remove() completely.
okay, I will drop the ->remove() completely.
...
+static struct pci_device_id loongson_spi_devices[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a0b) },
+ { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a1b) },
+ { },
No comma for the terminator entry. It's by definition "terminating" something.
okay, I got it.
+};
...
+#include <linux/of.h>
There is no user of this header. Please, replace with what actually is being
used (presumably mod_devicetable.h and maybe others).
okay, I got it.
+#include <linux/platform_device.h>
+
+#include "spi-loongson.h"
+
+static int loongson_spi_platform_probe(struct platform_device *pdev)
+{
+ int ret;
+ void __iomem *reg_base;
+ struct device *dev = &pdev->dev;
+
+ reg_base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(reg_base))
+ return PTR_ERR(reg_base);
+
+ ret = loongson_spi_init_master(dev, reg_base);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to initialize master\n");
+
+ return ret;
return 0;
It seems was more appropriate that initialize ret then return ret.
Do you think so ?
+}
...
+#ifndef __LINUX_SPI_LOONGSON_H
+#define __LINUX_SPI_LOONGSON_H
+
+#include <linux/bits.h>
+#include <linux/device.h>
This header is not used.
okay, I got it.
+#include <linux/pm.h>
+#include <linux/spi/spi.h>
This neither.
That other .c file seems to need it and I will move it to other .c
code file.
+#include <linux/types.h>
For them use forward declarations
struct device;
struct spi_controller;
The rest of the inclusions is correct.
okay, I got 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
Where is this used outside of the main driver?
These definitions are only used in core.c
+/* Bits definition for Loongson SPI register */
+#define LOONGSON_SPI_PARA_MEM_EN BIT(0)
+#define LOONGSON_SPI_SPCR_CPHA BIT(2)
+#define LOONGSON_SPI_SPCR_CPOL BIT(3)
+#define LOONGSON_SPI_SPCR_SPE BIT(6)
+#define LOONGSON_SPI_SPSR_WCOL BIT(6)
+#define LOONGSON_SPI_SPSR_SPIF BIT(7)
+
+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;
+};
+
+int loongson_spi_init_master(struct device *dev, void __iomem *reg);
+extern const struct dev_pm_ops loongson_spi_dev_pm_ops;
+#endif /* __LINUX_SPI_LOONGSON_H */