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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3623 bytes --]

On Tue, May 21, 2019 at 08:59:58PM +0900, Masahisa Kojima wrote:

> +	switch (sspi->bpw) {
> +	case 8:

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

Please indent these properly.

> +	default:
> +		{
> +		u32 *buf = sspi->rx_buf;
> +
> +		readsl(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len);
> +		sspi->rx_buf = buf + len;
> +		break;
> +		}

It'd be better to explicitly list the values this works for and return
an error otherwise.

> +	if (sspi->rx_words) {
> +		val = SYNQUACER_HSSPI_RXE_FIFO_MORE_THAN_THRESHOLD |
> +		      SYNQUACER_HSSPI_RXE_SLAVE_RELEASED;
> +		writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_RXE);
> +		status = wait_for_completion_timeout(&sspi->transfer_done,
> +			msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC));
> +		writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE);
> +	}
> +
> +	if (xfer->tx_buf) {
> +		val = SYNQUACER_HSSPI_TXE_FIFO_EMPTY;
> +		writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_TXE);
> +		status = wait_for_completion_timeout(&sspi->transfer_done,
> +			msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC));
> +		writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE);
> +	}

I guess the TX will complete before the RX usually so I'd kind of expect
the waits to be in the other order?

> +	if (status < 0) {
> +		dev_err(sspi->dev, "failed to transfer\n");
> +		return status;
> +	}

Printing the error code could be helpful for users.

> +static void synquacer_spi_set_cs(struct spi_device *spi, bool enable)
> +{
> +	struct synquacer_spi *sspi = spi_master_get_devdata(spi->master);
> +	u32 val;
> +
> +	val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART);
> +	val &= ~(SYNQUACER_HSSPI_DMPSEL_CS_MASK <<
> +		 SYNQUACER_HSSPI_DMPSEL_CS_SHIFT);
> +	val |= spi->chip_select << SYNQUACER_HSSPI_DMPSEL_CS_SHIFT;
> +
> +	if (enable) {
> +		val |= SYNQUACER_HSSPI_DMSTOP_STOP;
> +		writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART);
> +
> +		if (sspi->rx_buf) {
> +			u32 buf[SYNQUACER_HSSPI_FIFO_DEPTH];
> +
> +			sspi->rx_buf = buf;
> +			sspi->rx_words = SYNQUACER_HSSPI_FIFO_DEPTH;
> +			read_fifo(sspi);
> +		}

This is doing things with the FIFO, that's completely inappropriate for
a set_cs() operation.  The set_cs() operation should set the chip select
and nothing else.

> +static irqreturn_t sq_spi_rx_handler(int irq, void *priv)
> +{
> +	uint32_t val;
> +	struct synquacer_spi *sspi = priv;
> +
> +	val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_RXF);
> +	if ((val & SYNQUACER_HSSPI_RXF_SLAVE_RELEASED) ||
> +	    (val & SYNQUACER_HSSPI_RXF_FIFO_MORE_THAN_THRESHOLD))
> +		read_fifo(sspi);
> +
> +	if (sspi->rx_words == 0) {
> +		writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE);
> +		complete(&sspi->transfer_done);
> +	}
> +
> +	return 0;
> +}

0 is not a valid return from an interrupt handler, IRQ_HANDLED or
IRQ_NONE.

> +	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);

The code looked awfully like we depend on having interrupts?  

> +	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL |
> +			    SPI_TX_QUAD | SPI_RX_QUAD;

I don't see any code in the driver that configures dual or quad mode
support other than setting _PCC_SAFESYNC, I'm not clear how the driver
supports these modes?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]