Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 21, 2019 at 3:00 PM Masahisa Kojima
<masahisa.kojima@xxxxxxxxxx> wrote:
>
> This patch adds support for controller found on synquacer platforms.

> +       case 8:
> +               {
> +               u8 *buf = sspi->rx_buf;
> +
> +               readsb(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len);
> +               sspi->rx_buf = buf + len;
> +               break;
> +               }

Slightly better style
case FOO: {
  ...
}

> +       /* Full Duplex only on 1-bit wide bus */
> +       if (xfer->rx_buf && xfer->tx_buf &&
> +           (xfer->rx_nbits != 1 || xfer->tx_nbits != 1)) {
> +               dev_err(sspi->dev,
> +                       "RX and TX bus widths must match for Full-Duplex!\n");

The message is not telling full truth. Not only match, but also be equal 1.

> +               return -EINVAL;
> +       }

> +static int synquacer_spi_transfer_one(struct spi_master *master,
> +                                     struct spi_device *spi,
> +                                     struct spi_transfer *xfer)
> +{
> +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
> +       int ret;
> +       int status = 0;
> +       unsigned int words;
> +       u8 bpw;
> +       u32 val;
> +
> +       val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG);
> +       val |= SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH;
> +       val |= SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH;
> +       writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG);
> +
> +       /*
> +        * See if we can transfer 4-bytes as 1 word
> +        * to maximize the FIFO buffer effficiency

Typo here, and period is missed.

> +        */

> +       case 8:
> +               words = xfer->len;
> +               break;
> +       case 16:
> +               words = xfer->len / 2;
> +               break;
> +       default:
> +               words = xfer->len / 4;
> +               break;

Hmm... Shouldn't be rather "less then or equal" comparisons?

> +       unsigned int retries = 0xfffff;

Hmm... better to use decimal value.

> +       /* Disable module */
> +       writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_MCTRL);

> +       while ((readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_MCTRL) &
> +               SYNQUACER_HSSPI_MCTRL_MES) && --retries)
> +               cpu_relax();
> +       if (!retries)
> +               return -EBUSY;

And here something like
do {
} while (--retries)

would look slightly better due to understanding that we do at least
one iteration.

Also, can readx_poll_timeout be used here?

> +static irqreturn_t sq_spi_tx_handler(int irq, void *priv)
> +{
> +       uint32_t val;
> +       struct synquacer_spi *sspi = priv;
> +
> +       val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_TXF);
> +
> +       if (val & SYNQUACER_HSSPI_TXF_FIFO_EMPTY) {
> +               if (sspi->tx_words == 0) {
> +                       writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE);
> +                       complete(&sspi->transfer_done);

> +                       return 0;

irqreturn_t type, please. We have corresponding defines.

> +               }
> +               write_fifo(sspi);
> +       }
> +
> +       return 0;

Ditto.

> +}

> +static int synquacer_spi_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct spi_master *master;
> +       struct synquacer_spi *sspi;
> +       struct resource *res;
> +       int ret;
> +       int rx_irq, tx_irq;
> +
> +       master = spi_alloc_master(&pdev->dev, sizeof(*sspi));
> +       if (!master)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, master);
> +
> +       sspi = spi_master_get_devdata(master);
> +       sspi->dev = &pdev->dev;
> +

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       sspi->regs = devm_ioremap_resource(sspi->dev, res);

devm_platform_ioremap_resource()

> +       if (IS_ERR(sspi->regs)) {
> +               ret = PTR_ERR(sspi->regs);
> +               goto put_spi;
> +       }

> +       } else {
> +               dev_err(&pdev->dev, "specified wrong clock source\n");
> +               ret = -EINVAL;
> +               goto put_spi;
> +       }

Not an issue for ACPI.

> +       if (IS_ERR(sspi->clk)) {
> +               dev_err(&pdev->dev, "clock not found\n");
> +               ret = PTR_ERR(sspi->clk);
> +               goto put_spi;
> +       }
> +
> +       sspi->aces = of_property_read_bool(np, "socionext,set-aces");
> +       sspi->rtm = of_property_read_bool(np, "socionext,use-rtm");
> +
> +       master->num_chipselect = SYNQUACER_HSSPI_NUM_CHIP_SELECT;
> +
> +       init_completion(&sspi->transfer_done);
> +
> +       rx_irq = platform_get_irq(pdev, 0);
> +       if (rx_irq < 0)
> +               dev_err(&pdev->dev, "get rx_irq failed\n");
> +
> +       tx_irq = platform_get_irq(pdev, 1);
> +       if (tx_irq < 0)
> +               dev_err(&pdev->dev, "get tx_irq failed\n");
> +
> +       ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler,
> +                               0, "synquacer-spi-rx", sspi);
> +       ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler,
> +                               0, "synquacer-spi-tx", sspi);
> +
> +       ret = clk_prepare_enable(sspi->clk);
> +       if (ret)
> +               goto put_spi;
> +
> +       master->dev.of_node = np;
> +       master->auto_runtime_pm = true;
> +       master->bus_num = pdev->id;
> +
> +       master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL |
> +                           SPI_TX_QUAD | SPI_RX_QUAD;
> +       master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(24) |
> +                                    SPI_BPW_MASK(16) | SPI_BPW_MASK(8);
> +
> +       master->max_speed_hz = clk_get_rate(sspi->clk);
> +       master->min_speed_hz = master->max_speed_hz / 254;
> +
> +       master->set_cs = synquacer_spi_set_cs;
> +       master->transfer_one = synquacer_spi_transfer_one;
> +
> +       ret = synquacer_spi_enable(master);
> +       if (ret)
> +               goto fail_enable;
> +
> +       pm_runtime_set_active(sspi->dev);
> +       pm_runtime_enable(sspi->dev);
> +
> +       ret = devm_spi_register_master(sspi->dev, master);
> +       if (ret)
> +               goto disable_pm;
> +
> +       return 0;
> +
> +disable_pm:
> +       pm_runtime_disable(sspi->dev);
> +fail_enable:
> +       clk_disable_unprepare(sspi->clk);
> +put_spi:
> +       spi_master_put(master);
> +
> +       return ret;
> +}


> +       if (!pm_runtime_suspended(dev))

This is not enough to check.

> +       if (!pm_runtime_suspended(dev)) {

Ditto.

-- 
With Best Regards,
Andy Shevchenko



[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux