On 10/18/2012 04:47 AM, Laxman Dewangan wrote:
Tegra20/Tegra30 supports the spi interface through its SLINK
controller. Add spi driver for SLINK controller.
diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c
+static inline void tegra_slink_writel(struct tegra_slink_data *tspi,
+ unsigned long val, unsigned long reg)
+{
+ writel(val, tspi-base + reg);
+
+ /* Read back register to make sure that register writes completed */
+ if (reg != SLINK_TX_FIFO)
+ readl(tspi-base + SLINK_MAS_DATA);
Is that really necessary on every single write, or only for certain
specific operations? This seems rather heavy-weight.
+static void tegra_slink_clear_status(struct tegra_slink_data *tspi)
+{
+ unsigned long val;
+ unsigned long val_write = 0;
+
+ val = tegra_slink_readl(tspi, SLINK_STATUS);
+
+ /* Write 1 to clear status register */
+ val_write = SLINK_RDY;
+ val_write |= (val SLINK_FIFO_ERROR);
Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
write only if the status was previously asserted? If that is true, how
do you avoid a race condition where the bit gets set in SLINK_STATUS
after you read it but before you write to clear it?
+ tegra_slink_writel(tspi, val_write, SLINK_STATUS);
+}
+static unsigned tegra_slink_calculate_curr_xfer_param(
+ if (bits_per_word == 8 || bits_per_word == 16) {
+ tspi-is_packed = 1;
+ tspi-words_per_32bit = 32/bits_per_word;
Spaces required around all operators. Does this pass checkpatch? No:
total: 1405 errors, 11 warnings, 1418 lines checked
+static unsigned tegra_slink_fill_tx_fifo_from_client_txbuf(
+ if (tspi-is_packed) {
+ nbytes = tspi-curr_dma_words * tspi-bytes_per_word;
+ max_n_32bit = (min(nbytes, tx_empty_count*4) - 1)/4 + 1;
+ for (count = 0; count max_n_32bit; ++count) {
Very minor almost irrelevant nit: count++ would be much more typical here.
+ x = 0;
+ for (i = 0; (i 4) nbytes; i++, nbytes--)
+ x |= (*tx_buf++) (i*8);
+ tegra_slink_writel(tspi, x, SLINK_TX_FIFO);
+ }
+ written_words = min(max_n_32bit * tspi-words_per_32bit,
+ tspi-curr_dma_words);
The calculations here seem a little over-complex. Why not calculate the
FIFO space in words above, and just set written words based on that.
Something like:
fifo_words_left = tx_empty_count * spi-words_per_32bit;
written_words = min(fifo_words_left, tspi-curr_dma_words);
nbytes = written_words * spi-bytes_per_word;
max_n_32bit = (nbytes + 3) / 4;
Most likely a similar comment applies to all the other similar functions
for filling FIFOs and buffers.
In fact, I suspect you can completely get rid of the if (is_packed)
statement here by appropriately parameterising the code using variables
in *spi...
+static unsigned int tegra_slink_read_rx_fifo_to_client_rxbuf(
+ bits_per_word = t-bits_per_word ? t-bits_per_word :
+ tspi-cur_spi-bits_per_word;
That logic is repeated a few times. Shouldn't there be a macro to avoid
cut/paste. Perhaps even in the SPI core rather than this driver.
+ rx_mask = (1 bits_per_word) - 1;
+ for (count = 0; count rx_full_count; ++count) {
+ x = tegra_slink_readl(tspi, SLINK_RX_FIFO);
+ x = rx_mask;
I don't think you need that mask; the loop below only extracts bytes
that actually exist in the FIFO right?
+ for (i = 0; (i tspi-bytes_per_word); ++i)
+ *rx_buf++ = (x (i*8)) 0xFF;
+static void tegra_slink_copy_client_txbuf_to_spi_txbuf(
+ struct tegra_slink_data *tspi, struct spi_transfer *t)
Are there no cases where we can simply DMA straight into the client
buffer? I suppose it would be limited to cache-line-aligned
cache-line-size-aligned buffers which is probably unlikely in general:-(
+static int tegra_slink_start_dma_based_transfer(
+ struct tegra_slink_data *tspi, struct spi_transfer *t)
+ /* Make sure that Rx and Tx fifo are empty */
+ status = tegra_slink_readl(tspi, SLINK_STATUS);
+ if ((status SLINK_FIFO_EMPTY) != SLINK_FIFO_EMPTY)
+ dev_err(tspi-dev,
+ Rx/Tx fifo are not empty status 0x%08lx\n, status);
Shouldn't this return an error, or drain the FIFO, or something?
+static int tegra_slink_init_dma_param(struct tegra_slink_data *tspi,
+ if (dma_to_memory) {
+ dma_sconfig.src_addr = tspi-phys + SLINK_RX_FIFO;
+ dma_sconfig.dst_addr = tspi-phys + SLINK_RX_FIFO;
+ } else {
+ dma_sconfig.src_addr = tspi-phys + SLINK_TX_FIFO;
+ dma_sconfig.dst_addr = tspi-phys + SLINK_TX_FIFO;
+ }
