Re: [PATCH v12 2/2] spi: loongson: add bus driver for the loongson spi controller

zhuyinbo <zhuyinbo@xxxxxxxxxxx> · Fri, 9 Jun 2023 15:10:05 +0800

在 2023/6/8 下午6:15, Andy Shevchenko 写道:
On Thu, Jun 8, 2023 at 10:28 AM Yinbo Zhu <zhuyinbo@xxxxxxxxxxx> wrote:

This bus driver supports the Loongson SPI hardware controller in the
Loongson platforms and supports to use DTS and PCI framework to

the use

okay, I got it.

register SPI device resources.

Thank you for an update. I have a few nit-picks below, but in general
this version is good (esp. if you address them)
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

You're welcome, I will add the reviewed-by in v13.

...

+static void loongson_spi_set_cs(struct spi_device *spi, bool val)
+{
+       int cs;
+       struct loongson_spi *loongson_spi = spi_controller_get_devdata(spi->controller);
+
+       cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG)
+                                          & ~(0x11 << spi_get_chipselect(spi, 0));
+       loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG,
+                                      (val ? (0x11 << spi_get_chipselect(spi, 0)) :
+                                      (0x1 << spi_get_chipselect(spi, 0))) | cs);

Can be done as

static void loongson_spi_set_cs(struct spi_device *spi, bool en)

     unsigned char mask = (BIT(4) | BIT(0)) << spi_get_chipselect(spi, 0);
     unsigned char val = en ? mask :  (BIT(0) << spi_get_chipselect(spi, 0));

     cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG) & ~mask;
     loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG, val | cs);

okay, I will do it.

(Renamed variables to be consistent with the other uses in the driver below)

sorry, I don't got it. Are you referring to the following changes ?
loongson_spi_set_cs(spi, 1) => loongson_spi_set_cs(spi, true)

+}
+
+static void loongson_spi_set_clk(struct loongson_spi *loongson_spi, unsigned int hz)
+{
+       unsigned char val;
+       unsigned int div, div_tmp;
+       static const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
+
+       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);

     val &= GENMASK(1, 0);

This seems to be "val &= ~GENMASK(1, 0);"

+       loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, (val & ~3) |
+                              loongson_spi->spcr);

        loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, val
| loongson_spi->spcr);

okay, I got it.

+       val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);

     val &= GENMASK(1, 0);

This seems to be "val &= ~GENMASK(1, 0);"

+       loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, (val & ~3) |
+                              loongson_spi->sper);

        loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, val
| loongson_spi->sper);

okay,  I got it.

+       loongson_spi->hz = hz;
+}
+
+static void loongson_spi_set_mode(struct loongson_spi *loongson_spi,
+                                 struct spi_device *spi)
+{
+       unsigned char val;
+
+       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;
+}
+
+static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
+                               struct spi_device *spi, struct spi_transfer *t)
+{
+       if (t && loongson_spi->hz != t->speed_hz)
+               loongson_spi_set_clk(loongson_spi, t->speed_hz);
+
+       if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK)
+               loongson_spi_set_mode(loongson_spi, spi);
+
+       return 0;
+}
+
+static int loongson_spi_setup(struct spi_device *spi)
+{
+       struct loongson_spi *loongson_spi;
+
+       loongson_spi = spi_controller_get_devdata(spi->controller);
+       if (spi->bits_per_word % 8)
+               return -EINVAL;
+
+       if (spi_get_chipselect(spi, 0) >= spi->controller->num_chipselect)
+               return -EINVAL;
+
+       loongson_spi->hz = 0;
+       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)
+{
+       int ret;
+       struct loongson_spi *loongson_spi = spi_controller_get_devdata(spi->controller);
+
+       if (tx_buf && *tx_buf)
+               loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, *((*tx_buf)++));
+       else
+               loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, 0);

+ Blank line

okay, I will add the blank line.

+       ret = readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr,
+                       (loongson_spi->spsr & 0x1) != LOONGSON_SPI_SPSR_RFEMPTY, 1, MSEC_PER_SEC);

                        (loongson_spi->spsr &
LOONGSON_SPI_SPSR_RFEMPTY) != LOONGSON_SPI_SPSR_RFEMPTY,
                        1, MSEC_PER_SEC);

okay, I got it.

+       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 ret;
+}
+
+static int loongson_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
+{
+       int ret;
+       unsigned int count;
+       const u8 *tx = xfer->tx_buf;
+       u8 *rx = xfer->rx_buf;
+
+       count = xfer->len;

+

Unneeded blank line.

okay, I will remove the blank line.

+       do {
+               ret = loongson_spi_write_read_8bit(spi, &tx, &rx, count);
+               if (ret)
+                       break;
+       } while (--count);
+
+       return ret;
+}
+
+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);

BIT(0) ?
LOONGSON_SPI_PARA_MEM_EN ?

I will use LOONGSON_SPI_PARA_MEM_EN.

+       return 0;
+}
+

...

+int loongson_spi_init_controller(struct device *dev, void __iomem *regs)
+{
+       struct spi_controller *controller;
+       struct loongson_spi *spi;
+       struct clk *clk;
+
+       controller = devm_spi_alloc_host(dev, sizeof(struct loongson_spi));
+       if (controller == NULL)
+               return -ENOMEM;
+
+       controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;

    ... = SPI_MODE_X_MASK | SPI_CS_HIGH;

okay, I got it.

Thanks,
Yinbo