On Tue, Jan 18, 2022 at 10:42 AM Li-hao Kuo <lhjeff911@xxxxxxxxx> wrote:
>
> Add spi driver for Sunplus SP7021.
...
> Changes in v6:
Thanks for update, my comments below.
...
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/spi/spi.h>
...
> + data_status = readl(pspim->s_base + SP7021_DATA_RDY_REG);
> + writel(data_status | SP7021_SLAVE_CLR_INT, pspim->s_base + SP7021_DATA_RDY_REG);
Wouldn't be
data_status = readl(pspim->s_base + SP7021_DATA_RDY_REG);
data_status |= SP7021_SLAVE_CLR_INT;
writel(data_status, pspim->s_base + SP7021_DATA_RDY_REG);
better to read? Same question to other places like this.
...
> + writel(SP7021_SLAVE_DMA_EN | SP7021_SLAVE_DMA_RW | FIELD_PREP(SP7021_SLAVE_DMA_CMD, 3),
> + pspim->s_base + SP7021_SLAVE_DMA_CTRL_REG);
Temporary variable?
var = ...
...
> + writel(readl(pspim->s_base + SP7021_DATA_RDY_REG) | SP7021_SLAVE_DATA_RDY,
> + pspim->s_base + SP7021_DATA_RDY_REG);
Ditto.
...
> +int sp7021_spi_slave_rx(struct spi_device *spi, struct spi_transfer *xfer)
> +{
> + struct sp7021_spi_ctlr *pspim = spi_controller_get_devdata(spi->controller);
> + int ret = 0;
Unused.
...
> + writel(SP7021_SLAVE_DMA_EN | FIELD_PREP(SP7021_SLAVE_DMA_CMD, 3),
> + pspim->s_base + SP7021_SLAVE_DMA_CTRL_REG);
Temporary variable?
...
> +static irqreturn_t sp7021_spi_master_irq(int irq, void *dev)
> +{
> + struct sp7021_spi_ctlr *pspim = dev;
> + unsigned int tx_cnt, total_len;
> + unsigned int tx_len, rx_cnt;
> + unsigned int fd_status;
> + unsigned long flags;
Why do you need this?
> + bool isrdone = false;
> + u32 value;
> + return IRQ_HANDLED;
> +}
...
> + div = clk_rate / xfer->speed_hz;
> + if (div < 2)
> + div = 2;
div = max(2U, clk_rate / xfer->speed_hz); ?
...
> +static int sp7021_spi_master_transfer_one(struct spi_controller *ctlr, struct spi_device *spi,
> + struct spi_transfer *xfer)
> +{
> + struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
> + unsigned long timeout = msecs_to_jiffies(1000);
> + unsigned int xfer_cnt, xfer_len, last_len;
> + unsigned int i, len_temp;
> + u32 reg_temp;
> + int ret;
Seems redundant.
> + xfer_cnt = xfer->len / SP7021_SPI_DATA_SIZE;
> + last_len = xfer->len % SP7021_SPI_DATA_SIZE;
> +
> + for (i = 0; i <= xfer_cnt; i++) {
> + mutex_lock(&pspim->buf_lock);
> + sp7021_prep_transfer(ctlr, spi);
> + sp7021_spi_setup_clk(ctlr, xfer);
> + reinit_completion(&pspim->isr_done);
> + if (i == xfer_cnt)
> + xfer_len = last_len;
> + else
> + xfer_len = SP7021_SPI_DATA_SIZE;
> +
> + pspim->tx_buf = xfer->tx_buf + i * SP7021_SPI_DATA_SIZE;
> + pspim->rx_buf = xfer->rx_buf + i * SP7021_SPI_DATA_SIZE;
> +
> + if (pspim->tx_cur_len < xfer_len) {
> + len_temp = min(pspim->data_unit, xfer_len);
> + sp7021_spi_master_wb(pspim, len_temp);
> + }
> + reg_temp = readl(pspim->m_base + SP7021_SPI_CONFIG_REG);
> + reg_temp &= ~SP7021_CLEAN_RW_BYTE;
> + reg_temp &= ~SP7021_CLEAN_FLUG_MASK;
> + reg_temp |= SP7021_FD_SEL | SP7021_FINISH_FLAG_MASK |
> + SP7021_TX_EMP_FLAG_MASK | SP7021_RX_FULL_FLAG_MASK |
> + FIELD_PREP(SP7021_TX_UNIT, 0) | FIELD_PREP(SP7021_RX_UNIT, 0);
> + writel(reg_temp, pspim->m_base + SP7021_SPI_CONFIG_REG);
> +
> + reg_temp = FIELD_PREP(SP7021_SET_TX_LEN, xfer_len) |
> + FIELD_PREP(SP7021_SET_XFER_LEN, xfer_len) |
> + SP7021_SPI_START_FD;
> + writel(reg_temp, pspim->m_base + SP7021_SPI_STATUS_REG);
> +
> + if (!wait_for_completion_interruptible_timeout(&pspim->isr_done, timeout)) {
> + dev_err(&spi->dev, "wait_for_completion err\n");
> + return -ETIMEDOUT;
> + }
> +
> + reg_temp = readl(pspim->m_base + SP7021_SPI_STATUS_REG);
> + if (reg_temp & SP7021_FINISH_FLAG) {
> + writel(SP7021_FINISH_FLAG, pspim->m_base + SP7021_SPI_STATUS_REG);
> + writel(readl(pspim->m_base + SP7021_SPI_CONFIG_REG) &
> + SP7021_CLEAN_FLUG_MASK, pspim->m_base + SP7021_SPI_CONFIG_REG);
> + }
> +
> + if (pspim->xfer_conf & SP7021_CPOL_FD)
> + writel(pspim->xfer_conf, pspim->m_base + SP7021_SPI_CONFIG_REG);
> +
> + mutex_unlock(&pspim->buf_lock);
> + ret = 0;
> + }
> + return ret;
> +}
...
> +static int sp7021_spi_slave_transfer_one(struct spi_controller *ctlr, struct spi_device *spi,
> + struct spi_transfer *xfer)
> +{
> + struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
> + struct device *dev = pspim->dev;
> + int mode, ret = 0;
ret assignment can be moved to the default case below, where it will
naturally fit.
> + mode = SP7021_SPI_IDLE;
> + if (xfer->tx_buf && xfer->rx_buf) {
> + dev_dbg(&ctlr->dev, "%s() wrong command\n", __func__);
> + ret = -EINVAL;
Do you need this check if you properly set the capabilities of the controller?
If still needed, why not return here?
> + } else if (xfer->tx_buf) {
> + xfer->tx_dma = dma_map_single(dev, (void *)xfer->tx_buf,
> + xfer->len, DMA_TO_DEVICE);
> + if (dma_mapping_error(dev, xfer->tx_dma))
> + return -ENOMEM;
> + mode = SP7021_SLAVE_WRITE;
> + } else if (xfer->rx_buf) {
> + xfer->rx_dma = dma_map_single(dev, xfer->rx_buf, xfer->len,
> + DMA_FROM_DEVICE);
> + if (dma_mapping_error(dev, xfer->rx_dma))
> + return -ENOMEM;
> + mode = SP7021_SLAVE_READ;
> + }
> +
> + switch (mode) {
> + case SP7021_SLAVE_WRITE:
> + ret = sp7021_spi_slave_tx(spi, xfer);
> + break;
> + case SP7021_SLAVE_READ:
> + ret = sp7021_spi_slave_rx(spi, xfer);
> + break;
> + default:
> + break;
> + }
> + if (xfer->tx_buf)
> + dma_unmap_single(dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE);
> + if (xfer->rx_buf)
> + dma_unmap_single(dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE);
Why can't you use SPI core DMA mapping code?
> + spi_finalize_current_transfer(ctlr);
> + return ret;
> +}
...
> + device_set_node(&ctlr->dev, pdev->dev.fwnode);
Use dev_fwnode() in the second argument.
--
With Best Regards,
Andy Shevchenko
