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

[Romain Perier]

Hello,

Le 05/12/2016 à 14:37, Mark Brown a écrit :
> On Thu, Dec 01, 2016 at 11:27:16AM +0100, Romain Perier wrote:
>
>> 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)

>
>> -	int i;
>> +	int i, ret = 0;
>
> Coding style - don't combine initialized and non-initalized variables on
> one line.

Ok

>
>>  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

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