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
