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

[zhuyinbo]

在 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