Re: [PATCH v3 2/5] spi: armada-3700: Add support for the FIFO mode

Hello,

Le 05/12/2016 à 14:37, Mark Brown a écrit :

On Thu, Dec 01, 2016 at 11:27:16AM +0100, Romain Perier wrote:

quoted

Changes in v3:
 - Don't enable the fifo mode based on the compatible string, we introduce
   a module parameter "pio_mode". By default this option is set to zero, so
   the fifo mode is enabled. Pass pio_mode=1 to the driver enables the PIO
   mode.

Why?  If the hardware supports a more efficient mode of operation it
doesn't seem sensible not to use it.

That's just that our customer want to keep both modes, this is why I 
decided to use the more efficient mode by default and disable it via a 
module parameter. Previously the more efficient mode was always enabled, 
based on the "compatible string" of the DT (the PIO mode was simply 
useless in this case and dead code)

quoted

-	int i;
+	int i, ret = 0;

Coding style - don't combine initialized and non-initalized variables on
one line.

Ok

quoted

 static int a3700_spi_transfer_one(struct spi_master *master,
 				  struct spi_device *spi,
 				  struct spi_transfer *xfer)
 {
 	struct a3700_spi *a3700_spi = spi_master_get_devdata(master);
-	int ret = 0;
+	int ret;

 	a3700_spi_transfer_setup(spi, xfer);

@@ -505,6 +737,151 @@ static int a3700_spi_transfer_one(struct spi_master *master,
 	return ret;
 }

This appears to be a random unrelated change, should probably be part of
the initial driver commit.

Good catch

quoted

+static int a3700_spi_fifo_transfer_one(struct spi_master *master,
+				       struct spi_device *spi,
+				       struct spi_transfer *xfer)
+{
+	struct a3700_spi *a3700_spi = spi_master_get_devdata(master);
+	int ret = 0, timeout = A3700_SPI_TIMEOUT;
+	unsigned int nbits = 0;
+	u32 val;
+
+	a3700_spi_transfer_setup(spi, xfer);
+
+	a3700_spi->tx_buf  = xfer->tx_buf;
+	a3700_spi->rx_buf  = xfer->rx_buf;
+	a3700_spi->buf_len = xfer->len;
+
+	/* SPI transfer headers */
+	a3700_spi_header_set(a3700_spi);
+
+	if (xfer->tx_buf)
+		nbits = xfer->tx_nbits;
+	else if (xfer->rx_buf)
+		nbits = xfer->rx_nbits;
+
+	a3700_spi_pin_mode_set(a3700_spi, nbits);
+
+	if (xfer->rx_buf) {
+		/* Set read data length */
+		spireg_write(a3700_spi, A3700_SPI_IF_DIN_CNT_REG,
+			     a3700_spi->buf_len);
+		/* Start READ transfer */
+		val = spireg_read(a3700_spi, A3700_SPI_IF_CFG_REG);
+		val &= ~A3700_SPI_RW_EN;
+		val |= A3700_SPI_XFER_START;
+		spireg_write(a3700_spi, A3700_SPI_IF_CFG_REG, val);
+	} else if (xfer->tx_buf) {
+		/* Start Write transfer */

So this only supports single duplex transfers but there doesn't seem to
be anything that enforces this.

A flag or a capability, typically? I will investigate

Thanks,
Romain

-- 
Romain Perier, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html