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

From: zhuyinbo <zhuyinbo@loongson.cn>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, Jianmin Lv <lvjianmin@loongson.cn>,
	wanghongliang@loongson.cn, Liu Peibao <liupeibao@loongson.cn>,
	loongson-kernel@lists.loongnix.cn, zhuyinbo@loongson.cn
Subject: Re: [PATCH v12 2/2] spi: loongson: add bus driver for the loongson spi controller
Date: Fri, 9 Jun 2023 15:10:05 +0800	[thread overview]
Message-ID: <44239068-e0ac-1dc8-337e-fb44f5266097@loongson.cn> (raw)
In-Reply-To: <CAHp75VfrPX=VsXMry0Dg_Y4zgt59S=uY=rxCZzv8fBvr_w+i-g@mail.gmail.com>

在 2023/6/8 下午6:15, Andy Shevchenko 写道:
> On Thu, Jun 8, 2023 at 10:28 AM Yinbo Zhu <zhuyinbo@loongson.cn> 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@gmail.com>
> 

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